Skip to content

fix ippGetLength overcount for value tags 0x7f-0xff - #205

Closed
tanjiroK-coder wants to merge 1 commit into
OpenPrinting:masterfrom
tanjiroK-coder:ipp-length-extension-tags
Closed

tanjiroK-coder wants to merge 1 commit into
OpenPrinting:masterfrom
tanjiroK-coder:ipp-length-extension-tags

Conversation

@tanjiroK-coder

Copy link
Copy Markdown
Contributor

ipp_length, which backs ippGetLength, adds five octets per value for any value tag at or above IPP_TAG_EXTENSION (0x7f), as if every such tag used the 32-bit extension encoding. ippReadIO stores tags 0x7f through 0xff as ordinary one-octet tags (an RFC 8010 0x7f value carries its type code in the first four octets of the value, which libcups keeps as opaque data), and ippWriteIO only writes the five-octet form for tags above 0xff, so each such value makes ippGetLength 4 bytes larger than what ippWriteIO actually sends. cupsSendRequest, cupsDoIORequest and ippeveprinter all use ippGetLength for the HTTP Content-Length, so a request or response carrying one of these values goes out with a Content-Length larger than its body. The tag can come from the peer: an unauthenticated Validate-Job to ippeveprinter with ipp-attribute-fidelity encoded as tag 0x7f is copied back in the unsupported-attributes group, and the reply says Content-Length: 177 but contains 173 bytes, with the gap growing by 4 bytes for each such value. On a keep-alive connection the receiver then waits for, or takes from the next message, bytes that are not part of this one, which desynchronizes HTTP framing for that connection and for anything reusing it, such as a proxy. I found this while comparing ippGetLength with the bytes ippWriteIO produced for fuzzed messages, and the change counts one tag octet for tags up to 0xff so it matches the writer. The new testipp case reads a 0x7f extension value and checks ippGetLength against the encoded size; it reports 30 instead of 26 bytes without the fix and passes with it, and the other cups unit tests and tools/run-tests.sh pass locally on macOS with OpenSSL (apart from testjson's live HTTPS fetch, which fails here for lack of a CA bundle, with or without this change).

Changelog: Fixed ippGetLength reporting 4 extra bytes per value for value tags 0x7f through 0xff.

@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.

OK, there are actually a bunch of issues with the extension tag support in libcups, mostly because they've NEVER be used in almost 30 years of IPP...

I'll use this PR to track a fix for the next release, and I might even include the extension unit test bytes to verify things in testipp. Internally libcups just treats extension tags as octetString values.

@michaelrsweet

Copy link
Copy Markdown
Member

[master be71e7a] Fix issues related to extension tag values (Issue #205)

@tanjiroK-coder

Copy link
Copy Markdown
Contributor Author

thanks for landing this in be71e7a, the testipp bytes are a nice addition.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants