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

fix: get_session behavior for TLS 1.3 #4104

Merged

jmayclin merged 18 commits into aws:main from jmayclin:additional-st-test

Jul 25, 2023

Contributor

jmayclin commented Jul 20, 2023

Description of changes:

We currently only clear the client_ticket if the connection is using TLS 1.2. This produces a bug where if a client sets a TLS 1.2 session ticket and then negotiates TLS 1.3, then calls s2n_connection_get_session to retrieve a session ticket before it has been received, they will receive a malformed session session ticket instead of receiving no session_ticket

The fix is to also clear the client_ticket if the connection is using TLS 1.3

Testing:

I added a unit test that fails under the current behavior (the assert on line 615 fails).
I then added a change to always clear the client ticket, and confirmed that the new unit test passes.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jmayclin added 3 commits

July 19, 2023 23:29


          repo for funky session ticket chain

3d36f49


          failing sanity check

1ccd1d5


          fix for 1.2 ticket confusion

7767d0b

jmayclin requested review from lrstewart and maddeleine

July 20, 2023 19:53


          Merge branch 'main' into additional-st-test

c114ff7

github-actions bot added the s2n-core label

Contributor

baldwinmatt commented Jul 21, 2023

Looks like CMBC and one other automated test failed. Did this change introduce regressions here?

Contributor Author

jmayclin commented Jul 21, 2023

It's possible, but seems unlikely. We've seen these failures on a few different PRs example so I suspect we are just having issues with the proofs themselves.

maddeleine reviewed

View reviewed changes

tests/unit/s2n_self_talk_session_resumption_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_self_talk_session_resumption_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_self_talk_session_resumption_test.c Outdated Show resolved Hide resolved


          address PR feedback

64ba924

* assert on blocked io
* add assert for get_session_length
* remove diagram

The actual request was to rewrite the diagram in english, but it was
getting very wordy so I just removed it.

maddeleine approved these changes

View reviewed changes

lrstewart reviewed

View reviewed changes

tests/s2n_test.h Outdated Show resolved Hide resolved

tls/s2n_server_hello.c Outdated Show resolved Hide resolved

tests/unit/s2n_self_talk_session_resumption_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_self_talk_session_resumption_test.c Outdated Show resolved Hide resolved

Contributor

lrstewart commented Jul 21, 2023

Oh and as part of this fix, could you also add a sanity check to the serialization logic so that we fail to create a ticket if the secret length is 0? It'll fail anyway on set_session, so it shouldn't succeed in get_session.

lrstewart reviewed

View reviewed changes

tests/unit/s2n_self_talk_session_resumption_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_self_talk_session_resumption_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_self_talk_session_resumption_test.c Outdated Show resolved Hide resolved

jmayclin added 5 commits

July 21, 2023 20:04


          Address PR feedback

ba5fadf

* revert block io asserts
* add sanity check for session secret size
* add deferred cleanups
* document s2n_realloc preference


          Address PR feedback

d1802f3

* allocate memory for the ticket to ensure that we get a useful error
  rather than a segfault
* address failing tests from added sanity check


          remove print statements

ef21857


          remove unused variable

abe6db6


          Address PR feedback

c76edac

* remove the additional handshake

jmayclin requested a review from lrstewart

July 21, 2023 21:57

jmayclin enabled auto-merge (squash)

July 22, 2023 03:07

lrstewart reviewed

View reviewed changes

utils/s2n_mem.c Outdated Show resolved Hide resolved

tests/unit/s2n_self_talk_session_resumption_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_self_talk_session_resumption_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_self_talk_session_resumption_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_resume_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_resume_test.c Show resolved Hide resolved

tests/unit/s2n_resume_test.c Outdated Show resolved Hide resolved

jmayclin added 4 commits

July 24, 2023 20:42


          address PR feedback

* move comment to header file
* use generic ticket.size rather than 512
* remove extra information from test name
* remove session ticket retrieval from test
* add separate test for zero length session ticket


          Merge branch 'main' into additional-st-test

36a1dbf


          address PR feedback

6c9f5dd

* add more secret sizes


          correct print message

2ccacc7

lrstewart reviewed

View reviewed changes

tests/unit/s2n_self_talk_session_resumption_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_resume_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_resume_test.c Outdated Show resolved Hide resolved

jmayclin added 2 commits

July 25, 2023 17:54


          address PR feedback

e85187f

* remove comment about non-zero ticket size
* add control case for test
* remove unnecessary zero


          Merge branch 'main' into additional-st-test

999c0f3

lrstewart approved these changes

View reviewed changes

tests/unit/s2n_resume_test.c Outdated Show resolved Hide resolved

jmayclin and others added 2 commits

July 25, 2023 11:31


          Update tests/unit/s2n_resume_test.c

2999f2f

Co-authored-by: Lindsay Stewart <stewart.r.lindsay@gmail.com>


          clean up memory in test

13fbd39

bc I am a silly goose

jmayclin merged commit bce2b1a into aws:main

23 checks passed

goatgoose added a commit to goatgoose/s2n-tls that referenced this pull request


          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)

jmayclin deleted the additional-st-test branch

December 22, 2023 02:03

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels