feat(security): simplify decrypt error handling #51301
Conversation
Before this patch, when decrypting a value without using a password, it would call `decryptWithoutSecret` with the system `secret` as `password`. When this fails, it would retry with an empty string as `password`. This has the practical disadvantage that it can lead to confusing error messages. For example, when using the TOTP app, when the system `secret` is misconfigured, the first invocation will throw a sensible `HMAC does not match.` error, but then it is retried and the retry throws a `Hash_hkdf(): Argument nextcloud#2 ($key) cannot be empty` error causing confusion (e.g. https://help.nextcloud.com/t/hash-hkdf-argument-2-key-cannot-be-empty/192556). Of course this fallback to using an empty string is likely part of some sort of graceful migration from the days when the secret could be empty (e.g. nextcloud#34012, nextcloud#31499). However, taking a wider perspective, such 'fallback logic' in security-critical areas makes things more complex, which is a risk. It's not quite the same scenario, but Heartbleed does come to mind. For this reason, rather than a 'surgical' improvement for the particular case encountered above (increasing complexity further), I think it'd be worth to start considering removing this fallback entirely (perhaps in v32.0.0?) - hence this conversation-starter PR. Signed-off-by: Arnout Engelen <[email protected]>
raboof
requested review from
come-nc,
sorbaugh and
yemkareems
and removed request for
a team
|
Related: #46176 (I haven't had a chance to pick it back up). |
Ah, missed that, thanks for adding the link! I think both PRs still make sense: your 'surgical' improvement (that can be backported) and possibly my more 'blunt' one (for the next major version). |
|
If I recall correctly the problem is that it’s not possible for an instance to change its secret I think, so if an empty secret was used you are stuck with it, no? Or more likely you can set the secret, but it will only be used for new data. If there is an upgrade path from a 31 with an empty secret to a 32 with a secret correctly set then I’m perfectly fine with dropping the workaround. But that means making sure all data using the empty secret is migrated to the new one. The version in #46176 is way safer in my opinion. My comments in there still stand though. |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
Realistically I think properly analyzing that is more than I have time for right now, so I'll close this (even though keeping such 'fallback' code around in security-critical areas does still make me uneasy :) ) |
3 participants
Summary
Before this patch, when decrypting a value without using a password, it would call
decryptWithoutSecretwith the systemsecretaspassword. When this fails, it would retry with an empty string aspassword.This has the practical disadvantage that it can lead to confusing error messages. For example, when using the TOTP app, when the system
secretis misconfigured, the first invocation will throw a sensibleHMAC does not match.error, but then it is retried and the retry throws aHash_hkdf(): Argument #2 ($key) cannot be emptyerror causing confusion (e.g.https://help.nextcloud.com/t/hash-hkdf-argument-2-key-cannot-be-empty/192556).
Of course this fallback to using an empty string is likely part of some sort of graceful migration from the days when the secret could be empty (e.g. #34012, #31499, /cc @juliusknorr ).
However, taking a wider perspective, such 'fallback logic' in security-critical areas makes things more complex, which is a risk. It's not quite the same scenario, but Heartbleed does come to mind.
For this reason, rather than a 'surgical' improvement for the particular case encountered above (increasing complexity further), I think it'd be worth it to start considering removing this fallback entirely (perhaps in the next major version?) - hence this conversation-starter PR.
I'll keep this in draft for now because it needs a unit test, but otherwise I'd love to start the review/conversation.
TODO
Checklist