`pkcs1v15::SigningKey` and `pkcs1v15::DecryptingKey` should implement Zeroize and Drop

[mx-shift]

Both SigningKey and DecryptingKey contain instances of RsaPrivateKey. RsaPrivateKey implements Zeroize and Drop but there is currently no way to invoke them on the inner instances contained in these types.

[tarcieri]

I think it's debatable whether RsaPrivateKey should impl Zeroize explicitly as opposed to only zeroing in the Drop handler.

The problem with having a Zeroize impl on RsaPrivateKey itself is it allows zeroing out the key then using it afterward, a sort of use-after-zeroize bug.

This is why the ZeroizeOnDrop trait exists: as a marker that RsaPrivateKey will take care of zeroizing itself so you don't have to worry about doing it explicitly.

I would suggest removing the Zeroize impl and adding a ZeroizeOnDrop impl instead.

[mx-shift]

Implementing ZeroizeOnDrop without implementing Zeroize doesn't match the docs for ZeroizeOnDrop (Marker trait signifying that this type will Zeroize::zeroize https://docs.rs/zeroize/latest/zeroize/trait.Zeroize.html#tymethod.zeroize itself on Drop https://doc.rust-lang.org/nightly/core/ops/drop/trait.Drop.html.) but I understand your point. It would prevent using Zeroizing as a signal to readers that a given type will ZeroizeOnDrop when the marker traits are cumbersome to check (stack-allocated usage).

On Tue, Apr 4, 2023, 5:36 PM Tony Arcieri @.***> wrote:

I think it's debatable whether RsaPrivateKey should impl Zeroize explicitly as opposed to only zeroing in the Drop handler.

The problem with having a Zeroize impl on RsaPrivateKey itself is it allows zeroing out the key then using it afterward, a sort of use-after-zeroize bug.

This is why the ZeroizeOnDrop trait exists: as a marker that RsaPrivateKey will take care of zeroizing itself so you don't have to worry about doing it explicitly.

I would suggest removing the Zeroize impl and adding a ZeroizeOnDrop impl instead.

— Reply to this email directly, view it on GitHub https://github.com/RustCrypto/RSA/issues/285#issuecomment-1496770696, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACIEHF3NMZFAXM42TGSTVRDW7S5BLANCNFSM6AAAAAAWTMOJFM . You are receiving this because you authored the thread.Message ID: @.***>

[tarcieri]

I'm the author of zeroize.

The intent is definitely not that every type which calls Zeroize from the Drop handler also has to implement the Zeroize trait itself.

Types need to maintain invariants. In the case of a cryptographic public or secret key, in many cases one of those invariants is "the inner value is non-zero".

Drop handlers provide a place those invariants can be violated because the value is definitionally inaccessible.

[mx-shift]

Sure, I understand. That's not what the documentation I copied from ZeroizeOnDrop conveys.

On Wed, Apr 5, 2023, 6:33 AM Tony Arcieri @.***> wrote:

I'm the author of zeroize.

The intent is definitely not that every type which calls Zeroize from the Drop handler also has to implement the Zeroize trait itself.

Types need to maintain invariants. In the case of a cryptographic public or secret key, in many cases one of those invariants is "the inner value is non-zero".

Drop handlers provide a place those invariants can be violated because the value is definitionally inaccessible.

— Reply to this email directly, view it on GitHub https://github.com/RustCrypto/RSA/issues/285#issuecomment-1497495188, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACIEHF3XDRWQ5YEQMZQNBBLW7VYDXANCNFSM6AAAAAAWTMOJFM . You are receiving this because you authored the thread.Message ID: @.***>