6 ms·
The first thing I do whenever someone writes their own password manager is to read the Encrypt function. This one is AES-CBC with its own hand rolled integrity
by ejcx 7y ago
The first thing I do whenever someone writes their own password manager is to read the Encrypt function. This one is AES-CBC with its own hand rolled integrity scheme. Not very strong by modern standards
- beefhash 7y agoDoesn't look very hand rolled to me. It's standard HMAC. The only unusual thing is the timing-unsafe comparison[1], which probably needs fixing. It looks like an attempt was made at a constant-time comparison (|= ^ pattern sure looks like it), but the early return breaks it again. I'm not sure if much can be gained from a timing attack in this particular instance though, since the key fully depends on user input in the first place. (By the way, even Microsoft's own documentation doesn't use constant-time comparisons for HMAC[2]!) [1] https://github.com/nrosvall/ylva/blob/2a4afcfb3727151fa09fdd7fa4bd58954b457dd6/Ylva/EncryptionDataIntegrity.cs#L38-L44 https://github.com/nrosvall/ylva/blob/2a4afcfb3727151fa09fdd... [2] See the example on https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.hmacsha256?view=netframework-4.8 https://docs.microsoft.com/en-us/dotnet/api/system.security....
- ejcx 7y agoI'm not a C# expert by any means. Is the IntegrityHash of the plaintext, and not the ciphertext? https://github.com/nrosvall/ylva/blob/2a4afcfb3727151fa09fdd7fa4bd58954b457dd6/Ylva/EncryptionAES.cs#L78 https://github.com/nrosvall/ylva/blob/2a4afcfb3727151fa09fdd... That would be a really serious flaw. If not, hand rolled AES-CBC-SHA256.... why not just use an AEAD implementation? This is exactly why I look at these. There's a lot of nuance to that one decision, and so it usually gives quite a bit of signal about the project as a whole.
- d76d6776yudsy 7y agoPretty thick are ya? Parent is telling you it doesn't look hand rolled and you...