Merge pull request #2757 from lucas-clemente/key-phase-error-on-successful-decryption

only return an invalid first key phase error for decryptable packets

internal/handshake/updatable_aead.go

@@ -153,26 +153,31 @@ func (a *updatableAEAD) Open(dst, src []byte, rcvTime time.Time, pn protocol.Pac
 	}
 	binary.BigEndian.PutUint64(a.nonceBuf[len(a.nonceBuf)-8:], uint64(pn))
 	if kp != a.keyPhase.Bit() {
+		var receivedWrongInitialKeyPhase bool
 		if a.firstRcvdWithCurrentKey == protocol.InvalidPacketNumber || pn < a.firstRcvdWithCurrentKey {
 			if a.keyPhase == 0 {
 				// This can only occur when the first packet received has key phase 1.
 				// This is an error, since the key phase starts at 0,
 				// and peers are only allowed to update keys after the handshake is confirmed.
-				return nil, qerr.NewError(qerr.ProtocolViolation, "wrong initial keyphase")
+				// Proceed from here, and only return an error if decryption of the packet succeeds.
+				receivedWrongInitialKeyPhase = true
+			} else {
+				if a.prevRcvAEAD == nil {
+					return nil, ErrKeysDropped
+				}
+				// we updated the key, but the peer hasn't updated yet
+				dec, err := a.prevRcvAEAD.Open(dst, a.nonceBuf, src, ad)
+				if err != nil {
+					err = ErrDecryptionFailed
+				}
+				return dec, err
 			}
-			if a.prevRcvAEAD == nil {
-				return nil, ErrKeysDropped
-			}
-			// we updated the key, but the peer hasn't updated yet
-			dec, err := a.prevRcvAEAD.Open(dst, a.nonceBuf, src, ad)
-			if err != nil {
-				err = ErrDecryptionFailed
-			}
-			return dec, err
 		}
 		// try opening the packet with the next key phase
 		dec, err := a.nextRcvAEAD.Open(dst, a.nonceBuf, src, ad)
-		if err != nil {
+		if err == nil && receivedWrongInitialKeyPhase {
+			return nil, qerr.NewError(qerr.ProtocolViolation, "wrong initial key phase")
+		} else if err != nil {
 			return nil, ErrDecryptionFailed
 		}
 		// Opening succeeded. Check if the peer was allowed to update.

internal/handshake/updatable_aead_test.go

@@ -207,7 +207,15 @@ var _ = Describe("Updatable AEAD", func() {
 							client.rollKeys(time.Now())
 							encrypted := client.Seal(nil, msg, 0x1337, ad)
 							_, err := server.Open(nil, encrypted, time.Now(), 0x1337, protocol.KeyPhaseOne, ad)
-							Expect(err).To(MatchError("PROTOCOL_VIOLATION: wrong initial keyphase"))
+							Expect(err).To(MatchError("PROTOCOL_VIOLATION: wrong initial key phase"))
+						})
+
+						It("only errors when the peer starts with key phase 1 if decrypting the packet succeeds", func() {
+							client.rollKeys(time.Now())
+							encrypted := client.Seal(nil, msg, 0x1337, ad)
+							encrypted = encrypted[:len(encrypted)-1]
+							_, err := server.Open(nil, encrypted, time.Now(), 0x1337, protocol.KeyPhaseOne, ad)
+							Expect(err).To(MatchError(ErrDecryptionFailed))
 						})
 
 						It("errors when the peer updates keys too frequently", func() {