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

crypto/tls: randomly generate ticket_age_add [freeze exception] #52814

Closed
nervuri opened this issue May 10, 2022 · 15 comments
Closed

crypto/tls: randomly generate ticket_age_add [freeze exception] #52814

nervuri opened this issue May 10, 2022 · 15 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Milestone

Comments

@nervuri
Copy link

nervuri commented May 10, 2022

crypto/tls always sets newSessionTicketMsgTLS13.ageAdd to 0, which makes it so that clients resuming a session can't obfuscate the obfusacted_ticket_age. This violates the TLS 1.3 spec (RFC 8446, section 4.6.1):

ticket_age_add: A securely generated, random 32-bit value that is
used to obscure the age of the ticket that the client includes in
the "pre_shared_key" extension. The client-side ticket age is
added to this value modulo 2^32 to obtain the value that is
transmitted by the client. The server MUST generate a fresh value
for each ticket it sends.

See the sendSessionTickets() function.

How to reproduce

  • Run a simple TLS server: https://go.dev/play/p/t2moO8mDTmb (notice I set srv.SetKeepAlivesEnabled(false); we don't want connection reuse)
  • open Wireshark, listen on loopback interface and filter on tls.handshake
  • curl -k https://localhost:8443 https://localhost:8443

In Wireshark, open the second Client Hello message, look at the pre_shared_key extension and you'll see that obfuscated_ticket_age is 0 (or very close to 0).

Proposed fix

Given that you don't check the obfuscated_ticket_age, it's enough to assign ageAdd a random value each time.

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 10, 2022
@seankhliao
Copy link
Member

seankhliao commented May 10, 2022

cc @golang/security

@FiloSottile
Copy link
Contributor

FiloSottile commented May 10, 2022

Ah, yes, that's an oversight, thank you.

I would argue this is worth a freeze exception, because the fix is very simple, we are early in the freeze, and it fixes a privacy issue (allowing network observers to correlate successive connections even if the client changed location). Maybe even worth a CVE and a backport? Not sure. /cc @golang/release

If we ever expose the session ticket structure (which might be something that happens as part of #25351 #46718 #19199 #6379), we might want to add the obfuscated_ticket_age to it (or start generating it from the key material?) in case other implementations want to use it, but for now we are the only consumers of the tickets and indeed don't care about it.

(While at it, let's also add a note about why ticket_nonce is always zero, which is simply that we only ever send one ticket. It's a good comment to have in there in case that invariant ever changes.)

@FiloSottile FiloSottile changed the title crypto/tls: ticket_age_add is always 0 crypto/tls: randomly generate ticket_age_add [freeze exception] May 10, 2022
@nervuri
Copy link
Author

nervuri commented May 10, 2022

Here's my patch, for what it's worth:

https://github.com/nervuri/go/commit/1cfb6b392b1d22127c9f6afcf45619d190ed9bfe

Not sure if I should submit it as a pull request... for such a small change, the contribution setup is a tad overkill. :) And you may want to write the patch differently. What do you think?

@heschi
Copy link
Contributor

heschi commented May 10, 2022

cc @golang/security

If the fix is easy and we can get a CL in before the end of the week I think it's fine. Up to the security team to decide about a CVE/backport.

@rolandshoemaker
Copy link
Member

rolandshoemaker commented May 10, 2022

We will work to get a CL in this week. Backporting seems reasonable given how small the fix is.

@gopherbot Please open backport issues for Go 1.17 and Go 1.18, this is a privacy issue.

@gopherbot
Copy link

gopherbot commented May 10, 2022

Backport issue(s) opened: #52832 (for 1.17), #52833 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@komuw
Copy link
Contributor

komuw commented May 11, 2022

Not sure if I should submit it as a pull request..., the contribution setup is a tad overkill

The Go project does accept normal github pull requests: https://go.dev/doc/contribute#sending_a_change_github

@nervuri
Copy link
Author

nervuri commented May 11, 2022

The Go project does accept normal github pull requests: https://go.dev/doc/contribute#sending_a_change_github

Yes, but to contribute I need to sign a CLA, for which I need a Google account, for which I have to provide my phone number. I would not have found this bug if I didn't care about privacy. Requiring all contributors to get an account with Big Brother is just rude.

