h2: guard PADDED frames with zero-length payload

[MegaManSec]

Fix OOB read in h2_recv_data() and h2_recv_headers() when PADDED is set but frame length is 0: we now require ≥1 payload byte before reading the Pad Length octet. Sends GOAWAY/PROTOCOL_ERROR instead of touching OOB.

[gstrauss]

The data is in a NIL-terminated string, so reading a 0-byte is 0. Do you have a tool which indicates this an OOB-read outside the buffer? The read should not be outside the buffer and is not an OOB-read. However, yes, the code is reading a byte when the remaining length in the frame is 0, even if the buffer has an extra \0.

[gstrauss]

I think this would be a simpler equivalent to your suggested patch:

--- a/src/h2.c
+++ b/src/h2.c
@@ -1188,7 +1188,7 @@ h2_recv_data (connection * const con, const uint8_t * const s, const uint32_t le
     uint32_t alen = len; /* actual data len, minus padding */
     uint32_t pad = 0;
     if (s[4] & H2_FLAG_PADDED) {
-        pad = s[9];
+        pad = len ? s[9] : 0;
         if (pad >= len) {
             h2_send_goaway_e(con, H2_E_PROTOCOL_ERROR);
             return 0;
@@ -1868,7 +1868,7 @@ h2_recv_headers (connection * const con, uint8_t * const s, uint32_t flen)
     uint32_t alen = flen;
     if (s[4] & H2_FLAG_PADDED) {
         ++psrc;
-        const uint32_t pad = s[9];
+        const uint32_t pad = alen ? s[9] : 0;
         if (alen < 1 + pad) {
             /* Padding that exceeds the size remaining for the header block
              * fragment MUST be treated as a PROTOCOL_ERROR. */

[MegaManSec]

right; yeah, out-of-frame, not out-of-buffer.

[gstrauss]

Thank you for the analysis and for putting together the PR.

I am curious: did you use a tool which flagged this code for further review?

Reviewing the code, I see that it has deterministic behavior reading the nil-terminated-string. The code tests the length after reading the padding byte in order to make a single check rather than two checks, knowing that the padding byte read is deterministic and valid even if 1 byte out of the frame (where \0 will be read instead). If you modify the PR to the following or something similar to add comments, I'll merge the commit with your name.

--- a/src/h2.c
+++ b/src/h2.c
@@ -1188,7 +1188,7 @@ h2_recv_data (connection * const con, const uint8_t * const s, const uint32_t le
     uint32_t alen = len; /* actual data len, minus padding */
     uint32_t pad = 0;
     if (s[4] & H2_FLAG_PADDED) {
-        pad = s[9];
+        pad = s[9]; /*(reads '\0' after string if 0 == len)*/
         if (pad >= len) {
             h2_send_goaway_e(con, H2_E_PROTOCOL_ERROR);
             return 0;
@@ -1868,7 +1868,7 @@ h2_recv_headers (connection * const con, uint8_t * const s, uint32_t flen)
     uint32_t alen = flen;
     if (s[4] & H2_FLAG_PADDED) {
         ++psrc;
-        const uint32_t pad = s[9];
+        const uint32_t pad = s[9]; /*(reads '\0' after string if 0 == alen)*/
         if (alen < 1 + pad) {
             /* Padding that exceeds the size remaining for the header block
              * fragment MUST be treated as a PROTOCOL_ERROR. */

[MegaManSec]

Done!

I am curious: did you use a tool which flagged this code for further review?

Yes; I'm using ZeroPath at the moment on some interesting (for me) codebases. Basically, it's an AI-powered vulnerability/bug scanner. For example in curl, I've reported (+ submitted patches) over 200 valid bugs (see this post or this article or this digest or this blog post).

If you're interested, I could send you the raw results from the scan against lighttpd1.4?

[gstrauss]

If you're interested, I could send you the raw results from the scan against lighttpd1.4?

Yes, please. I am interested.
You may find my email in the git commit history, or contact via https://wiki.lighttpd.net/
Thank you!