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 <agl@golang.org>
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 @@
// 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 @@
}
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 @@
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 @@
"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 @@
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 @@
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 @@
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 @@
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
