From aeb0262c111aba457462a18e0ab779a1c77ad783 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Wed, 4 Jun 2014 11:40:03 -0700 Subject: [PATCH] go.crypto/openpgp: fix hash presence checks. At some point in the distant past, crypto.Hash.New() changed from returning nil when a hash function wasn't provided, to panicing. Some of the code in openpgp predates this and was still using the nil check. LGTM=bradfitz R=sburford, bradfitz CC=golang-codereviews https://golang.org/cl/104850045 --- openpgp/clearsign/clearsign.go | 4 ++-- openpgp/packet/public_key_v3.go | 3 ++- openpgp/read.go | 4 ++-- openpgp/read_test.go | 32 ++++++++++++++++++++++++++++++++ openpgp/s2k/s2k.go | 4 ++-- 5 files changed, 40 insertions(+), 7 deletions(-) diff --git a/openpgp/clearsign/clearsign.go b/openpgp/clearsign/clearsign.go index a753bcf..4f7bc2c 100644 --- a/openpgp/clearsign/clearsign.go +++ b/openpgp/clearsign/clearsign.go @@ -296,10 +296,10 @@ func Encode(w io.Writer, privateKey *packet.PrivateKey, config *packet.Config) ( return nil, errors.UnsupportedError("unknown hash type: " + strconv.Itoa(int(hashType))) } - h := hashType.New() - if h == nil { + if !hashType.Available() { return nil, errors.UnsupportedError("unsupported hash type: " + strconv.Itoa(int(hashType))) } + h := hashType.New() buffered := bufio.NewWriter(w) // start has a \n at the beginning that we don't want here. diff --git a/openpgp/packet/public_key_v3.go b/openpgp/packet/public_key_v3.go index 8028996..0897bcc 100644 --- a/openpgp/packet/public_key_v3.go +++ b/openpgp/packet/public_key_v3.go @@ -237,9 +237,10 @@ func (pk *PublicKeyV3) VerifyKeySignatureV3(signed *PublicKeyV3, sig *SignatureV // userIdSignatureV3Hash returns a Hash of the message that needs to be signed // to assert that pk is a valid key for id. func userIdSignatureV3Hash(id string, pk signingKey, hfn crypto.Hash) (h hash.Hash, err error) { - if h = hfn.New(); h == nil { + if !hfn.Available() { return nil, errors.UnsupportedError("hash function") } + h = hfn.New() // RFC 4880, section 5.2.4 pk.SerializeSignaturePrefix(h) diff --git a/openpgp/read.go b/openpgp/read.go index da59dcc..94c4e6a 100644 --- a/openpgp/read.go +++ b/openpgp/read.go @@ -276,10 +276,10 @@ FindLiteralData: // returns two hashes. The second should be used to hash the message itself and // performs any needed preprocessing. func hashForSignature(hashId crypto.Hash, sigType packet.SignatureType) (hash.Hash, hash.Hash, error) { - h := hashId.New() - if h == nil { + if !hashId.Available() { return nil, nil, errors.UnsupportedError("hash not available: " + strconv.Itoa(int(hashId))) } + h := hashId.New() switch sigType { case packet.SigTypeBinary: diff --git a/openpgp/read_test.go b/openpgp/read_test.go index 3c90837..b08469d 100644 --- a/openpgp/read_test.go +++ b/openpgp/read_test.go @@ -11,6 +11,7 @@ import ( "encoding/hex" "io" "io/ioutil" + "strings" "testing" ) @@ -285,6 +286,33 @@ func TestDetachedSignatureDSA(t *testing.T) { testDetachedSignature(t, kring, readerFromHex(detachedSignatureDSAHex), signedInput, "binary", testKey3KeyId) } +func testHashFunctionError(t *testing.T, signatureHex string) { + kring, _ := ReadKeyRing(readerFromHex(testKeys1And2Hex)) + _, err := CheckDetachedSignature(kring, nil, readerFromHex(signatureHex)) + if err == nil { + t.Fatal("Packet with bad hash type was correctly parsed") + } + unsupported, ok := err.(errors.UnsupportedError) + if !ok { + t.Fatalf("Unexpected class of error: %s", err) + } + if !strings.Contains(string(unsupported), "hash ") { + t.Fatalf("Unexpected error: %s", err) + } +} + +func TestUnknownHashFunction(t *testing.T) { + // unknownHashFunctionHex contains a signature packet with hash + // function type 153 (which isn't a real hash function id). + testHashFunctionError(t, unknownHashFunctionHex) +} + +func TestMissingHashFunction(t *testing.T) { + // missingHashFunctionHex contains a signature packet that uses + // RIPEMD160, which isn't compiled in. + testHashFunctionError(t, missingHashFunctionHex) +} + func TestReadingArmoredPrivateKey(t *testing.T) { el, err := ReadArmoredKeyRing(bytes.NewBufferString(armoredPrivateKeyBlock)) if err != nil { @@ -373,3 +401,7 @@ VrM0m72/jnpKo04= -----END PGP PRIVATE KEY BLOCK-----` const dsaKeyWithSHA512 = `9901a2044f04b07f110400db244efecc7316553ee08d179972aab87bb1214de7692593fcf5b6feb1c80fba268722dd464748539b85b81d574cd2d7ad0ca2444de4d849b8756bad7768c486c83a824f9bba4af773d11742bdfb4ac3b89ef8cc9452d4aad31a37e4b630d33927bff68e879284a1672659b8b298222fc68f370f3e24dccacc4a862442b9438b00a0ea444a24088dc23e26df7daf8f43cba3bffc4fe703fe3d6cd7fdca199d54ed8ae501c30e3ec7871ea9cdd4cf63cfe6fc82281d70a5b8bb493f922cd99fba5f088935596af087c8d818d5ec4d0b9afa7f070b3d7c1dd32a84fca08d8280b4890c8da1dde334de8e3cad8450eed2a4a4fcc2db7b8e5528b869a74a7f0189e11ef097ef1253582348de072bb07a9fa8ab838e993cef0ee203ff49298723e2d1f549b00559f886cd417a41692ce58d0ac1307dc71d85a8af21b0cf6eaa14baf2922d3a70389bedf17cc514ba0febbd107675a372fe84b90162a9e88b14d4b1c6be855b96b33fb198c46f058568817780435b6936167ebb3724b680f32bf27382ada2e37a879b3d9de2abe0c3f399350afd1ad438883f4791e2e3b4184453412068617368207472756e636174696f6e207465737488620413110a002205024f04b07f021b03060b090807030206150802090a0b0416020301021e01021780000a0910ef20e0cefca131581318009e2bf3bf047a44d75a9bacd00161ee04d435522397009a03a60d51bd8a568c6c021c8d7cf1be8d990d6417b0020003` + +const unknownHashFunctionHex = `8a00000040040001990006050253863c24000a09103b4fe6acc0b21f32ffff01010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101` + +const missingHashFunctionHex = `8a00000040040001030006050253863c24000a09103b4fe6acc0b21f32ffff01010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101` diff --git a/openpgp/s2k/s2k.go b/openpgp/s2k/s2k.go index 33462cc..aab3041 100644 --- a/openpgp/s2k/s2k.go +++ b/openpgp/s2k/s2k.go @@ -91,10 +91,10 @@ func Parse(r io.Reader) (f func(out, in []byte), err error) { if !ok { return nil, errors.UnsupportedError("hash for S2K function: " + strconv.Itoa(int(buf[1]))) } - h := hash.New() - if h == nil { + if !hash.Available() { return nil, errors.UnsupportedError("hash not available: " + strconv.Itoa(int(hash))) } + h := hash.New() switch buf[0] { case 0: