Subject: Fix ABI breakage introduced by accident in 2.8.1-1+deb10u1 Author: Hilko Bengen Date: 2021-10-22 Bug-Debian: https://bugs.debian.org/996460 https://bugs.debian.org/996939 https://bugs.debian.org/996997 Comment: (by the http-parser maintainer) # Problem The fix for CVE-2019-15605 introduced an ABI break by changing the layout of struct http_parser - a change that was needed to store a ninth bit in the "flags" field. However, this affected offsets of fields declared as public, causing applications to break. # Workaround To restore the previous layout while still implementing the fix: Steal one bit from the (private) content_length field (the remaining 63 are more than enough) to store the newly introduced flag. Also rename the related constant as a safeguard against applications that use it (they must not, see below). # Possible regressions A lot of work was done to avoid damage for well-behaving applications. It seems all applications in Debian built against http-parser fall into that category. Applications however that access fields in struct http_parser that are in the section marked "/** PRIVATE **/" may face issues. Such a behaviour is inacceptable anyway. If such a mis-behaving application ... * was built using an earlier version of http-parser, the code will assume content_length is a 64 bit value. Depending on endianess and status of the F_TRANSFER_ENCODING bit, things may work. Possibly they will not. * uses the private F_TRANSFER_ENCODING constant and was built using http-parser 2.8.1-1+deb10u1, it will not see the information it expects to see. Additionally, and re-build will fail. This is by design. Again, applications must not access fields declared private, and their authors should not expect pity if they encounter breakage any anything changes there. # Acknowledgments Thanks a lot to Hilko Bengen for the idea, implementation, a first round of tests and many suggestions. --- a/http_parser.c +++ b/http_parser.c @@ -25,9 +25,7 @@ #include #include -#ifndef ULLONG_MAX -# define ULLONG_MAX ((uint64_t) -1) /* 2^64-1 */ -#endif +#define CONTENT_LENGTH_MAX (((uint64_t)-1) >> 1) #ifndef MIN # define MIN(a,b) ((a) < (b) ? (a) : (b)) @@ -724,7 +722,8 @@ if (ch == CR || ch == LF) break; parser->flags = 0; - parser->content_length = ULLONG_MAX; + parser->flags2 = 0; + parser->content_length = CONTENT_LENGTH_MAX; if (ch == 'H') { UPDATE_STATE(s_res_or_resp_H); @@ -759,7 +758,8 @@ case s_start_res: { parser->flags = 0; - parser->content_length = ULLONG_MAX; + parser->flags2 = 0; + parser->content_length = CONTENT_LENGTH_MAX; switch (ch) { case 'H': @@ -923,7 +923,8 @@ if (ch == CR || ch == LF) break; parser->flags = 0; - parser->content_length = ULLONG_MAX; + parser->flags2 = 0; + parser->content_length = CONTENT_LENGTH_MAX; if (UNLIKELY(!IS_ALPHA(ch))) { SET_ERRNO(HPE_INVALID_METHOD); @@ -1314,7 +1315,7 @@ parser->header_state = h_general; } else if (parser->index == sizeof(TRANSFER_ENCODING)-2) { parser->header_state = h_transfer_encoding; - parser->flags |= F_TRANSFER_ENCODING; + parser->flags2 |= F_TRANSFER_ENCODING2; } break; @@ -1528,7 +1529,7 @@ t += ch - '0'; /* Overflow? Test against a conservative limit for simplicity. */ - if (UNLIKELY((ULLONG_MAX - 10) / 10 < parser->content_length)) { + if (UNLIKELY((CONTENT_LENGTH_MAX - 10) / 10 < parser->content_length)) { SET_ERRNO(HPE_INVALID_CONTENT_LENGTH); parser->header_state = h_state; goto error; @@ -1765,7 +1766,7 @@ /* Cannot us transfer-encoding and a content-length header together per the HTTP specification. (RFC 7230 Section 3.3.3) */ - if ((parser->flags & F_TRANSFER_ENCODING) && + if ((parser->flags2 & F_TRANSFER_ENCODING2) && (parser->flags & F_CONTENTLENGTH)) { /* Allow it for lenient parsing as long as `Transfer-Encoding` is * not `chunked` @@ -1834,7 +1835,7 @@ parser->nread = 0; hasBody = parser->flags & F_CHUNKED || - (parser->content_length > 0 && parser->content_length != ULLONG_MAX); + (parser->content_length > 0 && parser->content_length != CONTENT_LENGTH_MAX); if (parser->upgrade && (parser->method == HTTP_CONNECT || (parser->flags & F_SKIPBODY) || !hasBody)) { /* Exit, the rest of the message is in a different protocol. */ @@ -1850,7 +1851,7 @@ /* chunked encoding - ignore Content-Length header, * prepare for a chunk */ UPDATE_STATE(s_chunk_size_start); - } else if (parser->flags & F_TRANSFER_ENCODING) { + } else if (parser->flags2 & F_TRANSFER_ENCODING2) { if (parser->type == HTTP_REQUEST && !lenient) { /* RFC 7230 3.3.3 */ @@ -1877,7 +1878,7 @@ /* Content-Length header given but zero: Content-Length: 0\r\n */ UPDATE_STATE(NEW_MESSAGE()); CALLBACK_NOTIFY(message_complete); - } else if (parser->content_length != ULLONG_MAX) { + } else if (parser->content_length != CONTENT_LENGTH_MAX) { /* Content-Length header given and non-zero */ UPDATE_STATE(s_body_identity); } else { @@ -1901,7 +1902,7 @@ (uint64_t) ((data + len) - p)); assert(parser->content_length != 0 - && parser->content_length != ULLONG_MAX); + && parser->content_length != CONTENT_LENGTH_MAX); /* The difference between advancing content_length and p is because * the latter will automaticaly advance on the next loop iteration. @@ -1991,7 +1992,7 @@ t += unhex_val; /* Overflow? Test against a conservative limit for simplicity. */ - if (UNLIKELY((ULLONG_MAX - 16) / 16 < parser->content_length)) { + if (UNLIKELY((CONTENT_LENGTH_MAX - 16) / 16 < parser->content_length)) { SET_ERRNO(HPE_INVALID_CONTENT_LENGTH); goto error; } @@ -2035,7 +2036,7 @@ assert(parser->flags & F_CHUNKED); assert(parser->content_length != 0 - && parser->content_length != ULLONG_MAX); + && parser->content_length != CONTENT_LENGTH_MAX); /* See the explanation in s_body_identity for why the content * length and data pointers are managed this way. @@ -2124,12 +2125,12 @@ } /* RFC 7230 3.3.3, see `s_headers_almost_done` */ - if ((parser->flags & F_TRANSFER_ENCODING) && + if ((parser->flags2 & F_TRANSFER_ENCODING2) && (parser->flags & F_CHUNKED) == 0) { return 1; } - if ((parser->flags & F_CHUNKED) || parser->content_length != ULLONG_MAX) { + if ((parser->flags & F_CHUNKED) || parser->content_length != CONTENT_LENGTH_MAX) { return 0; } --- a/http_parser.h +++ b/http_parser.h @@ -225,7 +225,7 @@ , F_UPGRADE = 1 << 5 , F_SKIPBODY = 1 << 6 , F_CONTENTLENGTH = 1 << 7 - , F_TRANSFER_ENCODING = 1 << 8 + , F_TRANSFER_ENCODING2 = 1 << 0 /* this goes into http_parser.flags2 */ }; @@ -296,14 +296,15 @@ struct http_parser { /** PRIVATE **/ unsigned int type : 2; /* enum http_parser_type */ + unsigned int flags : 8; /* F_* values from 'flags' enum; semi-public */ unsigned int state : 7; /* enum state from http_parser.c */ unsigned int header_state : 7; /* enum header_state from http_parser.c */ unsigned int index : 7; /* index into current matcher */ unsigned int lenient_http_headers : 1; - unsigned int flags : 16; /* F_* values from 'flags' enum; semi-public */ uint32_t nread; /* # bytes read in various scenarios */ - uint64_t content_length; /* # bytes in body (0 if no Content-Length header) */ + uint64_t flags2 : 1; /* one bit another flag */ + uint64_t content_length : 63; /* # bytes in body (0 if no Content-Length header) */ /** READ-ONLY **/ unsigned short http_major;