AntiSnatch0r

Sometimes you’ve certainly heard about automated vs manual penetration testing, how the latter is better in terms of discovering security issues, and so on.

As we (IntegratingWeb) are developers and committers of the Opentaps ERP/CRM open source project, I’m spending some of my time analyzing the vast amount of source code of the application to find exploitable points and security issues, in order to create a more secure product.

During my research I’ve found some logic security flaws, that surely you cannot discover using static analysis or other automated tools.

The flaw was present in the implementation of the updatePassword logic, by which a user can update his password: in case of an admin, he can update third party passwords too. The issue was that the checks implemented to see if the user was actually an admin were flawed: if the user had the PARTYMGR_UPDATE CRUD permission, a SecurityPermission (speaking in the OFBiz language)  that EVERY PARTY must have (using default permissions) in order to modify his profile data, then he could modify third party passwords. That means INCLUDING THE ADMIN ONE. More than this, any checks on the current passwords were skipped: we didn’t need to know the old admin password before changing it to a new one.

You can understand that in this circumstances it was so easy for me to build an attack vector, to exploit this kind of behavior with a XSRF.

I’m using plain Javascript for my easy attack vector, without relying on any ajax frameworsk for XMLHTTPRequests.

document.body.innerHTML += ‘<form id=”maliciousform” action=”http://localhost:8080/partymgr/control/updatePassword” method=”post”><input type=”hidden” name=”userLoginId” value=”euronymous666″><input type=”hidden” name=”partyId” value=”10010″><input type=”hidden” name=”currentPassword” value=”blabla”><input type=”hidden” name=”newPassword” value=”passwordwedontknow”><input type=”hidden” name=”newPasswordVerify” value=”hardpasswd”><input type=”hidden” name=”passwordHint” value=”"></form>’;
document.getElementById(”maliciousform”).submit();

I’ve changed the code in the trunk to check for ADMIN permissions instead of simple UPDATE permissions, because I suppose that most
custom permission implementations are actually creating some users with full admin privileges, and then other user groups
(such as customers, in e-commerce applications) that have more restrictive permission such as:

<SecurityGroupPermission groupId=”CUSTOMER” permissionId=”PARTYMGR_CREATE”/>
<SecurityGroupPermission groupId=”CUSTOMER” permissionId=”PARTYMGR_UPDATE”/>

Users of the group CUSTOMERS just need to update their profile, change mail or change password, and can eventually use the forgot password
link.

The point here is that basically enforcement on current password should not removed in any circumstances, even on admin
users: if someone doesn’t remember his password, he can use the forgot password service (not so secure in these days
of DNS bug proliferation, Kaminsky said ).

More than this the check

if (!userLoginId.equals(loggedInUserLogin.getString(”userLoginId”)))

is not secure either. Take a UNIX system, and change your unprivileged user account: it will ask for the old password, of course:

euronymouss-macbook-pro:opentaps_trunk euronymous$ passwd euronymous
Changing password for euronymous.
Old password:
New password:
Retype new password:

I’ve just changed the code in a way that IF AND ONLY IF the user is actually calling the service has PARTY ADMIN privileges
( so basically the superuser that would change the password if an employee is asking it – no social engineering I hope -),
he can change the password for a third party or for itself without knowing the current password.

I’ve also removed the check for password.lowercase: it should be finally removed from security.properties, even if by default is
set to false. It’s a bad security practice, because it drastically reduces password entropy (and it – badly – remembers me Microsoft LM).

Finally, I think that even in this circumstances, if a user is the admin and want to change the password of a third party,
it should actually put his current password to confirm that the request is not a BLIND XSRF ( possible otherwise).

For any interested in the source code modifies, just check svn://svn.opentaps.org/opentaps/versions/1.0/trunk at revision 12522 (look for euronymous666 changes).

Any more informations here and here (thanks to Si Chen).