My pull request won't go through, but do feel free to use the code. I am providing it to the project under the project's license (although the patch is so small that it's probably not copyrightable).

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented May 12, 2022

@nervuri I completely understand not wanting to sign the CLA, but in that situation please don't send us code via GitHub pull requests or e-mail or in any other way. Our legal advice is very clear: we can only accept code changes from people who have signed the CLA. When you send us code without signing the CLA we have to be careful to not look at that code and not rely on it in any way in making our own changes. Thanks for understanding.

@nervuri
Copy link
Author

nervuri commented May 12, 2022

I would be willing to sign the CLA if it didn't require using a Google account and providing Google my phone number. Golang could use Github's cla-bot, for instance. Or I'll just do it here: I hereby sign the CLA currently hosted at https://cla.developers.google.com/about/google-individual

Anyway, I closed the pull request - hope that makes it easier for you. I'm sure nobody looked at the code. :)

@gopherbot
Copy link

gopherbot commented May 12, 2022

Change https://go.dev/cl/405994 mentions this issue: crypto/tls: randomly generate ticket_age_add

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 17, 2022
@dmitshur dmitshur added this to the Go1.19 milestone May 17, 2022
@cagedmantis
Copy link
Contributor

cagedmantis commented May 17, 2022

This freeze exception has been approved.

@gopherbot
Copy link

gopherbot commented May 25, 2022

Change https://go.dev/cl/408574 mentions this issue: [release-branch.go1.17] crypto/tls: randomly generate ticket_age_add

@gopherbot
Copy link

gopherbot commented May 25, 2022

Change https://go.dev/cl/408575 mentions this issue: [release-branch.go1.18 crypto/tls: randomly generate ticket_age_add

gopherbot pushed a commit that referenced this issue May 27, 2022
As required by RFC 8446, section 4.6.1, ticket_age_add now holds a
random 32-bit value. Before this change, this value was always set
to 0.

This change also documents the reasoning for always setting
ticket_nonce to 0. The value ticket_nonce must be unique per
connection, but we only ever send one ticket per connection.

Updates #52814
Fixes #52833
Fixes CVE-2022-30629

Change-Id: I6c2fc6ca0376b7b968abd59d6d3d3854c1ab68bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/405994
Reviewed-by: Tatiana Bradley <tatiana@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Tatiana Bradley <tatiana@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit fe4de36)
Reviewed-on: https://go-review.googlesource.com/c/go/+/408575
Run-TryBot: Roland Shoemaker <roland@golang.org>
gopherbot pushed a commit that referenced this issue May 27, 2022
As required by RFC 8446, section 4.6.1, ticket_age_add now holds a
random 32-bit value. Before this change, this value was always set
to 0.

This change also documents the reasoning for always setting
ticket_nonce to 0. The value ticket_nonce must be unique per
connection, but we only ever send one ticket per connection.

Updates #52814
Fixes #52832
Fixes CVE-2022-30629

Change-Id: I6c2fc6ca0376b7b968abd59d6d3d3854c1ab68bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/405994
Reviewed-by: Tatiana Bradley <tatiana@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Tatiana Bradley <tatiana@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit fe4de36)
Reviewed-on: https://go-review.googlesource.com/c/go/+/408574
Run-TryBot: Roland Shoemaker <roland@golang.org>
@tsaarni
Copy link

tsaarni commented Jun 10, 2022

Would it be possible to cast some more light for the reason for issuing CVE-2022-30629? As an outsider, this seems more like RFC 8446 non-compliance issue since the server does not use obfusacted_ticket_age (link).

danbudris pushed a commit to danbudris/go that referenced this issue Sep 9, 2022
As required by RFC 8446, section 4.6.1, ticket_age_add now holds a
random 32-bit value. Before this change, this value was always set
to 0.

This change also documents the reasoning for always setting
ticket_nonce to 0. The value ticket_nonce must be unique per
connection, but we only ever send one ticket per connection.

Updates golang#52814
Fixes golang#52832
Fixes CVE-2022-30629

Change-Id: I6c2fc6ca0376b7b968abd59d6d3d3854c1ab68bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/405994
Reviewed-by: Tatiana Bradley <tatiana@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Tatiana Bradley <tatiana@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit fe4de36)
Reviewed-on: https://go-review.googlesource.com/c/go/+/408574
Run-TryBot: Roland Shoemaker <roland@golang.org>
danbudris pushed a commit to danbudris/go that referenced this issue Sep 14, 2022
As required by RFC 8446, section 4.6.1, ticket_age_add now holds a
random 32-bit value. Before this change, this value was always set
to 0.

This change also documents the reasoning for always setting
ticket_nonce to 0. The value ticket_nonce must be unique per
connection, but we only ever send one ticket per connection.

Updates golang#52814
Fixes golang#52832
Fixes CVE-2022-30629

Change-Id: I6c2fc6ca0376b7b968abd59d6d3d3854c1ab68bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/405994
Reviewed-by: Tatiana Bradley <tatiana@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Tatiana Bradley <tatiana@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit fe4de36)
Reviewed-on: https://go-review.googlesource.com/c/go/+/408574
Run-TryBot: Roland Shoemaker <roland@golang.org>
rcrozean pushed a commit to rcrozean/go that referenced this issue Oct 5, 2022
# AWS EKS
Backported To: go-1.15.15-eks
Backported On: Thu, 22 Sept 2022
Backported By: budris@amazon.com
Backported From: release-branch.go1.17
EKS Patch Source Commit: danbudris@e25bfe0
Upstream Source Commit: golang@c15a8e2

# Original Information

As required by RFC 8446, section 4.6.1, ticket_age_add now holds a
random 32-bit value. Before this change, this value was always set
to 0.

This change also documents the reasoning for always setting
ticket_nonce to 0. The value ticket_nonce must be unique per
connection, but we only ever send one ticket per connection.

Updates golang#52814
Fixes golang#52832
Fixes CVE-2022-30629

Change-Id: I6c2fc6ca0376b7b968abd59d6d3d3854c1ab68bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/405994
Reviewed-by: Tatiana Bradley <tatiana@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Tatiana Bradley <tatiana@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit fe4de36)
Reviewed-on: https://go-review.googlesource.com/c/go/+/408574
Run-TryBot: Roland Shoemaker <roland@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.