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

Point Decompression does not handle invalid byte lengths #14

Closed
kirk-baird opened this issue Jul 29, 2020 · 3 comments
Closed

Point Decompression does not handle invalid byte lengths #14

kirk-baird opened this issue Jul 29, 2020 · 3 comments

Comments

@kirk-baird
Copy link
Contributor

kirk-baird commented Jul 29, 2020

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.

@dot-asm
Copy link
Collaborator

dot-asm commented Jul 29, 2020

Oh! It closed itself upon mention in commit message. A bit unexpected, but all right. Fix is committed. Thanks for the report.

@kirk-baird
Copy link
Contributor Author

kirk-baird commented Jul 30, 2020

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) != 0

There 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) != 0

Similarly for Signatures.

@dot-asm
Copy link
Collaborator

dot-asm commented Aug 3, 2020

Fixed in fff48fd. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants