Skip to content

bound destination writes in win32_utf8cpy - #1659

Open
aizu-m wants to merge 1 commit into
OpenPrinting:masterfrom
aizu-m:dnssd-utf8cpy-bounds
Open

bound destination writes in win32_utf8cpy#1659
aizu-m wants to merge 1 commit into
OpenPrinting:masterfrom
aizu-m:dnssd-utf8cpy-bounds

Conversation

@aizu-m

@aizu-m aizu-m commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Auditing the DNS-SD backends. win32_utf8cpy takes a dstsize but never touches it, so while (*src && dstsize > 4) is a one-time check that the buffer started out bigger than 4 bytes, not a running bound. The loop then stops only at the source nul.

Lifted the function out and gave it a 599-character name against a 256-byte buffer:

==52960==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x611000000140
WRITE of size 1 at 0x611000000140 thread T0
    #0 0x0001008f4e10 in win32_utf8cpy dnssd.c:2625
0x611000000140 is located 0 bytes after 256-byte region [0x611000000040,0x611000000140)

All six callers pass char[256] stack buffers holding strings that came off the wire. win32_browse_cb copies the PTR target of a browse reply into fullname[256]; win32_resolve_cb copies the instance name, the hostname, and every TXT key and value. A responder answering an _ipp._tcp.local browse with a long name walks straight past the buffer.

win32_wstrcpy directly below keeps its own counter, and cupsDNSSDSeparateFullName guards each store with ptr < end, so the shape was already there. Bounded each encoding branch against dstend the same way. In-range output is byte-identical and the full 255 bytes are still used.

@michaelrsweet michaelrsweet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably intended to decrement dstsize, but using an end pointer is more in keeping with our usual coding style.

I would simply initialize dstend to "dst + dstsize - 5" to eliminate the extra checks for capacity. The only callers will be providing a buffer larger than 5 bytes.

@michaelrsweet michaelrsweet self-assigned this Aug 3, 2026
@michaelrsweet michaelrsweet added bug Something isn't working platform issue Issue is specific to an OS or desktop labels Aug 3, 2026
@michaelrsweet michaelrsweet added this to the v2.5 milestone Aug 3, 2026
Signed-off-by: Aizal Khan <aizumusheer2@gmail.com>
@aizu-m
aizu-m force-pushed the dnssd-utf8cpy-bounds branch from 906c879 to 69c40f0 Compare August 4, 2026 18:21
@aizu-m

aizu-m commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Done. dstend is dst + dstsize - 5 now and the three per-branch checks are gone, so the whole patch is down to two lines.

One thing I noticed re-running the harness: the fixed 5-byte slack means a pure-ASCII name truncates at 252 bytes rather than 255. Output is byte-identical below that, and the overflow is gone either way, so it seems the right trade for dropping the checks.

ASan on the extracted function is clean at 599 chars into a 256-byte buffer. make unittests and testdnssd both still pass.

@michaelrsweet

Copy link
Copy Markdown
Member

When converting many of the DNS-SD values, some have a hard limit of 63 bytes (size of one DNS label), others might be up to 4 labels (252 bytes) in length, so we are good with that limit... :)

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working platform issue Issue is specific to an OS or desktop

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants