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.

Sign up for GitHub

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

Jump to bottom

Trying to use an invalid ticket should not mutate state #4110

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Trying to use an invalid ticket should not mutate state #4110

merged 4 commits into from
Jul 25, 2023

Conversation

lrstewart
Copy link
Contributor

Resolved issues:

resolves #4102

Description of changes:

Customers want to ignore errors from s2n_connection_set_session. That can be risky when we may mutate the connection on failure. I think it was already fairly safe, because I think the protocol version was the only field we won't ignore the existing value of on a full handshake. But I can't be 100% sure, and not mutating the connection is the better choice.

Call-outs:

Does s2n_tls12_client_deserialize_session_state really need to operate on copies of the connection and secure? It's the safest solution. The three main issues with modifying the original connection were:

  • s2n_set_cipher_as_client: I could refactor it to set an input cipher suite (like s2n_set_cipher_as_client(conn, wire, &conn->secure.cipher_suite) / s2n_set_cipher_as_client(conn, wire, temp_cipher_suite)) but that affects everywhere else that method is called and could lead to us accidentally setting the cipher differently in set_session vs in response to a ServerHello.
  • secrets.version.tls12.master_secret: I could store the master secret in a temporary buffer and then copy it over later, but technically RESULT_CHECKED_MEMCPY could fail. In practice it won't, but it could.
  • client_ticket: similar to the master secret, technically s2n_freeing the old ticket can fail, even if in practice it won't.

Testing:

Testing was a bit tricky here, but I added new tests that I think can give us confidence that we're not mutating the connection and that changes to other code (like s2n_set_cipher_as_client) won't break that guarantee.

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 Jul 24, 2023
@lrstewart lrstewart marked this pull request as ready for review July 24, 2023 06:47
maddeleine
maddeleine reviewed Jul 24, 2023
View reviewed changes
tls/s2n_resume.c Show resolved Hide resolved
tls/s2n_resume.c Show resolved Hide resolved
tls/s2n_resume.c Outdated Show resolved Hide resolved
tls/s2n_resume.c Show resolved Hide resolved
tls/s2n_resume.c Show resolved Hide resolved
tls/s2n_resume.c Show resolved Hide resolved
tests/unit/s2n_resume_test.c Show resolved Hide resolved
@lrstewart lrstewart requested a review from jmayclin July 25, 2023 03:37
@lrstewart lrstewart enabled auto-merge (squash) July 25, 2023 21:55
@lrstewart lrstewart merged commit 403d5e6 into aws:main Jul 25, 2023
28 checks passed
@lrstewart lrstewart deleted the resume branch July 26, 2023 05:43
goatgoose added a commit to goatgoose/s2n-tls that referenced this pull request Jul 31, 2023
@goatgoose
Squashed commit of the following:
a5b1db2 
commit eafb8a2
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Mon Jul 31 18:05:50 2023 -0400

    shell -> bash

commit 12071b8
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Mon Jul 31 17:59:09 2023 -0400

    add ubuntu quickstart back to readme

commit 10bf557
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Mon Jul 31 17:52:06 2023 -0400

    fixes

commit 74adf8d
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Mon Jul 31 16:46:19 2023 -0400

    fixes

commit 0548d07
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Mon Jul 31 16:43:08 2023 -0400

    consolidate

commit cbe8f2d
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Mon Jul 31 14:55:30 2023 -0400

    remove old doc sections

commit f194321
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Mon Jul 31 12:25:28 2023 -0400

    more content

commit 882eb1d
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Mon Jul 31 09:08:24 2023 -0400

    fixes

commit ce37d0e
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Mon Jul 31 09:03:45 2023 -0400

    fixes

commit 011d15f
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Sat Jul 29 22:59:51 2023 -0400

    cmake consuming

commit 7feadc1
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Sat Jul 29 22:27:02 2023 -0400

    fixes

commit 2914950
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Sat Jul 29 21:34:24 2023 -0400

    traditional make

commit 02f9841
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Sat Jul 29 19:45:43 2023 -0400

    s2n-tls build section

commit 86c4983
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Sat Jul 29 11:56:32 2023 -0400

    Update build documentation

commit ea6d02a
Author: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Date:   Fri Jul 28 16:49:21 2023 -0400

    bindings: release 0.0.35 (aws#4122)

commit 35d08ba
Author: Justin Zhang <76919968+tinzh@users.noreply.github.com>
Date:   Fri Jul 28 12:31:21 2023 -0700

    refactor(bench): separate out client and server connections in benching harness (aws#4113)

    Enables more better control of connections for benching experiments

commit 65e74ca
Author: Lindsay Stewart <slindsay@amazon.com>
Date:   Wed Jul 26 02:26:40 2023 -0700

    Print error for 32bit test (aws#4107)

commit b0b253e
Author: toidiu <apoorv@toidiu.com>
Date:   Wed Jul 26 00:30:44 2023 -0700

    ktls: set keys on socket and enable ktls (aws#4071)

commit 403d5e6
Author: Lindsay Stewart <slindsay@amazon.com>
Date:   Tue Jul 25 16:03:09 2023 -0700

    Trying to use an invalid ticket should not mutate state (aws#4110)

commit bce2b1a
Author: James Mayclin <maycj@amazon.com>
Date:   Tue Jul 25 14:44:33 2023 -0700

    fix: get_session behavior for TLS 1.3 (aws#4104)

commit 6881358
Author: Justin Zhang <76919968+tinzh@users.noreply.github.com>
Date:   Tue Jul 25 10:10:21 2023 -0700

    feat(bench): add different certificate signature algorithms to benchmarks (aws#4080)

commit aab13d5
Author: Justin Zhang <76919968+tinzh@users.noreply.github.com>
Date:   Mon Jul 24 18:17:30 2023 -0700

    feat(bench): add memory bench with valgrind/massif (aws#4081)

commit 20b0174
Author: Justin Zhang <76919968+tinzh@users.noreply.github.com>
Date:   Mon Jul 24 13:26:32 2023 -0700

    feat(bench): add historical performance benchmark (aws#4083)

commit 5cc827d
Author: Doug Chapman <54039637+dougch@users.noreply.github.com>
Date:   Thu Jul 20 11:50:50 2023 -0700

    nix: pin corretto version (aws#4103)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmayclin jmayclin jmayclin approved these changes

@maddeleine maddeleine maddeleine approved these changes

@toidiu toidiu Awaiting requested review from toidiu

Assignees
No one assigned
Labels
s2n-core team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

s2n_connection_set_session shouldn't mutate on failure
4 participants
@lrstewart @jmayclin @maddeleine @dougch