Disclaimer: I’m not a security expert…

I identified a minor security flaw with CompiereService, which stores the password of the logged-in user in plain text. The password is also exposed bean-style through a public getter. The public getter does not seem to be used anywhere in the code.

There doesn’t seem to be a good reason for storing the password in plain text or for making it accessible via a bean. For security reasons, the best thing to do with a password is to use it and forget it as soon as possible. Leaving it hanging around in memory makes it easier to be found and exploited.

https://github.com/idempiere/idempiere/blob/f18d221b411a7fd9f81a6a6d0f44557707588370/org.idempiere.webservices/WEB-INF/src/org/idempiere/adinterface/CompiereService.java#L459-L466

For security reasons, I think it would be better if passwords are not kept hanging around in memory longer than is needed. If for some reason there is a security exploit available that gives an attacker access to memory locations outside of its sandbox, or if (eg) there is a crash dump and the memory contents are dumped to disk,

Proposed solution:

1. Rename the m_password to m_passwordHash.

2. After successful validation of the username/password credentials, call m_cs.setPasswordHash(DigestUtils.md5Hex(loginRequest.getPassword()));

3. Call loginRequest.setPass(null) immediately after this step.

4. There are other earlier code paths in AbstractService.login() where the password is not even used (if the session is cached) - these could also call loginRequest.setPass(null) as soon as they know that a password check isn’t going to be needed.

As a bigger issue - there are more secure ways to do client authentication than for the client to send the password in plain text. The use of https mitigates against the risk of a man-in-the-middle attack, but I think it could be better still.