Non-CP1252 characters in passwords are insecure

[Ross Younger]

I was reviewing the crypto code in keepassx for fun and discovered a flaw in the derivation of an encryption key from a password.

Summary:
Any characters in your password that are not found in codepage 1252 are silently squashed to '?' before the software generates the encryption key it uses to encrypt your data.

Affected versions:
I was looking at the source to keepassx 0.4.3-1ubuntu2 (as found in precise).
Judging by the comments in Kdb3Database.cpp I think that all versions since 0.3.1 are affected, and possibly earlier.
The function responsible is Kdb3Database::setPasswordKey().

To demonstrate this issue:
1. Create a password database in keepassx.
2. Protect this database with a password made up of a character from a non-Latin script. (You can do this by copying and pasting from the Character Map applet. You can probably type it if your input methods are set up suitably.) Note that you can press the eyeball button to see the character as you paste and confirm that the text entry box does appear to be handling the characters correctly.
3. Save the database and close it.
4. Re-open the database. Instead of the password, type a question mark.
5. Notice that the database opens successfully. (If you use any other Latin character, or get the length wrong, it doesn't.)
(You can do this with any length of password, provided you get the number of question marks right.)

Who is affected:
Anybody who uses keepassx password protection where their password is entirely made up of non-Latin characters.

Impact:
If your keepassx database is protected by a password made up of non-CP1252 characters, an attacker who can steal your database would be able to quickly break the password by a brute force attack.

Possible remedies:
- Patch Kdb3Database::setPasswordKey() to reject or warn about non-CP1252 characters.
- Change your database password to one comprised entirely of characters found in CP-1252.
- Use key file protection as well as (or instead of) password protection, but bear in mind that an insecure password doesn't add much security.
- Migrate to a different password manager.

Further notes:
- Take care to not confuse keepass (keepass.info) with keepassx (keepassx.org). They are different codebases but their databases are compatible, up to a point.
- I note that keepassx 0.4.x is no longer maintained by upstream.
- I have not tried the keepassx 2.0 alpha.
- I have tried keepass2 2.18+dfsg-2 (from precise) and found that the above demonstration of this issue does not work, though I have not looked at its code.
- I suspect the same issue applies to the original v1 keepass software (keepass.info) but have not tried it as it is Windows only.

[Seth Arnold (seth-arnold)]

Ross, this is very interesting, nice work.

Because this is an intentional feature of the program, I'm choosing to not ask for a CVE number, and I'm also just opening the bug report for public view. This is likely a feature designed to ease inter-operation with the Windows program of similar name, and "fixing" this issue would likely break the easy movement of encrypted password stores.

At least once the trade off is publicly visible, users can choose to continue using keepassx or not as they wish, or modify how they use it, with knowledge of its limitations.

I'm curious if you can speak to the key derivation function used? Their website is remarkably information-free on the important parts of password storage and the corresponding keepass.info Windows-program has the rather terrifying "SHA-256 is used as password hash. SHA-256 is a 256-bit cryptographically secure one-way hash function. Your master password is hashed using this algorithm and its output is used as key for the encryption algorithms."

Thanks

[Felix Geyer (debfx)]

The key derivation function works like this:
The password is hashed with sha256, encrypted x-times with a random key, then the result is concatenated with 16 random bytes and hashed again.

finalKey = sha256(seed || key(sha256(password), iterations, transformSeed))

key(password, 1) = aes256cbc(password, transformSeed, [0, ..., 0])
key(password, iterations) = aes256cbc(key(password, iterations - 1), transformSeed, [0, ..., 0])

with aes256cbc(data, key, iv)
iterations is 50000 by default.

[Felix Geyer (debfx)]

The only mitigation that I think is viable would be to display a warning when non-CP1252 password chars are used.

[dtaylor84 (davidt-launchpad)]

I am not sure why a program being intentionally insecure makes the vulnerability any less secure?

This silently removes _all_ security from any user who uses a password comprised solely of non CP1252 characters, from a product designed to improve security. How could this make anyone more vulnerable?

[dtaylor84 (davidt-launchpad)]

sorry s/less secure/less serious/

[Ross Younger (crazyscot)]

Seth, I'll leave it to your judgement as part of the Ubuntu Security Team on whether and how to escalate this. Personally I would err on the side of removing the decision from the user as we've seen, time and again, that ordinary users just do not have the ability to make rational judgements over questions of security.

I wasn't sure where else I should report it, considering that 0.4.x is unmaintained upstream. Having thought about it some more I suspect the Debian security team ought to be alerted? I see that the same developers are involved with both Ubuntu and Debian packaging of keepassx.

Key derivation looks like this:

RawMasterKey := SHA256( ConvertToCP1252(password) )
Generate nonces N1,N2.
MasterKey := SHA256( AES-ECB(AES-ECB( ... /* 50000 times */ AES-ECB(data=RawMasterKey, key=N1) ... ,N1),N1) )
FinalKey := SHA256(N2, MasterKey)

The database payload is then AES-CBC or Twofish-CBC encrypted using FinalKey with a random IV. The IV and all nonces are generated using Yarrow, seeded from /dev/urandom, and stored in the database header.

If a key file (32 bytes binary or 64 hex digits) is used as well as a password,
RawMasterKey := SHA256(RawMasterKey from password || KeyFile).

There is a little further wrangling for backwards compatibility with earlier database versions that used Latin-1 or UTF-8 encoding for passwords; the file opening logic may also try with Latin-1 or UTF-8 conversions of the password entered.

I am only an armchair cryptographer so can't really speak for the security of this key derivation scheme, but I didn't spot any obvious issues with it. It's not the same as PBKDF2, though may be comparable. The nonces - different on every save - seem to reduce the scope for attacks based on obtaining multiple copies of the same database protected by the same password but I haven't thought about them terribly deeply.

[Ross Younger (crazyscot)]

Rather than simply displaying a warning if non-CP1252 characters were entered, I think it would be better if keepassx refused to allow non-CP1252 characters to be used when setting a new password. There should perhaps be a warning when entering them on opening a database, to change your password ASAP?

[Seth Arnold (seth-arnold)]

Ross, Felix, David, thanks for the feedback.

At least the key derivation function isn't as bad as I feared. It might not be standardized but it isn't obviously bad.

An update to warn about a password that contains non-cp1252 characters feels appropriate to me. (Refusing to use non-cp1252 characters less so, but I don't feel strongly about this.)

If one of you does prepare a patch to address this, please do coordinate with the Debian maintainer -- if Debian is the closest there is to upstream, it'd be best to get the patch as high as possible.

Thanks

[Ross Younger (crazyscot)]

Florian Weimer of the Debian security team writes:
> I think the proper fix would be to encode the password in UTF-8 for
> new encryptions, and try both the old cp1252 method and the new one on
> decryption.
>
> I would add this information to the Launchpad bug, but for some
> reason, I get error message.
>
> In any case, until upstream has implemented something in this
> direction, I don't think it's worth pushing a security update.