Support verification of non-canonical BER encoded ECDSA signatures #408
Description
Description
Recently I debugged why a small % of our webauthn signatures were failing to verify and traced it down to non-canonical BER encoding for the (r, s) components of some ECDSA signatures out there.
This is because go-webauthn uses Golang's asn parser, which doesn't allow non-canonical BER encoding anymore.
I'll note that non-canonical BER is generally a bug: there was some interesting discussion in w3c/webauthn#799, and as a result the webauthn spec now says:
the sig value MUST be encoded as an ASN.1 DER Ecdsa-Sig-Value
(https://www.w3.org/TR/webauthn/#sctn-signature-attestation-types)
Nonetheless, I've found devices in the wild which do not produce DER-encoded signatures. Let's support them if possible?
Reproduction
Example of a signature from the wild (caught in production logs)
MEUCIABUG1NYYRFhRNbIdD17nvA1c52-hs6186EPDQfpC5vOAiEA677L3zMwkKmM2rBC2vHyaB5W5wDNMaD5mQ4XAheobj8
In hex, formatted for clarity:
// SEQUENCE tag (len: 0x45=69)
30 45
// INTEGER tag (len: 0x20=32)
02 20
// content of "r"
00541b535861116144d6c8743d7b9ef035739dbe86ceb5f3a10f0d07e90b9bce
// INTEGER tag (len: 0x21=33)
0221
// content of "s"
00ebbecbdf333090a98cdab042daf1f2681e56e700cd31a0f9990e170217a86e3f
Note the extra "00" at the start of "r": this isn't canonical DER (on the other hand: the "00" at the start of "s" is, because the next byte, "eb" is >128, so we need this null byte to indicate that we're dealing with a positive integer). This is just one example: there are other examples with needless null bytes for "s" or for both components.
Anyway, if you plug this value in webauthncose.VerifySignature you'll get an error: Signature invalid or not provided, which comes from the failure to unmarshal as asn.1:
webauthn/protocol/webauthncose/webauthncose.go
Lines 102 to 105 in ae44c20
Expectations
I'd like to explore making go-webauthn capable of handling these signatures. They're in the wild, they'll continue to be in the wild, and users do not have a choice to produce canonical DER if their device/browser doesn't.
I can make a PR if you agree it'd be valuable.
Documentation
I have a workaround for ECDSA signature which I'm not proud of, but works: it carefully parses the signature and trims zeros off "r" and "s" as needed.
// Normalize signature bytes (r and s components) to make them canonical DER
func NormalizeSignatureBytes(signatureBytes []byte) ([]byte, error) {
// We need at least 8 bytes to trim bytes off without crashing:
// - sequence tag
// - sequence length
// - integer tag
// - integer length
// - at least one byte for the actual int, which we'll trim
// - integer tag for s
// - integer length for s
// - at least one byte for the actual int, which we'll trim
minSignatureLen := 8
if len(signatureBytes) < minSignatureLen {
return []byte{}, fmt.Errorf("signature bytes are %d bytes long, but expected %d minimum", len(signatureBytes), minSignatureLen)
}
if signatureBytes[0] != 0x30 || signatureBytes[2] != 0x02 {
return nil, errors.New("signature bytes do not start with 0x30__02. Is this an ECDSA signature?")
}
type ECDSASignature struct {
R, S *big.Int
}
var ecdsaSig ECDSASignature
// Try parsing the signature bytes with asn1
_, err := asn1.Unmarshal(signatureBytes, &ecdsaSig)
if err == nil {
// If no parsing error, no need to normalize the signature, we're good!
return signatureBytes, nil
} else if err.Error() == "asn1: structure error: integer not minimally-encoded" {
// If we have this specific error from asn1, we have some fixing to do.
// Example of a signature captured from the wild: below you see a sequence of 2 integers.
// These are the "r" and "s" components of our signature.
// 30 -> SEQUENCE tag
// 45 -> length of the sequence (0x45 -> 69 bytes)
// 02 -> INTEGER tag
// 20 -> length of the integer in bytes (0x20 -> 32 bytes)
// 00541b535861116144d6c8743d7b9ef035739dbe86ceb5f3a10f0d07e90b9bce
// 02 -> INTEGER tag
// 21 -> length of the integer in bytes (0x21 -> 33 bytes)
// 00ebbecbdf333090a98cdab042daf1f2681e56e700cd31a0f9990e170217a86e3f
rStart := 4
rLen := int(signatureBytes[3])
// We expect another 0x02 after r stops (this is the INTEGER tag for "s")
if signatureBytes[rStart+rLen] != 0x02 {
return nil, fmt.Errorf("signature does not have an INTEGER tag after r component. Expected 0x02 at index %d", rStart+rLen)
}
sStart := rStart + rLen + 2
sLen := int(signatureBytes[rStart+rLen+1])
// Trim leading zeros if any
newR := trimLeadingZeros(signatureBytes[rStart : rStart+rLen])
newS := trimLeadingZeros(signatureBytes[sStart : sStart+sLen])
// Yes this code could be a lot more terse, but the important thing is for it to be readable.
newSig := []byte{0x30} // SEQUENCE tag
newSig = append(newSig, byte(2+len(newR)+2+len(newS))) // length byte for sequence (includes r, s, and their tags)
newSig = append(newSig, 0x02) // INTEGER tag
newSig = append(newSig, byte(len(newR))) // length byte for r
newSig = append(newSig, newR...) // value for r
newSig = append(newSig, 0x02) // INTEGER tag
newSig = append(newSig, byte(len(newS))) // length byte for s
newSig = append(newSig, newS...) // value for s
return newSig, nil
} else {
// If the error is something else, simply bubble it up!
return []byte{}, err
}
}
// Remove all leading 0x00 bytes unless the result would be interpreted as negative
func trimLeadingZeros(b []byte) []byte {
// In DER, if the most significant bit (MSB) of the first byte is 1, the integer will be interpreted as negative.
// To prevent positive integers from being misinterpreted as negative, a 0x00 byte is prepended when the MSB of the first actual value byte is 1.
// 0x80 is 128, or 0b10000000. If the second byte is < 0x80, we're good to trim. Otherwise, the null byte is there for a good reason!
for len(b) > 1 && b[0] == 0x00 && b[1] < 0x80 {
b = b[1:]
}
return b
}