-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Keepass2john: Support newer format XML keyfile #5922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Keepass2john: Support newer format XML keyfile #5922
Conversation
solardiz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know anything about the "newer format", but I made some comments on the code assuming it can end up running on arbitrary/wrong input.
67aaeec to
070fc3a
Compare
|
Samples added to john-samples. The new (well it was many many years ago) format for an XML keyfile is like this: The 50BD7B21 is supposedly a CRC32 of the following 32 bytes of key data, not required and not used nor needed in any way for opening (or cracking) a database. If that attribute exists, KeePass would warn if it doesn't match. We don't currently even validate it (I fail to arrive at the CRC above, and I didn't care to figure out why - maybe endianess). The old format (which we already supported) was more like this (IIRC) for the same 32 byte key: I think there might be whitespace between We don't have such sample (the samples we had are "any file used as a key file", not XML). I may try to create one for john-samples later just for completeness. |
2a7da6e to
49e412d
Compare
|
Sorry I jumped the gun with requesting a review, I think it's good to go now. Tested with v2 and mockup v1. |
Turns out KeePass 2.x and KeePassXC calculate this CRC differently, making it pretty much useless. I will not add any check, it's just overkill. |
solardiz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment on one code issue. Also, maybe remove unsigned from definition of buffer instead of all the casts?
49e412d to
fb0ea7e
Compare
| data = p; | ||
| p = strstr(p, "</Data>"); | ||
| printf ("%s", base64_convert_cp(data, e_b64_mime, p - data, b64_decoded, e_b64_hex, sizeof(b64_decoded), flg_Base64_NO_FLAGS, 0)); | ||
| if (p == NULL || p - buffer < 44) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be != 44 rather than < 44 here. That would be consistent with the check for hex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually no, because there may be whitespace (of any sort) between the Base64 and </Data>.
Admittedly we don't know they are 44 actual Base64 characters anyway. I'll see if base64_convert_cp returns something useful. BTW I don't recognize that _cp suffix, need to check what the heck that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK so I can get an output size from base64_convert... but it returns 64 no matter what garbage I feed it. There are also means for an error variable but it always returns "no error". I'm ignoring this for now.
The existing support was keyfile v1 only (meaning xml keyfile v1 of KeePass v2). This adds support for xml keyfile v2, tested with a v4 database. Also adds some robustness.
fb0ea7e to
44192b5
Compare
Despite comments in code, the existing support appeared to be v1 only. This new clause is tested with a v4 database and keyfile.