Browse Source

Fix ABI breakage introduced by accident in 2.8.1-1+deb10u1. Closes: #996460, #996939, #996997

Christoph Biedl 2 years ago
parent
commit
823ef38783
2 changed files with 225 additions and 0 deletions
  1. 224 0
      debian/patches/fix-ABI-breakage.patch
  2. 1 0
      debian/patches/series

+ 224 - 0
debian/patches/fix-ABI-breakage.patch

@@ -0,0 +1,224 @@
+Subject: Fix ABI breakage introduced by accident in 2.8.1-1+deb10u1
+Author: Hilko Bengen <bengen@debian.org>
+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 <string.h>
+ #include <limits.h>
+ 
+-#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;

+ 1 - 0
debian/patches/series

@@ -1,3 +1,4 @@
 improve-installation.patch
 
 1580760635.v2.9.2-2-g7d5c99d.support-multi-coding-transfer-encoding.patch
+fix-ABI-breakage.patch