diff --git a/pgp/yubikey.go b/pgp/yubikey.go index 6573724e9d26614d52f017be96a2d4ef5c93b3a1..7b2f02415f10f9c6854f99b588d61970a6609dd9 100644 --- a/pgp/yubikey.go +++ b/pgp/yubikey.go @@ -414,6 +414,10 @@ func writeNewFormatLength(w *bytes.Buffer, length int) { } // parseASN1Signature extracts r and s from an ASN.1 DER encoded ECDSA signature. +// +// Each intermediate slice access is bounds-checked against len(der). A truncated +// or malformed signature produces a typed error rather than an index-out-of-range +// panic; the minimum-length check up front only rules out obvious runts (#613). func parseASN1Signature(der []byte) (r, s []byte, err error) { // ASN.1 SEQUENCE { INTEGER r, INTEGER s } if len(der) < 6 || der[0] != 0x30 { @@ -423,22 +427,34 @@ func parseASN1Signature(der []byte) (r, s []byte, err error) { pos := 2 // skip SEQUENCE tag and length // Parse R - if der[pos] != 0x02 { + if pos >= len(der) || der[pos] != 0x02 { return nil, nil, fmt.Errorf("expected INTEGER tag for R") } pos++ + if pos >= len(der) { + return nil, nil, fmt.Errorf("ASN.1 signature truncated before R length") + } rLen := int(der[pos]) pos++ + if pos+rLen > len(der) { + return nil, nil, fmt.Errorf("ASN.1 signature truncated: R length overflow") + } rVal := new(big.Int).SetBytes(der[pos : pos+rLen]) pos += rLen // Parse S - if der[pos] != 0x02 { + if pos >= len(der) || der[pos] != 0x02 { return nil, nil, fmt.Errorf("expected INTEGER tag for S") } pos++ + if pos >= len(der) { + return nil, nil, fmt.Errorf("ASN.1 signature truncated before S length") + } sLen := int(der[pos]) pos++ + if pos+sLen > len(der) { + return nil, nil, fmt.Errorf("ASN.1 signature truncated: S length overflow") + } sVal := new(big.Int).SetBytes(der[pos : pos+sLen]) return rVal.Bytes(), sVal.Bytes(), nil diff --git a/pgp/yubikey_test.go b/pgp/yubikey_test.go new file mode 100644 index 0000000000000000000000000000000000000000..9666ec6ba6cfe84d2ab213b8504f22feb048765e --- /dev/null +++ b/pgp/yubikey_test.go @@ -0,0 +1,74 @@ +package pgp + +import ( + "strings" + "testing" +) + +// TestParseASN1Signature_TruncatedDoesNotPanic covers the bounds-check path +// added for #613. Each input would have panicked in the original parser +// with "index out of range"; here we expect a typed error instead. +func TestParseASN1Signature_TruncatedDoesNotPanic(t *testing.T) { + cases := []struct { + name string + der []byte + wantErr string + }{ + { + // Length byte declares 0x10 bytes of R but only 1 byte follows. + name: "R length overruns buffer", + der: []byte{0x30, 0x06, 0x02, 0x10, 0xAA, 0x00}, + wantErr: "R length overflow", + }, + { + // Length byte declares 0x10 bytes of S but only 1 byte follows. + name: "S length overruns buffer", + der: []byte{0x30, 0x06, 0x02, 0x01, 0x01, 0x02, 0x10, 0xAA}, + wantErr: "S length overflow", + }, + { + // Valid R, then no S block at all. + name: "missing S after R", + der: []byte{0x30, 0x06, 0x02, 0x01, 0x01, 0x00}, + wantErr: "expected INTEGER tag for S", + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + // The test must not panic: the fix replaces panics with errors. + defer func() { + if r := recover(); r != nil { + t.Fatalf("parseASN1Signature panicked: %v", r) + } + }() + _, _, err := parseASN1Signature(tc.der) + if err == nil { + t.Fatalf("want error, got nil") + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error = %q, want it to mention %q", err.Error(), tc.wantErr) + } + }) + } +} + +// TestParseASN1Signature_WellFormed guards against regressions in the +// happy path: a minimal SEQUENCE { INTEGER, INTEGER } must still decode +// to the original r and s bytes. +func TestParseASN1Signature_WellFormed(t *testing.T) { + // SEQUENCE (6 bytes) { INTEGER 0x01, INTEGER 0x02 } + der := []byte{0x30, 0x06, 0x02, 0x01, 0x01, 0x02, 0x01, 0x02} + + r, s, err := parseASN1Signature(der) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(r) != 1 || r[0] != 0x01 { + t.Errorf("r = %x, want 01", r) + } + if len(s) != 1 || s[0] != 0x02 { + t.Errorf("s = %x, want 02", s) + } +}