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

refactor: zero static s2n_configs on cleanup #4416

Merged
merged 5 commits into from
Feb 15, 2024

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Feb 14, 2024

Resolved issues:

Test failures in #4407

Description of changes:

4407 introduces a change which causes internal validation to be triggered on the config whenever the security policy is set. This caused two different test failures

s2n_init

s2n-init was failing in the following sequence of events

  1. s2n_init is called
  2. s2n_config_defaults_init is called
  3. s2n_config_init is called for all default configs in static memory
  4. s2n_config_init sets the cipher preference before allocating the map
  5. This results in map validation failing with S2N_ERR_NULL because the map was null.

My knee jerk reaction was to have s2n_config_validate_loaded_certificates return S2N_RESULT_OK when the map was null. (note that I don't think the is the long term correct course) However this resulted in an odder series of failures

  1. s2n_init is called
  2. s2n_config_defaults_init is called
  3. s2n_config_init is called for all default configs in static memory
  4. s2n_config_init sets the cipher preference
  5. s2n_config_validate_loaded_certs sees the the map is NULL is returns OK
  6. map is allocated
  7. s2n_cleanup is called
  8. s2n_wipe_static_configs is called
  9. s2n_config_cleanup is called, where the objects in the static configs are freed, but the configs are not zeros.
  10. s2n_init is called
  11. ...
  12. s2n_config_validate_loaded_certs sees that the map is not NULL, but fails because the map is not immutable

The immutability error is happening because the map was previously zero'd, so the boolean field of the struct is interpretated as false, but in fact this is a use-after-free. The map pointer is dangling. This was confirmed with ASAN.

==2326782==ERROR: AddressSanitizer: heap-use-after-free on address 0xffff90a2f958 at pc 0x0000005a5048 bp 0xfffff8ffb800 sp 0xfffff8ffb7f8
READ of size 4 at 0xffff90a2f958 thread T0
    #0 0x5a5044 in s2n_map_iterator_init /home/ec2-user/workspace/s2n-tls/utils/s2n_map.c:284:5
    #1 0x509588 in s2n_config_validate_loaded_certificates /home/ec2-user/workspace/s2n-tls/tls/s2n_config.c:590:5
    #2 0x5679cc in s2n_config_set_cipher_preferences /home/ec2-user/workspace/s2n-tls/tls/s2n_security_policies.c:1213:5
    #3 0x505b08 in s2n_config_setup_default /home/ec2-user/workspace/s2n-tls/tls/s2n_config.c:72:5
    #4 0x505b08 in s2n_config_init /home/ec2-user/workspace/s2n-tls/tls/s2n_config.c:107:5
    #5 0x50597c in s2n_config_defaults_init /home/ec2-user/workspace/s2n-tls/tls/s2n_config.c:240:9
    #6 0x4f1fdc in s2n_init /home/ec2-user/workspace/s2n-tls/utils/s2n_init.c:77:5
    #7 0x4eac84 in main /home/ec2-user/workspace/s2n-tls/tests/unit/s2n_init_test.c:58:9
    #8 0xffff93fe3a74 in __libc_start_call_main /usr/src/debug/glibc-2.34-52.amzn2023.0.7.aarch64/csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #9 0xffff93fe3b58 in __libc_start_main@GLIBC_2.17 /usr/src/debug/glibc-2.34-52.amzn2023.0.7.aarch64/csu/../csu/libc-start.c:389:3
    #10 0x4312ec in _start (/home/ec2-user/workspace/s2n-tls/build/bin/s2n_init_test+0x4312ec) (BuildId: cc99f4e4b285b425b59ad7c53490bfa523f9918f)

To avoid these kinds of errors we should also wipe the configs when we call cleanup.

s2n_cipher_suite_match_test.c

This test experienced failures because there were cases where we'd

  1. call s2n_connection_wipe, which preserved the reference to the associated config
  2. cal s2n_config_free on the config that was previously associated with the connection

After this, the conn->config pointer is a dangling pointer towards garbage, free'd memory. This was causing a crash in

    RESULT_ENSURE_REF(cert_key_pair);
    RESULT_ENSURE_REF(cert_key_pair->cert_chain); # this line crashed

The first RESULT_ENSURE_REF didn't trigger because the pointer wasn't null, it was dangling. Once again this was confirmed with ASAN.

This can be fixed by associating valid configs with the connections before using them in tests.

Call-outs:

For non static configs, the configs are zero'd when they are freed. This means that there is a "double zeroing" happening for configs, but I thought that this implementation (adding a POSIX_CHECKED_MEMSET) was the most readable way of solving this.

Also, the zeroing of static configs on wiping isn't technically necessary as long as you initialize all of the config fields in the correct order. However this creates a very sharp edge and could lead to nondeterministic failures and crashes, so I'd prefer to zero them.

Testing:

All tests/CI runs should pass.

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 Feb 14, 2024
@jmayclin jmayclin marked this pull request as ready for review February 14, 2024 01:08
@lrstewart
Copy link
Contributor

Yeah, this should be fine. Our documentation says not to free configs that are in use, so that's already undefined behavior.

@jmayclin jmayclin enabled auto-merge (squash) February 15, 2024 00:24
@jmayclin jmayclin merged commit 629c60a into aws:main Feb 15, 2024
31 checks passed
@jmayclin jmayclin deleted the zero-configs-on-cleanup branch June 15, 2024 00:17
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.

3 participants