Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: Address ZeroizeOnDrop suggestion #244

Closed
moCello opened this issue Sep 18, 2024 · 0 comments · Fixed by #245
Closed

core: Address ZeroizeOnDrop suggestion #244

moCello opened this issue Sep 18, 2024 · 0 comments · Fixed by #245

Comments

@moCello
Copy link
Member

moCello commented Sep 18, 2024

Summary

From the phoenix-audit:

It is instead advised to derive ZeroizeOnDrop where Zeroize is derived, as this will make the zeroing out of memory happen automatically as the struct goes out of scope, and takes the mental burden of ensuring this is done correctly everywhere off the programmer.

Even though we agree that it would be good and probably safer as well to take the mental burden of manually zeroizing the secret-keys off the programmer, we have found that this approach does not ensure that memory is reliably erased.

Detailed Description

To test this one needs to implement the ZeroizeOnDrop derive on SecretKey and run the following test:

#[test]
fn zeroize_on_drop() {
    let sk = SecretKey::new(JubJubScalar::from(1u64), JubJubScalar::from(2u64));

    let ptr_a: *const JubJubScalar = sk.a();
    let ptr_b: *const JubJubScalar = sk.b();

    // dropping the value should zeroize the underlying jubjub-scalar
    drop(sk);

    // but the memory is not erased
    unsafe {
        assert_eq!(
            core::slice::from_raw_parts(ptr_a, 1)[0],
            JubJubScalar::from(1u64)
        );
        assert_eq!(
            core::slice::from_raw_parts(ptr_b, 1)[0],
            JubJubScalar::from(2u64)
        );
    };

    // let's try again and call zeroize explicitly before dropping:
    let mut sk =
        SecretKey::new(JubJubScalar::from(1u64), JubJubScalar::from(2u64));
    let ptr_a: *const JubJubScalar = sk.a();
    let ptr_b: *const JubJubScalar = sk.b();

    sk.zeroize();
    drop(sk);

    // now the memory is zeroed
    unsafe {
        assert_eq!(
            core::slice::from_raw_parts(ptr_a, 1)[0],
            JubJubScalar::from(0u64)
        );
        assert_eq!(
            core::slice::from_raw_parts(ptr_b, 1)[0],
            JubJubScalar::from(0u64)
        );
    };
}

If the memory were to be erased on drop the test would not pass.

Relevant Context

We have already explored this route as part of the audit of our bls-signature scheme. See dusk-network/bls12_381-bls#6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant