Exception safety issue

[gameraccoon]

Hi, I've been looking through the code of this repo while doing my own Noise XX and KK implementation based on monocypher and noticed an issue with usage of C++ exceptions.

I see that not all the code that exceptions can be thrown through is exception-safe. For example, some code fails to call crypto_wipe if an exception is thrown. Specifically worrying is the exceptions thrown in decrypt, as failing to decrypt doesn't necessarily mean that the communication session needs to be terminated (specifically, after the handshake phase), so this could leave some sensitive for the session values on the stack.

Biased opinion follows.
My personal opinion would be not to use exceptions at all, and use enum-based return codes instead. I feel like this would increase the adoption of the library, as some C++ programmers are allergic to exceptions.

[ethindp]

@gameraccoon Can you be more specific as to where exception safety is an actual issue? I'm looking at decrypt (both implementations) and we do wipe the context, key, and nonce before throwing exceptions.

Re: error codes over exceptions. My biased opinion is that this would just complicate things. I know that many C++ programmers are (unreasonably, IMO) allergic to exceptions, and I very much disagree with this kind of allergen, because it (from my knowledge) is and has been nonsensical for at least a decade; exceptions are zero-cost when they aren't thrown, and I know that modern compiler enhancements to Clang have been working towards completely zero-cost exceptions. It might be worth introducing std::expected or the reference implementation of it regardless, though.

I haven't really worked on this library since I initially published it purely because I haven't really seen anyone adopt it/report anything, so development has pretty much stalled.

[ethindp]

Okay, so poking around I can see what you mean. I admit to being concerned about the bugs in read_message for example. I guess we could catch, clean up and re-throw?

[gameraccoon]

I actually overreacted about decrypt, I did not notice that you call crypto_wipe in both branches. But yes, there is at least one more place in SymmetricState::decrypt_and_hash and where it called from in HandshakeState::read_message (I did not check everything, only noticed these).

As for the solutions, it's your call.

[ethindp]

I think I might have a solution for this. Could do what Botan and similar do with secure_array/vector types. Would I think solve a majority of these issues. We'd have duplicate crypto wipes in places, but those wouldn't I think necessarily pose a problem and could be cleaned up if needed.

[ethindp]

@gameraccoon It may be my call but I prefer collaborating with issue reporters instead of making unilateral decisions. Especially with libraries like these where a mistake could be way larger than I realize later down the line.

[ethindp]

Also, digging around deeper I think read_message might have a state consistency issue: if an exception is thrown mid-handshake, the handshake could get irreversibly mutated into some weird middle-state that we don't want it to be in where that part of the handshake has been partially both completed and not completed. I... Am currently uncertain how to resolve that potential issue, or if we should try to resolve it at all or require the caller to just throw away the handshake state and recreate it (since if it fails midway through then something is very, very wrong, and attempting to continue from that point onward might be a very bad idea).

[gameraccoon]

I think I might have a solution for this. Could do what Botan and similar do with secure_array/vector types. Would I think solve a majority of these issues

@ethindp This sounds like good solution for this. I ended up doing the same for my implementation as well.