crypto/tls: fix a minor MAC vs padding leak

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/+/170701Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

src/crypto/tls/conn.go

@@ -274,6 +274,17 @@ func extractPadding(payload []byte) (toRemove int, good byte) {
 	good &= good << 1
 	good = uint8(int8(good) >> 7)
 
+	// Zero the padding length on error. This ensures any unchecked bytes
+	// are included in the MAC. Otherwise, an attacker that could
+	// distinguish MAC failures from padding failures could mount an attack
+	// similar to POODLE in SSL 3.0: given a good ciphertext that uses a
+	// full block's worth of padding, replace the final block with another
+	// block. If the MAC check passed but the padding check failed, the
+	// last byte of that block decrypted to the block size.
+	//
+	// See also macAndPaddingGood logic below.
+	paddingLen &= good
+
 	toRemove = int(paddingLen) + 1
 	return
 }
@@ -416,7 +427,15 @@ func (hc *halfConn) decrypt(record []byte) ([]byte, recordType, error) {
 		remoteMAC := payload[n : n+macSize]
 		localMAC := hc.mac.MAC(hc.seq[0:], record[:recordHeaderLen], payload[:n], payload[n+macSize:])
 
-		if subtle.ConstantTimeCompare(localMAC, remoteMAC) != 1 || paddingGood != 255 {
+		// This is equivalent to checking the MACs and paddingGood
+		// separately, but in constant-time to prevent distinguishing
+		// padding failures from MAC failures. Depending on what value
+		// of paddingLen was returned on bad padding, distinguishing
+		// bad MAC from bad padding can lead to an attack.
+		//
+		// See also the logic at the end of extractPadding.
+		macAndPaddingGood := subtle.ConstantTimeCompare(localMAC, remoteMAC) & int(paddingGood)
+		if macAndPaddingGood != 1 {
 			return nil, 0, alertBadRecordMAC
 		}