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

Investigate removing s2n_blob_zeroize_free #3475

Open
camshaft opened this issue Sep 1, 2022 · 0 comments
Open

Investigate removing s2n_blob_zeroize_free #3475

camshaft opened this issue Sep 1, 2022 · 0 comments
Labels

Comments

@camshaft
Copy link
Contributor

camshaft commented Sep 1, 2022

Problem:

We currently have both s2n_blob_free and s2n_blob_zeroize_free which are very subtly different.

s2n_blob_zeroize_free always zeroes the memory, even if it's not an allocated blob.

s2n-tls/utils/s2n_mem.c

Lines 300 to 303 in 1209208

POSIX_GUARD(s2n_blob_zero(b));
if (b->allocated) {
POSIX_GUARD(s2n_free(b));
}

s2n_blob_free will fail if you try and free a non-growable blob:

POSIX_ENSURE(s2n_blob_is_growable(b), S2N_ERR_FREE_STATIC_BLOB);

however it still zeroes the blob:

int zero_rc = s2n_blob_zero(b);

Solution:

It would be ideal if we only had one of these. The s2n_blob_zeroize_free is only referenced in a handful of places:

$ rg s2n_blob_zeroize_free bin crypto error pq-crypto stuffer tls
tls/s2n_tls13_secrets.c
347:    DEFER_CLEANUP(struct s2n_blob shared_secret = { 0 }, s2n_blob_zeroize_free);

tls/s2n_client_key_exchange.c
229:    DEFER_CLEANUP(struct s2n_blob shared_key = { 0 }, s2n_blob_zeroize_free);
328:    DEFER_CLEANUP(struct s2n_blob shared_key = { 0 }, s2n_blob_zeroize_free);

tls/s2n_tls13_handshake.c
97:    DEFER_CLEANUP(struct s2n_blob ecdhe_shared_secret = { 0 }, s2n_blob_zeroize_free);

tls/s2n_kem.c
228:        POSIX_GUARD(s2n_blob_zeroize_free(&kem_params->private_key));
229:        POSIX_GUARD(s2n_blob_zeroize_free(&kem_params->public_key));
230:        POSIX_GUARD(s2n_blob_zeroize_free(&kem_params->shared_secret));

Assuming all of these can be replaced with s2n_blob_free, we should just get rid of s2n_blob_zeroize_free entirely.

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

No branches or pull requests

2 participants