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

feat: add dynamic buffer capabilities #3472

Merged
merged 3 commits into from
Sep 1, 2022
Merged

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Aug 29, 2022

Description of changes:

When connections read and write from a socket, they allocate IO buffers to do so. After allocation, these buffers are retained until the connection itself is freed or the application explicitly calls s2n_connection_release_buffers. For environments that are memory constrained, these options are somewhat limited.

This changes adds a configuration option to the connection to free IO buffers after they are used via the s2n_connection_set_dynamic_buffers function. This option can drastically reduce the amount of required memory for a connection, especially if it's not actively being used.

When paired with an allocator that has a thread-local cache or lock-free allocation, the overhead of allocating and freeing buffers with each call is practically eliminated. The following graphs show CPU efficiency with current main (no dynamic buffers), openssl, mimalloc (with dynamic buffers), and system (with glibc allocator and dynamic buffers).

bytes/cpu

Call-outs:

Because we are freeing memory more often, it also means we are zeroizing memory more often, which can decrease throughput a bit:

Before

image

After

image

To overcome this issue, I've added support for non-zeroizing frees for stuffers and blobs. The reasoning here is that:

  • for out buffers, the bytes have already been encrypted and can be considered as "public" data, so zeroizing provides little benefit
  • for in buffers we use the s2n_stuffer_erase_and_read call to copy out bytes, which already zeroizes the data:
    POSIX_CHECKED_MEMCPY(out->data, ptr, out->size);
    POSIX_CHECKED_MEMSET(ptr, 0, out->size);

I plan on updating the USAGE_GUIDE with instructions on optimizing s2n-tls and discussing some of the tradeoffs of each option.

Testing:

I've included a test that sends and receives data and asserts that the buffers are empty after they're completely flushed.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Aug 29, 2022
@camshaft camshaft marked this pull request as ready for review August 29, 2022 21:59
@camshaft camshaft requested a review from a team as a code owner August 29, 2022 21:59
Comment on lines 28 to 30
int s2n_free(struct s2n_blob *b);
int s2n_free_without_wipe(struct s2n_blob *b);
int s2n_blob_zeroize_free(struct s2n_blob *b);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that there's now three of these free methods. Why do we need s2n_blob_zeroize_free if s2n_free zeroes? We should probably create an issue to get rid of s2n_blob_zeroize_free.

Copy link
Contributor Author

@camshaft camshaft Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Looks like it's only referenced in a handful of places anyway:

$ 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));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it looks like there is a difference in functions.

s2n_blob_zeroize_free always zeroes the memory.

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);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no, that is an annoyingly subtle distinction xD So same behavior on success, but one ignores blobs that can't be freed and the other errors. I added half those references, and I certainly didn't do it because I was relying on that difference. I did it because the memory being freed was sensitive and definitely needed to be zeroed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camshaft camshaft merged commit 6cf41a5 into aws:main Sep 1, 2022
@camshaft camshaft deleted the dynamic-buffers branch September 1, 2022 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants