hyperledger / anoncreds-clsignatures-rs

Apache License 2.0
7 stars 12 forks source link

Inconsistency of OpenSSL secure heap usage for BN #26

Closed jovfer closed 1 year ago

jovfer commented 1 year ago

https://github.com/hyperledger/anoncreds-clsignatures-rs/pull/25#issuecomment-1785873783 by @andrewwhitehead says:

This [BigNumContext::new_secure()] defines the openSSL context used in all the BigNum operations, and using new_secure means that it is zeroed out when freed. If there's a need to be compatible with older versions then that could be added as a feature flag, but I can't say that I see the benefit.

but the private keys are stored in regular BNs, not in a context. They may be in a context for some period but would be overwritten almost immediately by other operation artifacts. Based on that, it seems more important to me to use BN_secure_new calls. Here is an example: private key https://github.com/hyperledger/anoncreds-clsignatures-rs/blob/main/src/types.rs#L348 creation of the key https://github.com/hyperledger/anoncreds-clsignatures-rs/blob/main/src/issuer.rs#L869 creation of the p https://github.com/hyperledger/anoncreds-clsignatures-rs/blob/main/src/issuer.rs#L843 allocation of the memory https://github.com/hyperledger/anoncreds-clsignatures-rs/blob/main/src/bn/openssl.rs#L485

Should we update BN::new() usage to BN::new_secure() for private keys?

andrewwhitehead commented 1 year ago

Yes, it does appear that we should be using new_secure for creating BigNumbers. I had thought that each was allocated within a provided context but that doesn't seem to be the case.