Allow always max 1024 padding for HEADERS

We need paddings regardless of payload and frame boundary to mitigate
certain attacks.

Since we handles CONTINUATION internally, we don't show FLAG_PAD_HIGH
and PAD_LOW flags of HEADERS in nghttp/nghttpd. We just show the
total paddings in HEADERS + CONTINUATION.

lib/nghttp2_session.c

@@ -1141,10 +1141,7 @@ static ssize_t nghttp2_session_prep_frame(nghttp2_session *session,
         return framebuflen;
       }
       padded_payloadlen = session_call_select_padding
-        (session, frame,
+        (session, frame, frame->hd.length + 1024);
-         (frame->hd.length == 0 ? NGHTTP2_MAX_FRAME_LENGTH :
-          (frame->hd.length + NGHTTP2_MAX_FRAME_LENGTH - 1)
-          / NGHTTP2_MAX_FRAME_LENGTH * NGHTTP2_MAX_FRAME_LENGTH));
       if(nghttp2_is_fatal(padded_payloadlen)) {
         return padded_payloadlen;
       }
@@ -1152,13 +1149,46 @@ static ssize_t nghttp2_session_prep_frame(nghttp2_session *session,
       frame->headers.padlen = padded_payloadlen - frame->hd.length;
       frame->hd.length = padded_payloadlen;
 
+      DEBUGF(fprintf(stderr, "payloadlen=%zu, padlen=%zu\n",
+                     frame->hd.length, frame->headers.padlen));
+
       if(frame->hd.length > NGHTTP2_MAX_FRAME_LENGTH) {
-        /* PAD_HIGH and PAD_LOW will be added in
+        if(NGHTTP2_MAX_FRAME_LENGTH >
-           nghttp2_session_after_frame_sent(). */
+           frame->hd.length - frame->headers.padlen) {
-        /* This may make framebuflen > session->aob.framebufmax. But
+          size_t padlen = NGHTTP2_MAX_FRAME_LENGTH -
+            (frame->hd.length - frame->headers.padlen);
+          nghttp2_frame_hd hd = frame->hd;
+
+          DEBUGF(fprintf(stderr, "padding across 2 frames\n"));
+
+          hd.flags &= ~NGHTTP2_FLAG_END_HEADERS;
+          hd.length = NGHTTP2_MAX_FRAME_LENGTH;
+
+          DEBUGF(fprintf(stderr, "first HEADERS payloadlen=%zu, padlen=%zu\n",
+                         hd.length, padlen));
+
+          r = nghttp2_frame_add_pad(&session->aob.framebuf,
+                                    &session->aob.framebufmax,
+                                    &session->aob.framebufoff,
+                                    &hd.flags,
+                                    hd.length - padlen,
+                                    padlen);
+          if(nghttp2_is_fatal(r)) {
+            return r;
+          }
+          framebuflen = session->aob.framebufoff + frame->hd.length
+            + NGHTTP2_FRAME_HEAD_LENGTH;
+
+          nghttp2_frame_pack_frame_hd
+            (session->aob.framebuf + session->aob.framebufoff, &hd);
+        } else {
+          /* PAD_HIGH and PAD_LOW will be added in
+             nghttp2_session_after_frame_sent(). */
+          framebuflen += frame->headers.padlen;
+        }
+        /* At this point, framebuflen > session->aob.framebufmax. But
            before we access the missing part, we will allocate it in
            nghttp2_session_after_frame_sent(). */
-        framebuflen += frame->headers.padlen;
       } else if(frame->hd.length <= NGHTTP2_MAX_FRAME_LENGTH &&
                 frame->headers.padlen > 0) {
         r = nghttp2_frame_add_pad(&session->aob.framebuf,
@@ -1504,17 +1534,24 @@ static int nghttp2_session_after_frame_sent(nghttp2_session *session)
         session->aob.framebufoff -= NGHTTP2_FRAME_HEAD_LENGTH;
         if(cont_hd.length + session->aob.framebufmark ==
            session->aob.framebuflen) {
+          size_t padlen;
+          if(cont_hd.length < frame->headers.padlen) {
+            padlen = cont_hd.length;
+          } else {
+            padlen = frame->headers.padlen;
+          }
           DEBUGF(fprintf(stderr,
                          "last CONTINUATION payloadlen=%zu, padlen=%zu\n",
-                         cont_hd.length, frame->headers.padlen));
+                         cont_hd.length, padlen));
 
           cont_hd.flags = NGHTTP2_FLAG_END_HEADERS;
+
           rv = nghttp2_frame_add_pad(&session->aob.framebuf,
                                      &session->aob.framebufmax,
                                      &session->aob.framebufoff,
                                      &cont_hd.flags,
-                                     cont_hd.length - frame->headers.padlen,
+                                     cont_hd.length - padlen,
-                                     frame->headers.padlen);
+                                     padlen);
           if(nghttp2_is_fatal(rv)) {
             return rv;
           }
@@ -1524,9 +1561,6 @@ static int nghttp2_session_after_frame_sent(nghttp2_session *session)
           session->aob.framebuflen = session->aob.framebufmark =
             session->aob.framebufoff + NGHTTP2_FRAME_HEAD_LENGTH +
             cont_hd.length;
