UBSAN coverage

[jmayclin]

Problem:

we currently have UBSAN violations in our codebase, meaning that s2n-tls relies on behavior that is technically undefined. This is dangerous as it means that the behavior of s2n-tls is behavior specific.

The following unit tests either contain undefined behavior themselves or hit undefined behavior in the main s2n-tls codebase.

The following tests FAILED:
        108 - s2n_hash_all_algs_test (Failed)
        122 - s2n_ktls_io_test (Failed)
        125 - s2n_ktls_test_utils_test (Failed)
        194 - s2n_self_talk_session_resumption_test (Failed)
        230 - s2n_stuffer_hex_test (Failed)
        232 - s2n_stuffer_test (Failed)
        234 - s2n_testlib_test (Failed)
        266 - s2n_x509_validator_test (Failed)

Null Arithmetic

One category of UBSAN is adding 0 to a null pointer. We expect the result of this to always be null, but it is technically undefined. An example error message from UBSAN is included below.

Running /home/ec2-user/workspace/s2n-tls/tests/unit/s2n_x509_validator_test.c ... /home/ec2-user/workspace/s2n-tls/stuffer/s2n_stuffer.c:330:36: runtime error: applying zero offset to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/ec2-user/workspace/s2n-tls/stuffer/s2n_stuffer.c:330:36 in 

memcmp with null arguments

Our other category of undefined behavior is passing in a null argument to memcmp and then specifying a length of 0. THis This is undefined behavior

Running /home/ec2-user/workspace/s2n-tls/tests/unit/s2n_testlib_test.c ... /home/ec2-user/workspace/s2n-tls/tests/unit/s2n_testlib_test.c:67:13: runtime error: null pointer passed as argument 1, which is declared to never be null
/usr/include/string.h:65:33: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/ec2-user/workspace/s2n-tls/tests/unit/s2n_testlib_test.c:67:13 in

Requirements / Acceptance Criteria:

R1: Fix all existing UBSAN errors

Firstly, we need to fix the UBSAN errors.

UBSAN support was already added to the codebase in #4440. It can be run with

# build build
cmake . -Bbuild \
      -DCMAKE_C_COMPILER=clang \
      -DCMAKE_BUILD_TYPE=RelWithDebInfo \
      -DUBSAN=ON

# build
cmake --build ./build -j $(nproc)

# test
CTEST_PARALLEL_LEVEL=$(nproc) make -C build test ARGS="--output-on-failure"

R2: Add UBSAN job to CI

We need to run UBSAN on our unit tests in CI to ensure that we don't have a regression in the future.

• RFC links: Links to relevant RFC(s)
• Related Issues: Link any relevant issues
• Will the Usage Guide or other documentation need to be updated?
• Testing: How will this change be tested? Call out new integration tests, functional tests, or particularly interesting/important unit tests.
  • Will this change trigger SAW changes? Changes to the state machine, the s2n_handshake_io code that controls state transitions, the DRBG, or the corking/uncorking logic could trigger SAW failures.
  • Should this change be fuzz tested? Will it handle untrusted input? Create a separate issue to track the fuzzing work.

Out of scope:

Is there anything the solution will intentionally NOT address?

[boquan-fang]

Those two merged PRs meet requirement 1 and 2 of this issue. This issue is fully resolved and closed.