3
4
Fork
You've already forked validns
5

fix: bound copies in verbose (-v) rdata rendering #140

Merged
jelu merged 8 commits from tobez/validns:fix-verbose-render-overflows into main 2026年07月08日 11:55:16 +02:00
Collaborator
Copy link

Four -v rdata renderers could overflow a fixed stack buffer from one parseable record, before any validation:

  • RRSIG: oversized ECDSA signature written below a 96-byte array — now length-checked at parse (64/96, RFC 6605).
  • IPSECKEY: type-3 gateway name strcpy'd into a 1024-byte buffer — now rendered without a fixed copy.
  • NSEC/NSEC3: type-bitmap cursor ran past its buffer, underflowing the remaining length — now clamped per site, as in SVCB/NSEC3PARAM/TXT.

Well-formed output unchanged. ASan fixtures added; passes on macOS, Rocky 9, Ubuntu, Debian (incl. i386) and FreeBSD; scan-build clean.

Four `-v` rdata renderers could overflow a fixed stack buffer from one parseable record, before any validation: - **RRSIG:** oversized ECDSA signature written below a 96-byte array — now length-checked at parse (64/96, RFC 6605). - **IPSECKEY:** type-3 gateway name `strcpy`'d into a 1024-byte buffer — now rendered without a fixed copy. - **NSEC/NSEC3:** type-bitmap cursor ran past its buffer, underflowing the remaining length — now clamped per site, as in SVCB/NSEC3PARAM/TXT. Well-formed output unchanged. ASan fixtures added; passes on macOS, Rocky 9, Ubuntu, Debian (incl. i386) and FreeBSD; scan-build clean.
fix: drop dead store in nsec3param salt-loop clamp
Some checks failed
test / debian (pull_request) Failing after 1s
test / centos (pull_request) Failing after 1s
test / ubuntu (pull_request) Failing after 1s
test / scan-build (pull_request) Failing after 1s
builds.sr.ht/freebsd Job completed
builds.sr.ht/openbsd Job completed
3f103998d9
jelu left a comment
Copy link

You seem to treat all failure of snprintf as "truncated string", I don't think that's good

NOTE: There are more places where return of snprintf is not checked, just +='ed

You seem to treat all failure of `snprintf` as "truncated string", I don't think that's good NOTE: There are more places where return of `snprintf` is not checked, just `+=`'ed
src/nsec3.c Outdated
@ -131,1 +135,3 @@
l = snprintf(s, 1024 - (s - ss), "%02X", (unsigned char)rr->salt.data[i]);
size_t rem = 1024 - (s - ss);
l = snprintf(s, rem, "%02X", (unsigned char)rr->salt.data[i]);
if (l < 0 || (size_t)l >= rem) { s = ss + 1023; break; }
Owner
Copy link

you're jumping forward 1023 chars here if snprintf fails, that's probably not correct

you're jumping forward 1023 chars here if `snprintf` fails, that's probably not correct
src/nsec3.c Outdated
@ -135,2 +141,2 @@
l = snprintf(s, 1024 - (s - ss), "-");
s += l;
size_t rem = 1024 - (s - ss);
l = snprintf(s, rem, "-");
Owner
Copy link

shouldn't it return NULL on failure?

shouldn't it `return NULL` on failure?
jelu marked this conversation as resolved
@ -146,2 +155,2 @@
l = snprintf(s, 1024 - (s - ss), " %s", hash);
s += l;
rem = 1024 - (s - ss);
l = snprintf(s, rem, " %s", hash);
Owner
Copy link

shouldn't it return NULL on failure?

shouldn't it `return NULL` on failure?
jelu marked this conversation as resolved
fix: render no rdata instead of truncated rdata in verbose output
All checks were successful
builds.sr.ht/freebsd Job completed
builds.sr.ht/openbsd Job completed
test / ubuntu (pull_request) Successful in 36s
test / debian (pull_request) Successful in 37s
test / scan-build (pull_request) Successful in 1m10s
test / centos (pull_request) Successful in 1m18s
2de362e859
Author
Collaborator
Copy link

Thank you for your feedback. I believe that all your points have been addressed in the latest commit.

Thank you for your feedback. I believe that all your points have been addressed in the latest commit.
Author
Collaborator
Copy link

@tobez wrote in #140 (comment):

Thank you for your feedback. I believe that all your points have been addressed in the latest commit.

@jelu

@tobez wrote in https://codeberg.org/DNS-OARC/validns/pulls/140#issuecomment-18788840: > Thank you for your feedback. I believe that all your points have been addressed in the latest commit. @jelu
Owner
Copy link

#build

#build
Owner
Copy link

@tobez I would recommend changing all the static buffer sizes to use sizeof(ss), but no need to add that into this PR

@tobez I would recommend changing all the static buffer sizes to use `sizeof(ss)`, but no need to add that into this PR
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
DNS-OARC/validns!140
Reference in a new issue
DNS-OARC/validns
No description provided.
Delete branch "tobez/validns:fix-verbose-render-overflows"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?