From 374053ea96cb300f8671b8d3b07edeeb06e203b4 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Thu, 15 Mar 2018 16:20:44 -0400 Subject: [PATCH] openpgp/packet: improve handling of short MPIs for RSA values MPIs are (supposed to be) stripped of leading zeroes. Avoid passing such short values into crypto/rsa, even if it currently happens to work. Change-Id: I5a5f4813b8358e83fcc2deeda1272d2733814542 Reviewed-on: https://go-review.googlesource.com/100844 Reviewed-by: Adam Langley --- openpgp/packet/encrypted_key.go | 3 +- openpgp/packet/encrypted_key_test.go | 63 +++++++++++++++------------- openpgp/packet/packet.go | 32 +++++++++----- openpgp/packet/public_key.go | 4 +- 4 files changed, 60 insertions(+), 42 deletions(-) diff --git a/openpgp/packet/encrypted_key.go b/openpgp/packet/encrypted_key.go index 5ed3cd2..02b372c 100644 --- a/openpgp/packet/encrypted_key.go +++ b/openpgp/packet/encrypted_key.go @@ -78,7 +78,8 @@ func (e *EncryptedKey) Decrypt(priv *PrivateKey, config *Config) error { // padding oracle attacks. switch priv.PubKeyAlgo { case PubKeyAlgoRSA, PubKeyAlgoRSAEncryptOnly: - b, err = rsa.DecryptPKCS1v15(config.Random(), priv.PrivateKey.(*rsa.PrivateKey), e.encryptedMPI1.bytes) + k := priv.PrivateKey.(*rsa.PrivateKey) + b, err = rsa.DecryptPKCS1v15(config.Random(), k, padToKeySize(&k.PublicKey, e.encryptedMPI1.bytes)) case PubKeyAlgoElGamal: c1 := new(big.Int).SetBytes(e.encryptedMPI1.bytes) c2 := new(big.Int).SetBytes(e.encryptedMPI2.bytes) diff --git a/openpgp/packet/encrypted_key_test.go b/openpgp/packet/encrypted_key_test.go index fee14cf..f2fcf4d 100644 --- a/openpgp/packet/encrypted_key_test.go +++ b/openpgp/packet/encrypted_key_test.go @@ -39,39 +39,44 @@ var encryptedKeyPriv = &PrivateKey{ } func TestDecryptingEncryptedKey(t *testing.T) { - const encryptedKeyHex = "c18c032a67d68660df41c70104005789d0de26b6a50c985a02a13131ca829c413a35d0e6fa8d6842599252162808ac7439c72151c8c6183e76923fe3299301414d0c25a2f06a2257db3839e7df0ec964773f6e4c4ac7ff3b48c444237166dd46ba8ff443a5410dc670cb486672fdbe7c9dfafb75b4fea83af3a204fe2a7dfa86bd20122b4f3d2646cbeecb8f7be8" - const expectedKeyHex = "d930363f7e0308c333b9618617ea728963d8df993665ae7be1092d4926fd864b" + for i, encryptedKeyHex := range []string{ + "c18c032a67d68660df41c70104005789d0de26b6a50c985a02a13131ca829c413a35d0e6fa8d6842599252162808ac7439c72151c8c6183e76923fe3299301414d0c25a2f06a2257db3839e7df0ec964773f6e4c4ac7ff3b48c444237166dd46ba8ff443a5410dc670cb486672fdbe7c9dfafb75b4fea83af3a204fe2a7dfa86bd20122b4f3d2646cbeecb8f7be8", + // MPI can be shorter than the length of the key. + "c18b032a67d68660df41c70103f8e520c52ae9807183c669ce26e772e482dc5d8cf60e6f59316e145be14d2e5221ee69550db1d5618a8cb002a719f1f0b9345bde21536d410ec90ba86cac37748dec7933eb7f9873873b2d61d3321d1cd44535014f6df58f7bc0c7afb5edc38e1a974428997d2f747f9a173bea9ca53079b409517d332df62d805564cffc9be6", + } { + const expectedKeyHex = "d930363f7e0308c333b9618617ea728963d8df993665ae7be1092d4926fd864b" - p, err := Read(readerFromHex(encryptedKeyHex)) - if err != nil { - t.Errorf("error from Read: %s", err) - return - } - ek, ok := p.(*EncryptedKey) - if !ok { - t.Errorf("didn't parse an EncryptedKey, got %#v", p) - return - } + p, err := Read(readerFromHex(encryptedKeyHex)) + if err != nil { + t.Errorf("#%d: error from Read: %s", i, err) + return + } + ek, ok := p.(*EncryptedKey) + if !ok { + t.Errorf("#%d: didn't parse an EncryptedKey, got %#v", i, p) + return + } - if ek.KeyId != 0x2a67d68660df41c7 || ek.Algo != PubKeyAlgoRSA { - t.Errorf("unexpected EncryptedKey contents: %#v", ek) - return - } + if ek.KeyId != 0x2a67d68660df41c7 || ek.Algo != PubKeyAlgoRSA { + t.Errorf("#%d: unexpected EncryptedKey contents: %#v", i, ek) + return + } - err = ek.Decrypt(encryptedKeyPriv, nil) - if err != nil { - t.Errorf("error from Decrypt: %s", err) - return - } + err = ek.Decrypt(encryptedKeyPriv, nil) + if err != nil { + t.Errorf("#%d: error from Decrypt: %s", i, err) + return + } - if ek.CipherFunc != CipherAES256 { - t.Errorf("unexpected EncryptedKey contents: %#v", ek) - return - } + if ek.CipherFunc != CipherAES256 { + t.Errorf("#%d: unexpected EncryptedKey contents: %#v", i, ek) + return + } - keyHex := fmt.Sprintf("%x", ek.Key) - if keyHex != expectedKeyHex { - t.Errorf("bad key, got %s want %x", keyHex, expectedKeyHex) + keyHex := fmt.Sprintf("%x", ek.Key) + if keyHex != expectedKeyHex { + t.Errorf("#%d: bad key, got %s want %s", i, keyHex, expectedKeyHex) + } } } @@ -121,7 +126,7 @@ func TestEncryptingEncryptedKey(t *testing.T) { keyHex := fmt.Sprintf("%x", ek.Key) if keyHex != expectedKeyHex { - t.Errorf("bad key, got %s want %x", keyHex, expectedKeyHex) + t.Errorf("bad key, got %s want %s", keyHex, expectedKeyHex) } } diff --git a/openpgp/packet/packet.go b/openpgp/packet/packet.go index 3eded93..625bb5a 100644 --- a/openpgp/packet/packet.go +++ b/openpgp/packet/packet.go @@ -11,10 +11,12 @@ import ( "crypto/aes" "crypto/cipher" "crypto/des" - "golang.org/x/crypto/cast5" - "golang.org/x/crypto/openpgp/errors" + "crypto/rsa" "io" "math/big" + + "golang.org/x/crypto/cast5" + "golang.org/x/crypto/openpgp/errors" ) // readFull is the same as io.ReadFull except that reading zero bytes returns @@ -500,19 +502,17 @@ func readMPI(r io.Reader) (mpi []byte, bitLength uint16, err error) { numBytes := (int(bitLength) + 7) / 8 mpi = make([]byte, numBytes) _, err = readFull(r, mpi) - return -} - -// mpiLength returns the length of the given *big.Int when serialized as an -// MPI. -func mpiLength(n *big.Int) (mpiLengthInBytes int) { - mpiLengthInBytes = 2 /* MPI length */ - mpiLengthInBytes += (n.BitLen() + 7) / 8 + // According to RFC 4880 3.2. we should check that the MPI has no leading + // zeroes (at least when not an encrypted MPI?), but this implementation + // does generate leading zeroes, so we keep accepting them. return } // writeMPI serializes a big integer to w. func writeMPI(w io.Writer, bitLength uint16, mpiBytes []byte) (err error) { + // Note that we can produce leading zeroes, in violation of RFC 4880 3.2. + // Implementations seem to be tolerant of them, and stripping them would + // make it complex to guarantee matching re-serialization. _, err = w.Write([]byte{byte(bitLength >> 8), byte(bitLength)}) if err == nil { _, err = w.Write(mpiBytes) @@ -525,6 +525,18 @@ func writeBig(w io.Writer, i *big.Int) error { return writeMPI(w, uint16(i.BitLen()), i.Bytes()) } +// padToKeySize left-pads a MPI with zeroes to match the length of the +// specified RSA public. +func padToKeySize(pub *rsa.PublicKey, b []byte) []byte { + k := (pub.N.BitLen() + 7) / 8 + if len(b) >= k { + return b + } + bb := make([]byte, k) + copy(bb[len(bb)-len(b):], b) + return bb +} + // CompressionAlgo Represents the different compression algorithms // supported by OpenPGP (except for BZIP2, which is not currently // supported). See Section 9.3 of RFC 4880. diff --git a/openpgp/packet/public_key.go b/openpgp/packet/public_key.go index fbd60f6..fcd5f52 100644 --- a/openpgp/packet/public_key.go +++ b/openpgp/packet/public_key.go @@ -520,7 +520,7 @@ func (pk *PublicKey) VerifySignature(signed hash.Hash, sig *Signature) (err erro switch pk.PubKeyAlgo { case PubKeyAlgoRSA, PubKeyAlgoRSASignOnly: rsaPublicKey, _ := pk.PublicKey.(*rsa.PublicKey) - err = rsa.VerifyPKCS1v15(rsaPublicKey, sig.Hash, hashBytes, sig.RSASignature.bytes) + err = rsa.VerifyPKCS1v15(rsaPublicKey, sig.Hash, hashBytes, padToKeySize(rsaPublicKey, sig.RSASignature.bytes)) if err != nil { return errors.SignatureError("RSA verification failure") } @@ -571,7 +571,7 @@ func (pk *PublicKey) VerifySignatureV3(signed hash.Hash, sig *SignatureV3) (err switch pk.PubKeyAlgo { case PubKeyAlgoRSA, PubKeyAlgoRSASignOnly: rsaPublicKey := pk.PublicKey.(*rsa.PublicKey) - if err = rsa.VerifyPKCS1v15(rsaPublicKey, sig.Hash, hashBytes, sig.RSASignature.bytes); err != nil { + if err = rsa.VerifyPKCS1v15(rsaPublicKey, sig.Hash, hashBytes, padToKeySize(rsaPublicKey, sig.RSASignature.bytes)); err != nil { return errors.SignatureError("RSA verification failure") } return