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

Jump to bottom

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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@odeke-em
Copy link
Member

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

go/src/crypto/rsa/pkcs1v15.go

Lines 200 to 217 in 47bc240

// 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

go/src/crypto/rsa/pkcs1v15.go

Line 216 in 47bc240

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 odeke-em added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 10, 2019
@odeke-em
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

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.

@gopherbot gopherbot mentioned this issue Nov 11, 2019
Closed
@ivmaykov
Copy link

ivmaykov commented Nov 21, 2019 •
edited

@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.

@golang golang locked and limited conversation to collaborators Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ivmaykov @FiloSottile @odeke-em @gopherbot