Project

General

Profile

Bug #2854

chunked transfer encoding in request body only works for tiny chunks

Added by the_jk 11 months ago. Updated 11 months ago.

Status:
Fixed
Priority:
Normal
Assignee:
-
Category:
core
Target version:
Start date:
2018-01-10
Due date:
% Done:

100%

Estimated time:
Missing in 1.5.x:

Description

curl -v --data-ascii "0123456789abcdef" -H "Transfer-Encoding: chunked" 

fails with "400 - Bad Request" while
curl -v --data-ascii "0123456789abcde" -H "Transfer-Encoding: chunked" 

succeeds as the chunk is less than 16 bytes.

Bug caused by an incorrect overflow protection in connection_handle_read_post_chunked in connections-glue.c

Associated revisions

Revision dc1675ea (diff)
Added by gstrauss 11 months ago

[core] fix POST with chunked request body (fixes #2854)

(thx the_jk)

x-ref:
"chunked transfer encoding in request body only works for tiny chunks"
https://redmine.lighttpd.net/issues/2854

Revision 58a17939 (diff)
Added by gstrauss 11 months ago

[core] fix 32-bit compile POST w/ chunked request body (#2854)

(thx the_jk)

x-ref:
"chunked transfer encoding in request body only works for tiny chunks"
https://redmine.lighttpd.net/issues/2854

History

#2

Updated by gstrauss 11 months ago

  • Status changed from New to Patch Pending
  • Target version changed from 1.4.x to 1.4.49

Ick. Thanks for reporting this bug. It is undefined behavior to right-shift a signed integral value.
Try this:

--- a/src/connections-glue.c
+++ b/src/connections-glue.c
@@ -132,7 +132,13 @@ static handler_t connection_handle_read_post_chunked(server *srv, connection *co
                 off_t hsz = p + 1 - (c->mem->ptr+c->offset);
                 unsigned char *s = (unsigned char *)c->mem->ptr+c->offset;
                 for (unsigned char u;(u=(unsigned char)hex2int(*s))!=0xFF;++s) {
-                    if (te_chunked > (~((off_t)-1) >> 4)) {
+                    static const off_t plim =
+                      (sizeof(off_t) == sizeof(uint64_t))
+                      ? (off_t)((uint64_t)~(uint64_t)0 >> 5)   /*INT64_MAX>>4*/
+                      : (sizeof(off_t) == sizeof(uint32_t))
+                        ? (off_t)((uint32_t)~(uint32_t)0 >> 5) /*INT32_MAX>>4*/
+                        : (off_t)((uint16_t)~(uint16_t)0 >> 5);/*INT16_MAX>>4*/
+                    if (te_chunked > plim) {
                         log_error_write(srv, __FILE__, __LINE__, "s",
                                         "chunked data size too large -> 400");
                         /* 400 Bad Request */

#3

Updated by the_jk 11 months ago

Right, my suggestion ended up comparing with UINT64_MAX>>4. Your code above works. But the conditionals in the above hurts my eyes ;) so, an alternative version that also work:

            (off_t)(1lu << (8 * sizeof(off_t) - 5)) - 1)

Tried to write a testcase to actually test overflow but couldn't figure out how to make the test conditional on sizeof(off_t)

#4

Updated by gstrauss 11 months ago

  • Status changed from Patch Pending to Fixed
  • % Done changed from 0 to 100
#5

Updated by gstrauss 10 months ago

  • Has duplicate Bug #2864: Rest API failed when sending large number of data added
#6

Updated by gstrauss 10 months ago

  • Has duplicate deleted (Bug #2864: Rest API failed when sending large number of data)

Also available in: Atom