-          /* Or-ing flags so that we can show these flags in
-             callback */
-          frame->hd.flags |= cont_hd.flags;
         } else {
           cont_hd.flags = NGHTTP2_FLAG_NONE;
           session->aob.framebufmark += cont_hd.length;
@@ -3506,6 +3540,8 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session,
       nghttp2_frame_unpack_frame_hd(&iframe->frame.hd, iframe->buf);
       iframe->payloadleft = iframe->frame.hd.length;
 
+      DEBUGF(fprintf(stderr, "payloadlen=%zu\n", iframe->payloadleft));
+
       switch(iframe->frame.hd.type) {
       case NGHTTP2_DATA: {
         DEBUGF(fprintf(stderr, "DATA\n"));
@@ -3841,6 +3877,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session,
       if(iframe->payloadleft) {
         break;
       }
+      /* Clear PAD_HIGH and PAD_LOW, because we rely on those flags
+         in the next CONTINUATION frame. */
+      iframe->frame.hd.flags &=
+        ~(NGHTTP2_FLAG_PAD_HIGH | NGHTTP2_FLAG_PAD_LOW);
       if((iframe->frame.hd.flags & NGHTTP2_FLAG_END_HEADERS) == 0) {
         inbound_frame_reset_left(iframe, NGHTTP2_FRAME_HEAD_LENGTH);
         iframe->padlen = 0;
@@ -3950,6 +3990,9 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session,
       }
       nghttp2_frame_unpack_frame_hd(&cont_hd, iframe->buf);
       iframe->payloadleft = cont_hd.length;
+
+      DEBUGF(fprintf(stderr, "payloadlen=%zu\n", iframe->payloadleft));
+
       if(cont_hd.type != NGHTTP2_CONTINUATION ||
          cont_hd.stream_id != iframe->frame.hd.stream_id) {
         DEBUGF(fprintf(stderr, "expected stream_id=%d, type=%d, but "

tests/main.c

@@ -199,6 +199,8 @@ int main(int argc, char* argv[])
                    test_nghttp2_session_data_backoff_by_high_pri_frame) ||
       !CU_add_test(pSuite, "session_pack_data_with_padding",
                    test_nghttp2_session_pack_data_with_padding) ||
+      !CU_add_test(pSuite, "session_pack_headers_with_padding",
+                   test_nghttp2_session_pack_headers_with_padding) ||
       !CU_add_test(pSuite, "pack_settings_payload",
                    test_nghttp2_pack_settings_payload) ||
       !CU_add_test(pSuite, "frame_pack_headers",

tests/nghttp2_session_test.c

@@ -40,7 +40,7 @@
 #define OB_DATA(ITEM) nghttp2_outbound_item_get_data_frame(ITEM)
 
 typedef struct {
-  uint8_t buf[4096];
+  uint8_t buf[65535];
   size_t length;
 } accumulator;
 
@@ -4039,6 +4039,54 @@ void test_nghttp2_session_pack_data_with_padding(void)
   nghttp2_session_del(session);
 }
 
+void test_nghttp2_session_pack_headers_with_padding(void)
+{
+  nghttp2_session *session, *sv_session;
+  accumulator acc;
+  my_user_data ud;
+  nghttp2_session_callbacks callbacks, sv_callbacks;
+  nghttp2_nv nva[8190];
+  size_t i;
+
+  for(i = 0; i < ARRLEN(nva); ++i) {
+    nva[i].name = (uint8_t*)":path";
+    nva[i].namelen = 5;
+    nva[i].value = (uint8_t*)"/";
+    nva[i].valuelen = 1;
+  }
+
+  memset(&callbacks, 0, sizeof(callbacks));
+  callbacks.send_callback = accumulator_send_callback;
+  callbacks.on_frame_send_callback = on_frame_send_callback;
+  callbacks.select_padding_callback = select_padding_callback;
+
+  acc.length = 0;
+  ud.acc = &acc;
+
+  memset(&sv_callbacks, 0, sizeof(sv_callbacks));
+  sv_callbacks.on_frame_recv_callback = on_frame_recv_callback;
+
+  nghttp2_session_client_new(&session, &callbacks, &ud);
+  nghttp2_session_server_new(&sv_session, &sv_callbacks, &ud);
+
+  ud.padding_boundary = 16385;
+
+  CU_ASSERT(0 ==
+            nghttp2_submit_request(session, NGHTTP2_PRI_DEFAULT,
+                                   nva, ARRLEN(nva), NULL, NULL));
+  CU_ASSERT(0 == nghttp2_session_send(session));
+
+  CU_ASSERT(acc.length > NGHTTP2_MAX_FRAME_LENGTH);
+  ud.frame_recv_cb_called = 0;
+  CU_ASSERT((ssize_t)acc.length ==
+            nghttp2_session_mem_recv(sv_session, acc.buf, acc.length));
+  CU_ASSERT(1 == ud.frame_recv_cb_called);
+  CU_ASSERT(NULL == nghttp2_session_get_next_ob_item(sv_session));
+
+  nghttp2_session_del(sv_session);
+  nghttp2_session_del(session);
+}
+
 void test_nghttp2_pack_settings_payload(void)
 {
   nghttp2_settings_entry iv[2];

tests/nghttp2_session_test.h

@@ -90,6 +90,7 @@ void test_nghttp2_session_get_effective_local_window_size(void);
 void test_nghttp2_session_set_option(void);
 void test_nghttp2_session_data_backoff_by_high_pri_frame(void);
 void test_nghttp2_session_pack_data_with_padding(void);
+void test_nghttp2_session_pack_headers_with_padding(void);
 void test_nghttp2_pack_settings_payload(void);
 
 #endif /* NGHTTP2_SESSION_TEST_H */