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

crypto/tls: fix pseudo-constant mitigation for lucky 13 #27071

Open
dgryski opened this issue Aug 18, 2018 · 11 comments
Open

crypto/tls: fix pseudo-constant mitigation for lucky 13 #27071

dgryski opened this issue Aug 18, 2018 · 11 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@dgryski
Copy link
Contributor

dgryski commented Aug 18, 2018

As detailed in the paper "Pseudo Constant Time Implementations of TLS
Are Only Pseudo Secure"
https://eprint.iacr.org/2018/747
@josharian
Copy link
Contributor

cc @FiloSottile

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 4, 2018
@andybons andybons added this to the Unplanned milestone Sep 4, 2018
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/170701 mentions this issue: crypto/tls: fix a minor MAC vs padding leak

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/170704 mentions this issue: crypto/tls: implement full Lucky13 countermeasures for SHA-1 CBC ciphers

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/170702 mentions this issue: crypto/tls: avoid cache-based side channels in extracting the remote MAC

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/170703 mentions this issue: crypto/sha1: add ConstantTimeSumWithData

gopherbot pushed a commit that referenced this issue Apr 16, 2019
@davidben @FiloSottile
crypto/tls: fix a minor MAC vs padding leak
d7df9de 
The CBC mode ciphers in TLS are a disaster. By ordering authentication
and encryption wrong, they are very subtly dependent on details and
implementation of the padding check, admitting attacks such as POODLE
and Lucky13.

crypto/tls does not promise full countermeasures for Lucky13 and still
contains some timing variations. This change fixes one of the easy ones:
by checking the MAC, then the padding, rather than all at once, there is
a very small timing variation between bad MAC and (good MAC, bad
padding).

The consequences depend on the effective padding value used in the MAC
when the padding is bad. extractPadding simply uses the last byte's
value, leaving the padding bytes effectively unchecked. This is the
scenario in SSL 3.0 that led to POODLE. Specifically, the attacker can
take an input record which uses 16 bytes of padding (a full block) and
replace the final block with some interesting block. The MAC check will
succeed with 1/256 probability due to the final byte being 16. This
again means that after 256 queries, the attacker can decrypt one byte.

To fix this, bitwise AND the two values so they may be checked with one
branch. Additionally, zero the padding if the padding check failed, to
make things more robust.

Updates #27071

Change-Id: I332b14d215078928ffafe3cfeba1a68189f08db3
Reviewed-on: https://go-review.googlesource.com/c/go/+/170701
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@FiloSottile FiloSottile modified the milestones: Unplanned, Go1.14 Oct 2, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.14 Oct 23, 2019
@ianlancetaylor
Copy link
Contributor

@FiloSottile Is there more to do here for 1.14?

@odeke-em
Copy link
Member

odeke-em commented Feb 9, 2020

Kindly ping @FiloSottile @katiehockman, shall we move this issue back to backlog, or is there something left for Go1.14 and then we can close it?

@katiehockman
Copy link
Contributor

Since we're in the freeze and the RC1 has been cut, I think we should move this to the 1.15 milestone. @FiloSottile what do you think? It looks like you gave a +2 to one of the CLs early last year.

@toothrot toothrot modified the milestones: Go1.14, Go1.15 Feb 25, 2020
@FiloSottile FiloSottile self-assigned this Mar 31, 2020
@odeke-em
Copy link
Member

Punting to Go1.16 as no movement on the CLs since April 2019.

@odeke-em odeke-em modified the milestones: Go1.15, Go1.16 May 13, 2020
@odeke-em
Copy link
Member

Punting to Go1.17.

@odeke-em odeke-em modified the milestones: Go1.16, Go1.17 Dec 25, 2020
@ianlancetaylor
Copy link
Contributor

Moving to Backlog.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.17, Backlog Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
10 participants
@toothrot @josharian @rsc @andybons @dgryski @FiloSottile @ianlancetaylor @odeke-em @gopherbot @katiehockman