UBSAN triggers on is_available getting passed an incorrect function type

[justhecuke]

Problem:

UBSAN is triggering on misuse of is_available with s2n_stream_cipher_rc4_available from

s2n-tls/crypto/s2n_stream_cipher_rc4.c

Line 33 in a56e9bf

static uint8_t s2n_stream_cipher_rc4_available()

.

s2n_cipher_suites.c:985:17: runtime error: call to function s2n_stream_cipher_rc4_available through pointer to incorrect function type 'unsigned char (*)(void)'

In this case, we're passing a uint8_t s2n_stream_cipher_rc4_available() to is_available which wants a uint8_t (*is_available)(void)

To Reproduce:
I don't have an explicit copy-pasteable example, but it should be easy to reproduce if you have UBSAN set up. Simply create a program that calls s2n_rc4.is_available() and compile (-fsanitize=undefined -fno-sanitize-recover=undefined) + run it with UBSAN . It should show you the issue.

Solution:

Either fix the signature of s2n_stream_cipher_rc4_available to return a pointer, or fix #3983

• Does this change what S2N sends over the wire? If yes, explain.

Probably not, but I lack the knowledge to say for sure.

• Does this change any public APIs? If yes, explain.

I think so, but I'm not certain.

• Which versions of TLS will this impact?
  N/A

Requirements / Acceptance Criteria:

Get the signatures of s2n_stream_cipher_rc4_available and is_available to match so that UBSAN stops complaining.

• RFC links: Links to relevant RFC(s)
• Related Issues: Link any relevant issues

#3983

• 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.

Get UBSAN running in CI/testing. This is a pretty trivial issue for it to detect and it makes me think there's other issues lurking around in the code.

• 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.

Unknown

• Should this change be fuzz tested? Will it handle untrusted input? Create a separate issue to track the fuzzing work.
  No. N/A

Out of scope:

Is there anything the solution will intentionally NOT address?

[justhecuke]

Found more

hash_table.c:68:12: runtime error: call to function aws_byte_cursor_eq through pointer to incorrect function type 'bool (*)(const void *, const void *)'
ref_count.c:29:9: runtime error: call to function s_s2n_ctx_destroy through pointer to incorrect function type 'void (*)(void *)'

[maddeleine]

Thanks for this issue, we'll take a look.

[jmayclin]

Hi @justhecuke! What platform did you use to produce these warnings?

I have ubsan running on Clang 15 with ARM, seem to get a different set of warning than the ones you encountered.

[justhecuke]

Hey, I'm not sure how to answer "what platform", but it's Clang 17 in Bazel. We have a bit of a frankenstein system and I'm building this as part of our own build, which is where I find the sanitizer issues.

The code I'm working with is from a1.11.185 aws-sdk-cpp archive and it's a bit confusing how to translate that aws-sdk-cpp tag into a tag/commit over here.

EDIT: I think it's 4654fecb05cd5aacbda262654eb95a3876183698 here which is v1.3.54?

[justhecuke]

In any case, even if I'm wrong on the specifics, it's somewhat clear that UBSAN can help in CI or some manual runs every now and then.

[jmayclin]

#3983 is now resolved, and with it this issue has been fixed.

I confirmed that I was able to reproduce the UBSAN failure with clang-18 using an old s2n-tls commit, and confirmed that it no longer reproduces on mainline s2n-tls.

I also opened #4684 to track adding UBSAN to our CI.