-
Notifications
You must be signed in to change notification settings - Fork 942
common/wireaddr: Fix an out-of-bounds bug in the address parser #8325
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
base: master
Are you sure you want to change the base?
Conversation
Changelog-Fixed: In `struct wireaddr`, the `addr` buffer is defined with a length of DNS_ADDRLEN (255). When parsing a valid DNS name that is exactly 255 bytes long, the subsequent attempt to append a `NULL` terminator overruns the buffer and triggers an out-of-bounds error under UBSan. Fix this by removing the line that appends `NULL`. This change is safe because the preceding call to: `memset(&addr->addr, 0, sizeof(addr->addr))` already zeroes the entire buffer.
This bug was discovered through the new fuzz test in #8324. |
Note that this bug is triggerable from outside |
common/test/run-wireaddr.c
Outdated
@@ -287,6 +224,31 @@ int main(int argc, char *argv[]) | |||
expect->u.unresolved.port = 1234; | |||
assert(wireaddr_internal_eq(&addr, expect)); | |||
|
|||
const char raw_input[] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add a comment describing the test, and also the bug that was fixed.
common/test/run-wireaddr.c
Outdated
const u8 *crashing_input = tal_dup_arr(tmpctx, u8, (const u8*) raw_input, sizeof(raw_input) - 1, 0); | ||
size_t crashing_input_len = tal_bytelen(crashing_input); | ||
|
||
struct wireaddr_internal decoded_wa; | ||
assert(fromwire_wireaddr_internal(&crashing_input, &crashing_input_len, &decoded_wa)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we improve the test to check the expected decoded values, as with the above tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the readability could also use some improvements.
Add a test in `common/test/run-wireaddr.c` that reproduces the out-of-bounds error when the fix is not applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK c8000f2.
In
struct wireaddr
, theaddr
buffer is defined with a length ofDNS_ADDRLEN
(255). When parsing a valid DNS name that is exactly 255 bytes long, the subsequent attempt to append aNULL
terminator overruns the buffer and triggers an out-of-bounds error under UBSan.Fix this bug and add a test to guard against it.
Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:
CC: @morehouse