Point Decompression does not handle invalid byte lengths
kirk-baird opened this issue · 3 comments
What is the issue?
The rust bindings uncompress() and deserialize() for each point object do not ensure the correct number of bytes are supplied before calling.
This is an issue if someone calls uncompress(&[]) with an empty array. The c code assumes it was passed a 48 byte array and will read the next sections of memory.
What needs to be done?
I think this will have to be checked in the rust uncompress() / deserialize() functions.
For uncompress() maybe something along the lines of pk_comp.len() == $pk_comp_size.
It's generally safe to do a strict check on length to prevent signature malleability by appending bytes.
A little more challenging for deserialize() as we have to handle the compressed and uncompressed byte lengths. But a check of the compression bit should do the trick.
Oh! It closed itself upon mention in commit message. A bit unexpected, but all right. Fix is committed. Thanks for the report.
It looks like in the update you can have a compressed public key which is 96 bytes and the last 48 bytes will be ignored by the function. That is set pk_in[0] & 0x80 == 0x80, pk_in[0..48] as a valid compressed public key and pk_in[48..] to any arbitrary bytes. The C code would interpret this as a compressed point and only read the first 48 bytes.
if pk_in.len() == $pk_ser_size
|| pk_in.len() == $pk_comp_size && (pk_in[0] & 0x80) != 0There is no chance of an index out of bounds access here. I do think it's worth enforcing the byte lengths exactly match the required number of bytes as it would help enforce injectiveness.
if pk_in.len() == $pk_ser_size && (pk_in[0] & 0x80) == 0
|| pk_in.len() == $pk_comp_size && (pk_in[0] & 0x80) != 0Similarly for Signatures.