Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Ensure proper memory clean-up for secret in libsecp256k1 dependency. #9870

Open
Slesarew opened this issue Sep 27, 2021 · 4 comments
Open

Ensure proper memory clean-up for secret in libsecp256k1 dependency. #9870

Slesarew opened this issue Sep 27, 2021 · 4 comments
Labels
J2-unconfirmed Issue might be valid, but it’s not yet known.

Comments

@Slesarew
Copy link

Two other upstream crypto libraries are using zeroize to clear memory used for storing secrets (see https://docs.rs/ed25519-dalek/1.0.1/src/ed25519_dalek/secret.rs.html#43 and https://docs.rs/schnorrkel/0.9.1/src/schnorrkel/keys.rs.html#680), but libsecp256k1 apparently does not. This will normally not be a severe issue in substrate as this algorithm is not used much, but it might cause all secrets to memory leak in the Signer, which depends on sp-core and uses all 3 crypto algorithms to maximize future compatibility.

As discussed with @kirushik and @burdges in PM, so tagging them here as well.

@Slesarew
Copy link
Author

Slesarew commented Oct 5, 2021

@kirushik we might want to wrap this crypto object in zeroize here depending on whether it would be wrapped upstream; not as good though, I understand.

We could forbid use of that lib in Signer temporarily as temporary workaround; not sure if it is worth it though - we'll lose this crypto type then obviously.

@kirushik
Copy link

kirushik commented Oct 5, 2021

@Slesarew since doing a better job at maintaining our libsecp fork is something we'd want to do anyway, looping Thomas in here — the linked paritytech/libsecp256k1#93 issue might be a good starting point for the new libsecp co-maintainer.

Please just use the library "as is" in Signer — and make a note to check that this issue is fixed before moving Signer anywhere near a public beta.

@Slesarew
Copy link
Author

Slesarew commented Oct 5, 2021

and make a note to check that this issue is fixed before moving Signer anywhere near a public beta.

Oh ok, so I'm moving this deadline from private to public beta, that's a relief

@bkchr
Copy link
Member

bkchr commented Oct 5, 2021

since doing a better job at maintaining our libsecp fork is something we'd want to do anyway

FWIW: #8089 we will switch away from our own fork in the future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
J2-unconfirmed Issue might be valid, but it’s not yet known.
Projects
None yet
Development

No branches or pull requests

4 participants