Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/rsa: invalid RIPEMD160 hash prefix #35495

Closed
odeke-em opened this issue Nov 10, 2019 · 4 comments
Closed

crypto/rsa: invalid RIPEMD160 hash prefix #35495

odeke-em opened this issue Nov 10, 2019 · 4 comments

Comments

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Nov 10, 2019

I am coming here from @ragansa's CL https://go-review.googlesource.com/c/go/+/203480/ and filing this issue so as to fast track that CL as it was submitted before the Go1.14 freeze start.

The issue is that we've got

// These are ASN1 DER structures:
// DigestInfo ::= SEQUENCE {
// digestAlgorithm AlgorithmIdentifier,
// digest OCTET STRING
// }
// For performance, we don't use the generic ASN1 encoder. Rather, we
// precompute a prefix of the digest value that makes a valid ASN1 DER string
// with the correct contents.
var hashPrefixes = map[crypto.Hash][]byte{
crypto.MD5: {0x30, 0x20, 0x30, 0x0c, 0x06, 0x08, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x02, 0x05, 0x05, 0x00, 0x04, 0x10},
crypto.SHA1: {0x30, 0x21, 0x30, 0x09, 0x06, 0x05, 0x2b, 0x0e, 0x03, 0x02, 0x1a, 0x05, 0x00, 0x04, 0x14},
crypto.SHA224: {0x30, 0x2d, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x04, 0x05, 0x00, 0x04, 0x1c},
crypto.SHA256: {0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x05, 0x00, 0x04, 0x20},
crypto.SHA384: {0x30, 0x41, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x02, 0x05, 0x00, 0x04, 0x30},
crypto.SHA512: {0x30, 0x51, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03, 0x05, 0x00, 0x04, 0x40},
crypto.MD5SHA1: {}, // A special TLS case which doesn't use an ASN1 prefix.
crypto.RIPEMD160: {0x30, 0x20, 0x30, 0x08, 0x06, 0x06, 0x28, 0xcf, 0x06, 0x03, 0x00, 0x31, 0x04, 0x14},
}

and in particular for RIPEMD160, we've got

crypto.RIPEMD160: {0x30, 0x20, 0x30, 0x08, 0x06, 0x06, 0x28, 0xcf, 0x06, 0x03, 0x00, 0x31, 0x04, 0x14},

but RFC 4880 says https://tools.ietf.org/html/rfc4880#section-5.2.2
Screen Shot 2019-11-10 at 3 12 44 PM
i.e. should be

   crypto.RIPEMD160: {0x30, 0x21, 0x30, 0x09, 0x06, 0x05, 0x2B, 0x24, 0x03, 0x02,
                                     0x01, 0x05, 0x00, 0x04, 0x14},

That code has been in for 9 years but this is the first report. This issue to have any discussions as well as attach it to the CL that spawned this.

Kindly ping our cryptographers @FiloSottile @agl @veorq.

@odeke-em

This comment has been minimized.

Copy link
Member Author

@odeke-em odeke-em commented Nov 10, 2019

This test

// Issue 35495: ensure that the hash prefixes from
// RFC 4880 Section 5.2.2 correctly match what we have.
func TestCheckHashPrefixes(t *testing.T) {
        want := []struct {
                name       string
                hash       crypto.Hash
                wantPrefix []byte
        }{
                {"MD5", crypto.MD5, []byte{0x30, 0x20, 0x30, 0x0C, 0x06, 0x08, 0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x02, 0x05, 0x05, 0x00, 0x04, 0x10}},
                {"SHA1", crypto.SHA1, []byte{0x30, 0x21, 0x30, 0x09, 0x06, 0x05, 0x2b, 0x0E, 0x03, 0x02, 0x1A, 0x05, 0x00, 0x04, 0x14}},
                {"SHA224", crypto.SHA224, []byte{0x30, 0x2d, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x04, 0x05, 0x00, 0x04, 0x1c}},
                {"SHA256", crypto.SHA256, []byte{0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x05, 0x00, 0x04, 0x20}},
                {"SHA384", crypto.SHA384, []byte{0x30, 0x41, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x02, 0x05, 0x00, 0x04, 0x30}},
                {"SHA512", crypto.SHA512, []byte{0x30, 0x51, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03, 0x05, 0x00, 0x04, 0x40}},
                {"RIPEMD-160", crypto.RIPEMD160, []byte{0x30, 0x21, 0x30, 0x09, 0x06, 0x05, 0x2B, 0x24, 0x03, 0x02, 0x01, 0x05, 0x00, 0x04, 0x14}},
        }

        for _, tt := range want {
                t.Run(tt.name, func(t *testing.T) {
                        gotPrefix := hashPrefixes[tt.hash]
                        if g, w := gotPrefix, tt.wantPrefix; !bytes.Equal(g, w) {
                                t.Fatalf("Hash prefix mismatch:\nGot:  % x\nWant: % x", g, w)
                        }
                })
        }
}

when added to pkcs1v15_test.go prints out

$ go test -run=HashPrefixes
--- FAIL: TestHashPrefixes (0.00s)
    --- FAIL: TestHashPrefixes/RIPEMD-160 (0.00s)
        pkcs1v15_test.go:321: Hash prefix mismatch:
            Got:  30 20 30 08 06 06 28 cf 06 03 00 31 04 14
            Want: 30 21 30 09 06 05 2b 24 03 02 01 05 00 04 14
FAIL
exit status 1
FAIL	crypto/rsa	0.006s
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 11, 2019

Thank you for opening an issue. I don't disagree we have a prefix that's different from the OpenPGP RFC, but I would want to understand why, and why no one noticed so far.

Also, an OpenPGP RFC is not exactly the authoritative source for RSA PKCS#1 v1.5. For example, they got the SHA-224 one wrong. https://www.rfc-editor.org/errata/eid2270 (Why do you have the correct one in your test?)

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 11, 2019

Looks like they are both RIPEMD-160 object identifiers.

This is the one we have

SEQUENCE (2 elem)
  SEQUENCE (1 elem)
    OBJECT IDENTIFIER 1.0.10118.3.0.49 ripemd160 (ISO 10118-3 hash function)
  OCTET STRING BBBBBBBBBBBBBBBBBBBB

and this is the OpenPGP one (which for some reason has a NULL in the prefix as well)

SEQUENCE (2 elem)
  SEQUENCE (2 elem)
    OBJECT IDENTIFIER 1.3.36.3.2.1 ripemd160 (Teletrust hash algorithm)
    NULL
  OCTET STRING BBBBBBBBBBBBBBBBBBBB

Since ours is the ISO one, and it has served us until now, I don't think we should change it.

@ivmaykov

This comment has been minimized.

Copy link

@ivmaykov ivmaykov commented Nov 21, 2019

@FiloSottile there may be two different OIDs for RIPEMD-160. However, the OID specified in RFC4880 (the Teletrust one) is the same one that's used by OpenSSL when creating RSASSA-PKCS1-v1_5 signatures with RIPEMD-160 as the digest algorithm. So, I believe that using the ISO OID will generate signatures that are not compatible with OpenSSL. The Teletrust OID is also used by at least one large HSM vendor (nCipher).

RIPEMD-160 is not mentioned in https://tools.ietf.org/html/rfc8017 so it's unclear what OID should be used, but OpenSSL is very widely deployed so it might be worth it to aim for compatibility with it. It's not a commonly-used hash algorithm so maybe it doesn't matter that much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.