Stack buffer overflow in ipptool with_value() for long octetString WITH-VALUE

[Tomer-PL]

Summary

In tools/ipptool.c:7654-7656, the with_value() function copies a literal string into withdata[1023] via memcpy with no length check. The hex-encoded path (line 7616) correctly validates length, but the literal string path does not. A .test file with a WITH-VALUE longer than 1023 bytes overflows the stack buffer by up to ~1024 bytes.

Details

// tools/ipptool.c:7608
unsigned char withdata[1023];

// Line 7654-7656: literal string path -- NO bounds check
withlen = strlen(value);
memcpy(withdata, value, (size_t)withlen);  // OVERFLOW if value > 1023

// Compare with hex path at line 7616: HAS bounds check
if (withlen > (int)(2 * (sizeof(withdata) + 1)))
    // ... error handling

Reproducer

Create a .test file:

{
    OPERATION Get-Printer-Attributes
    GROUP operation-attributes-tag
    ATTR charset attributes-charset utf-8
    ATTR naturalLanguage attributes-natural-language en
    ATTR uri printer-uri $uri
    EXPECT job-password-encryption OF-TYPE octetString
    WITH-VALUE "AAAA...2048 A's...AAAA"
}

gcc -g -fsanitize=address -I. -L./cups -Wl,-rpath,./cups \
    -o ipptool_asan tools/ipptool.c -lcups
./ipptool_asan -v http://localhost:631/ /tmp/overflow.test

ASan output:

ERROR: AddressSanitizer: stack-buffer-overflow
WRITE of size 2047 overflowing 1023-byte withdata variable

Suggested Fix

  withlen = strlen(value);
+ if (withlen > (int)sizeof(withdata))
+ {
+     print_fatal_error(data, "WITH-VALUE too long for octetString comparison.");
+     return (false);
+ }
  memcpy(withdata, value, (size_t)withlen);

[ValdikSS]

Patches welcome.

[Tomer-PL]

cool. will open a PR with all these suggestions then :)

[michaelrsweet]

You don't need to do a PR for a simple fix like this (and FWIW I would probably refactor the proposed change anyways...)

[michaelrsweet]

... and looking at the code, ipptool is limiting the WITH-VALUE values to 1024 bytes (including nul) so I don't see how you are getting it to accept 2048 byte values - that should yield an error on any current version of CUPS.

What version did you test this on?

[Tomer-PL]

oops, you are right. the original report misidentified the overflow location. The real bug is a 1-byte stack overflow in _ippFileReadToken when the comma-concatenation loop in token_cb passes a nearly-exhausted buffer (size 1) after reading a 1023-char quoted string. It's already fixed in latest via the tokensize < 32 guard. The with_value()/withdata path is a separate defense-in-depth concern, not the actual trigger.

[michaelrsweet]

[2.4.x 8a70a7b] Add buffer size check from CUPS 2.5.x to _ippFileReadToken (Issue #1542)