LoupVaillant / Monocypher

An easy to use, easy to deploy crypto library
https://monocypher.org
Other
614 stars 80 forks source link

Return plaintext even if Poly1305 fails #210

Closed HACKERALERT closed 3 years ago

HACKERALERT commented 3 years ago

Hey LoupVaillant. Is there a way to return the plaintext, regardless of whether Poly1305 passes or not (crypto_unlock)? In my file encryption tool, I have an option that lets the user keep a corrupted or modified file after decryption, but it seems Monocypher returns an empty array of 0's if the Poly1305 is not correct. I understand that this is to prevent any security issues, but it would be nice if there was a way to keep the output, because sometimes, my tool is used by people to encrypt valuable photos, and if a byte in the Poly1305 corrupts, then there's no way that Monocypher lets me retrieve the original data.

LoupVaillant commented 3 years ago

Hi, As you have guessed, I haven't implemented in the high level interfaces because it would run awful of the Cryptographic Doom Principle. You would change that over my dead body. :dagger:

By the way, crypto_unlock() doesn't even fill the output array if Poly1305 fails. If it fails the function has no effect at all. The zeroes you see have been written before you call crypto_unlock() (static arrays in C, or maybe some initialization from C#).

For your use case (valuable data), you should consider error correction codes. First you compress the data (optional, and depends on the data). Then you encrypt it (with authenticated encryption). Then you add error correction codes on top. If you have some random corruption, ECC will fix it, and Poly1305 will work. Perhaps there's an ECC library you could use?

That said, what you ask for is possible. Naughty, but possible.

Such heresy requires you to use the low level interfaces (Chacha20 and Poly1305). Basically, you want a modified version of crypto_unlock(), that doesn't return early, but instead sets the error code to -1 instead of 0. Implementing it in C is easy (just copy & paste crypto_unlock() and its private dependencies, make the change, and make sure it decrypts images all right. Maybe modify crypto_unlock() itself, and run the test suite, see that you didn't break anything (you may break a couple tests that check that outputs are unmodified upon failure). If you're using language bindings, you can also bind the low-level Chacha20 and Poly1305 functions, then re-implement your naughty version of crypto_unlock() in your favourite language.

Hope that helps, Loup.

HACKERALERT commented 3 years ago

Thanks for your detailed explanation. I love how you use the word naughty, I can't think of a better word ;). Thanks for letting me know. I give users the option to encode the output with Reed-Solomon, which prevents most errors, but you know, someone might drop their hard drive (🤦) and try to recover as much as possible. Looking at crypto_unlock_aead here: https://github.com/LoupVaillant/Monocypher/blob/master/src/monocypher.c#L3009, it seems pretty trivial to just remove the return -1; on line 3009, set a boolean on that line, and return -1 at the end if that boolean is true. If I'm not mistaken, then the crypto_chacha20_ctr will run and the plaintext will be populated, and there shouldn't be any security issues apart from potentially some failing tests, as you mentioned. As you're the creator of this library, can you see any potential pitfalls with this? I check for the return code whenever crypto_unlock is used, so everything should be fine. Edit: I also warn the user that the file is corrupted/modified and tell them to use the output at their own risk.

LoupVaillant commented 3 years ago

it seems pretty trivial to just remove the return -1; on line 3009, set a boolean on that line, and return -1 at the end if that boolean is true.

Yup, that's it. Just don't forget not to wipe sub_key right away, you will need it for the decryption.

As you're the creator of this library, can you see any potential pitfalls with this?

Not many to be honest. Cryptographic Doom notwithstanding, nothing bad can happen at the library level: even with corrupted data under attacker control, we'll just get a corrupted buffer. Can't have like a buffer overflow if the rest of the program is written correctly.

Also, to ease review, do make sure to clearly mark the modification as a departure from the original library. Or better yet, write your own modified version, which can then separate from Monocypher (easier updates, though you should never need them).

Last: we still have a Cryptographic Doom problem at the user level, so do make sure to warn your users about corrupted files:

HACKERALERT commented 3 years ago

Nice, thank you for letting me know. I won't make any changes to the Go bindings that you've linked to on Monocypher's homepage, and I'll just create a separate repo with a notice that it is for a specific use case and point users to the unmodified Go bindings for general use. I'll definitely warn users about corruption and intentional modification, although I think most of then are smart enough not to encrypt an executable and then run it. Thanks again for helping me with this issue, I'll close it as it is completed.