Skip to content

Commit

Permalink
crypto/tls: remove a forgotten note to future self
Browse files Browse the repository at this point in the history
Now, this is embarrassing. While preparing CL 142818, I noticed a
possible vulnerability in the existing code which I was rewriting. I
took a note to go back and assess if it was indeed an issue, and in case
start the security release process. The note unintentionally slipped
into the commit. Fortunately, there was no vulnerability.

What caught my eye was that I had fixed the calculation of the minimum
encrypted payload length from

    roundUp(explicitIVLen+macSize+1, blockSize)

to (using the same variable names)

    explicitIVLen + roundUp(macSize+1, blockSize)

The explicit nonce sits outside of the encrypted payload, so it should
not be part of the value rounded up to the CBC block size.

You can see that for some values of the above, the old result could be
lower than the correct value. An unexpectedly short payload might cause
a panic during decryption (a DoS vulnerability) or even more serious
issues due to the constant time code that follows it (see for example
Yet Another Padding Oracle in OpenSSL CBC Ciphersuites [1]).

In practice, explicitIVLen is either zero or equal to blockSize, so it
does not change the amount of rounding up necessary and the two
formulations happen to be identical. Nothing to see here.

It looked more suspicious than it is in part due to the fact that the
explicitIVLen definition moved farther into hc.explicitNonceLen() and
changed name from IV (which suggests a block length) to nonce (which
doesn't necessarily). But anyway it was never meant to surface or be
noted, except it slipped, so here we are for a boring explanation.

[1] https://blog.cloudflare.com/yet-another-padding-oracle-in-openssl-cbc-ciphersuites/

Change-Id: I365560dfe006513200fa877551ce7afec9115fdf
Reviewed-on: https://go-review.googlesource.com/c/147637
Reviewed-by: Brad Fitzpatrick <[email protected]>
  • Loading branch information
FiloSottile committed Nov 8, 2018
1 parent d258bac commit 05a85f4
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/crypto/tls/conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ func (hc *halfConn) decrypt(record []byte) ([]byte, recordType, error) {
}
case cbcMode:
blockSize := c.BlockSize()
minPayload := explicitNonceLen + roundUp(hc.mac.Size()+1, blockSize) // TODO: vuln?
minPayload := explicitNonceLen + roundUp(hc.mac.Size()+1, blockSize)
if len(payload)%blockSize != 0 || len(payload) < minPayload {
return nil, 0, alertBadRecordMAC
}
Expand Down

0 comments on commit 05a85f4

Please sign in to comment.