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

Allow client hellos from raw bytes #3871

Merged
merged 7 commits into from
Mar 21, 2023
Merged

Allow client hellos from raw bytes #3871

merged 7 commits into from
Mar 21, 2023

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Mar 9, 2023

Resolved issues:

resolves #3852

Description of changes:

Add a method to create an s2n_client_hello structure from bytes representing a ClientHello.

As part of this change, I refactored the existing s2n_client_hello_parse method so that it only does work that requires a connection or config, like setting conn->client_protocol_version. I moved the pure parsing to a new method called by both the existing s2n_client_hello_parse and my new method.

Call-outs:

  • I wonder if I should change "s2n_client_hello_parse_bytes" to just "s2n_client_hello_parse" and rename the internal method to something like "s2n_client_hello_parse_for_connection".
  • I went back and forth on whether s2n_client_hello_parse_bytes should return int or a pointer. I think a pointer is the right choice so that it matches s2n_connection_get_client_hello and we never have an uninitialized s2n_client_hello.
  • I also updated the grep_simple_mistakes script. The part verifying that we null-checked pointers returned by s2n_stuffer_raw_read was very out of date and checking for macros that no longer exist.

Testing:

I added some basic unit tests for the new methods.
I also integrated the new methods into the ja3 fingerprinting tests, including the known-value tests. This should ensure that client hellos from raw bytes and client hellos from connections don't parse input differently and produce the same JA3.

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 Mar 9, 2023
@lrstewart lrstewart marked this pull request as ready for review March 9, 2023 18:35
@lrstewart lrstewart requested a review from dougch as a code owner March 9, 2023 18:35
* @param size The size of raw_message.
* @returns A new s2n_client_hello on success, or NULL on failure.
*/
struct s2n_client_hello *s2n_client_hello_parse_bytes(uint8_t *bytes, uint32_t size);
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, it would be helpful if this function(or another) could guide the caller on how many more bytes are needed to parse the client hello.

I know this can be solved with TLS record parsing in the application, but even that kind of code would ideally live inside of s2n and the application can just supply bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, the use case we have is that customers have complete ClientHello messages, not unparsed records. But I realized the first version didn't actually expect complete ClientHello messages-- I forgot about the message header!

Parsing the client hello from TLS records is an interesting use case, but I think it's out of scope for now. However, in case we want to handle it in the future, I've renamed the function to make it clear that it operates on messages, not records. You're right that records would require a different signature with some way to indicate "partial success".

api/unstable/fingerprint.h Outdated Show resolved Hide resolved
RESULT_GUARD_POSIX(s2n_stuffer_read_bytes(in, client_protocol_version, S2N_TLS_PROTOCOL_VERSION_LEN));

/* random */
RESULT_GUARD_POSIX(s2n_stuffer_erase_and_read_bytes(in, client_random, S2N_TLS_RANDOM_DATA_LEN));
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this erase the customer's client hello?

Copy link
Contributor Author

@lrstewart lrstewart Mar 21, 2023

Choose a reason for hiding this comment

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

Good question, it scared me for a minute ;) No, because we've already copied the customer's client hello into the s2n_client_hello's raw message blob. We just erase the random bytes from our copy. But I added a test to verify

I could make the s2n_client_hello_parse_message's bytes parameter const, but that would require us to copy it a second time in order to parse it with any of our stuffer or blob functions, because blobs and stuffers aren't const :/ But maybe the second copy is worth the better signature?

Copy link
Contributor

Choose a reason for hiding this comment

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

const is never helpful to us :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got a second opinion from @alexw91 and decided to make the function signature correct and use const. This isn't a super performance sensitive API, so the extra copy shouldn't be a problem.

api/unstable/fingerprint.h Outdated Show resolved Hide resolved
tls/s2n_client_hello.c Show resolved Hide resolved
tls/s2n_client_hello.c Outdated Show resolved Hide resolved
tests/unit/s2n_client_hello_test.c Show resolved Hide resolved
@lrstewart lrstewart merged commit bcaaaeb into aws:main Mar 21, 2023
@lrstewart lrstewart deleted the ja3_raw branch March 21, 2023 22:51
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.

Allow ClientHello parsing from raw bytes
4 participants