search

cloudflare / circl

CIRCL: Cloudflare Interoperable Reusable Cryptographic Library
http://blog.cloudflare.com/introducing-circl
Other
1.3k stars 144 forks source link

hpke: slice bounds out of range in shortKEM.UnmarshalBinaryPrivateKey #488

Open emersion opened 7 months ago

emersion commented 7 months ago

When passing a byte slice of size > 66 bytes to shortKEM.UnmarshalBinaryPrivateKey when using the DHKEM(P-521, HKDF-SHA512) KEM, the following panic is triggered:

panic: runtime error: slice bounds out of range [-1:]

goroutine 3014 [running]:
panic({0x78c980?, 0xc00050a090?})
    /usr/lib/go/src/runtime/panic.go:770 +0x132
github.com/cloudflare/circl/hpke.shortKEM.UnmarshalBinaryPrivateKey({{{0x12, {0x7aa461, 0x19}, 0x7}, {0x820178, 0xc000016240}}, {0x81fe98, 0x9ad8e0}}, {0xc0002800a0, 0x43, ...})
    /home/simon/src/circl/hpke/shortkem.go:87 +0x35b
armfazh commented 6 months ago

This is a bug, the code shouldn't panic, but instead it should return an error. in #489 , code enforces to pass slices of the exact size for unmarshaling keys.

bwesterb commented 6 months ago

Not sure whether it's a bug to panic when the caller breaks the function contract. Not opposed to returning an error instead though.

emersion commented 6 months ago

I don't see where this function contract is defined? Nothing in https://pkg.go.dev/github.com/cloudflare/circl@v1.3.8/kem#Scheme

IMHO, panic'ing would be OK if it was documented and if it used an explicit check with a proper error message (instead of an out of bounds error).

However, please note that other functions with documented panics don't return an error (so don't really have a choice). UnmarshalBinaryPrivateKey does return an error. Moreover, "Unmarshal" is a kind of parsing step, and parsing deals with user-provided input. Panic'ing on bad user-provided input (such as fed from a network protocol) isn't super nice.