fix(bindings): validate secret key hex length - #487
Conversation
|
|
||
| var hex_buf: [NativeSecretKey.serialize_size * 2 + 3]u8 = undefined; | ||
| const hex = try hexFromString(hex_string, &hex_buf); | ||
| if (hex.len != NativeSecretKey.serialize_size * 2) { |
There was a problem hiding this comment.
this second check is ensuring that a non-0x-prefixed hex string thats the wrong length doesn't slip through
|
|
||
| var hex_buf: [NativeSecretKey.serialize_size * 2 + 3]u8 = undefined; | ||
| const hex = try hexFromString(hex_string, &hex_buf); | ||
| if (hex.len != NativeSecretKey.serialize_size * 2) { |
There was a problem hiding this comment.
this is more suitable to be an assert, since if this check fails it means something is wrong with hexFromString and not the consumer input
program logic error = assert
input error = throw error
semes like a good practice to go by
There was a problem hiding this comment.
- "0x" + 31 bytes has length 64, but becomes 62 after removing the prefix.
- An unprefixed 33-byte value has length 66 and remains 66.
Both pass the first check, so this should remain an input error rather than an assert.
There was a problem hiding this comment.
you're right, doesn't that mean hexFromString is misleading though because it accepts any string and returns it unchanged if it's not 0x prefixed
|
seems like your original commits are not signed @GrapeBaBa |
|
Superseded by #517, recreated from the latest |
## Summary - validate `SecretKey.fromHex` input length before copying into its fixed-size buffer - require exactly 32 decoded bytes after stripping an optional `0x` prefix - cover prefixed and unprefixed valid keys, short and long inputs, odd-length hex, and non-ASCII input ## Why Short hex strings previously reached a 32-byte slice operation without a length check, causing a ReleaseSafe bounds panic that aborted the Node process instead of throwing a JavaScript error. Checking the UTF-8 byte length before copying also prevents overlong strings from being silently truncated by N-API. Supersedes #487, which contains unsigned commits.
4 participants
Summary
Why
Short hex strings previously reached a 32-byte slice operation without a length check, causing a ReleaseSafe bounds panic that aborted the Node process instead of throwing a JavaScript error. Checking the UTF-8 byte length before copying also prevents overlong strings from being silently truncated by N-API.