Reject invalid QPACK static indexes

include/proxy/http3/QPACK.h

@@ -24,6 +24,7 @@
 #pragma once
 
 #include <map>
+#include <unordered_map>
 
 #include "swoc/IntrusiveDList.h"
 
@@ -94,7 +95,7 @@ class QPACK : public QUICApplication
   class StaticTable
   {
   public:
-    static const XpackLookupResult lookup(uint16_t index, const char **name, size_t *name_len, const char **value,
+    static const XpackLookupResult lookup(uint64_t index, const char **name, size_t *name_len, const char **value,
                                           size_t *value_len);
     static const XpackLookupResult lookup(const char *name, size_t name_len, const char *value, size_t value_len);
 
@@ -193,12 +194,13 @@ class QPACK : public QUICApplication
     uint16_t largest;
   };
 
-  XpackDynamicTable                         _dynamic_table;
-  std::map<uint64_t, struct EntryReference> _references;
-  uint32_t                                  _max_field_section_size = 0;
-  uint32_t                                  _header_field_max_size  = 0;
-  uint16_t                                  _max_table_size         = 0;
-  uint16_t                                  _max_blocking_streams   = 0;
+  std::unordered_map<QUICStreamId, QUICStreamVCAdapter::IOInfo> _streams;
+  XpackDynamicTable                                             _dynamic_table;
+  std::map<uint64_t, struct EntryReference>                     _references;
+  uint32_t                                                      _max_field_section_size = 0;
+  uint32_t                                                      _header_field_max_size  = 0;
+  uint16_t                                                      _max_table_size         = 0;
+  uint16_t                                                      _max_blocking_streams   = 0;
 
   Continuation *_event_handler = nullptr;
   void          _resume_decode();
@@ -216,7 +218,7 @@ class QPACK : public QUICApplication
   void _update_reference_counts(uint64_t stream_id);
 
   // Encoder Stream
-  int _read_insert_with_name_ref(IOBufferReader &reader, bool &is_static, uint16_t &index, Arena &arena, char **value,
+  int _read_insert_with_name_ref(IOBufferReader &reader, bool &is_static, uint64_t &index, Arena &arena, char **value,
                                  size_t &value_len);
   int _read_insert_without_name_ref(IOBufferReader &reader, Arena &arena, char **name, size_t &name_len, char **value,
                                     size_t &value_len);

src/proxy/http3/QPACK.cc

@@ -21,6 +21,8 @@
  *  limitations under the License.
  */
 
+#include <limits>
+
 #include "proxy/hdrs/HTTP.h"
 #include "proxy/hdrs/XPACK.h"
 #include "proxy/http3/QPACK.h"
@@ -165,32 +167,34 @@ QPACK::~QPACK()
 void
 QPACK::on_stream_open(QUICStream &stream)
 {
-  auto *info = new QUICStreamVCAdapter::IOInfo(stream);
+  auto  ret  = this->_streams.emplace(stream.id(), stream);
+  auto &info = ret.first->second;
 
   switch (stream.direction()) {
   case QUICStreamDirection::BIDIRECTIONAL:
     // ink_assert(!"QPACK does not use bidirectional streams");
     // QPACK offline interop uses stream 0 as a encoder stream.
-    info->setup_write_vio(this);
-    info->setup_read_vio(this);
+    info.setup_write_vio(this);
+    info.setup_read_vio(this);
     break;
   case QUICStreamDirection::SEND:
-    info->setup_write_vio(this);
+    info.setup_write_vio(this);
     break;
   case QUICStreamDirection::RECEIVE:
-    info->setup_read_vio(this);
+    info.setup_read_vio(this);
     break;
   default:
     ink_assert(false);
     break;
   }
 
-  stream.set_io_adapter(&info->adapter);
+  stream.set_io_adapter(&info.adapter);
 }
 
 void
-QPACK::on_stream_close(QUICStream & /* stream ATS_UNUSED */)
+QPACK::on_stream_close(QUICStream &stream)
 {
+  this->_streams.erase(stream.id());
 }
 
 int
@@ -1142,20 +1146,30 @@ QPACK::_on_encoder_stream_read_ready(IOBufferReader &reader)
     reader.memcpy(&buf, 1);
     if (buf & 0x80) { // Insert With Name Reference
       bool        is_static;
-      uint16_t    index;
-      const char *name;
-      size_t      name_len;
-      const char *dummy;
-      size_t      dummy_len;
+      uint64_t    index;
+      const char *name      = nullptr;
+      size_t      name_len  = 0;
+      const char *dummy     = nullptr;
+      size_t      dummy_len = 0;
       char       *value;
       size_t      value_len;
       if (this->_read_insert_with_name_ref(reader, is_static, index, this->_arena, &value, value_len) < 0) {
         this->_abort_decode();
         return EVENT_DONE;
       }
-      QPACKDebug("Received Insert With Name Ref: is_static=%d, index=%d, value=%.*s", is_static, index, static_cast<int>(value_len),
-                 value);
-      StaticTable::lookup(index, &name, &name_len, &dummy, &dummy_len);
+      QPACKDebug("Received Insert With Name Ref: is_static=%d, index=%" PRIu64 ", value=%.*s", is_static, index,
+                 static_cast<int>(value_len), value);
+      XpackLookupResult result;
+      if (is_static) {
+        result = StaticTable::lookup(index, &name, &name_len, &dummy, &dummy_len);
+      } else if (index <= std::numeric_limits<uint32_t>::max()) {
+        result = this->_dynamic_table.lookup(static_cast<uint32_t>(index), &name, &name_len, &dummy, &dummy_len);
+      }
+      if (result.match_type != XpackLookupResult::MatchType::EXACT) {
+        this->_arena.str_free(value);
+        this->_abort_decode();
+        return EVENT_DONE;
+      }
       this->_dynamic_table.insert_entry(name, name_len, value, value_len);
       this->_arena.str_free(value);
     } else if (buf & 0x40) { // Insert Without Name Reference
@@ -1219,14 +1233,18 @@ QPACK::estimate_header_block_size(const HTTPHdr & /* hdr ATS_UNUSED */)
 }
 
 const XpackLookupResult
-QPACK::StaticTable::lookup(uint16_t index, const char **name, size_t *name_len, const char **value, size_t *value_len)
+QPACK::StaticTable::lookup(uint64_t index, const char **name, size_t *name_len, const char **value, size_t *value_len)
 {
+  if (index >= countof(STATIC_HEADER_FIELDS)) {
+    return {0, XpackLookupResult::MatchType::NONE};
+  }
+
   const Header &header = STATIC_HEADER_FIELDS[index];
   *name                = header.name;
   *name_len            = header.name_len;
   *value               = header.value;
   *value_len           = header.value_len;
-  return {index, XpackLookupResult::MatchType::EXACT};
+  return {static_cast<uint32_t>(index), XpackLookupResult::MatchType::EXACT};
 }
 
 const XpackLookupResult
@@ -1500,7 +1518,7 @@ QPACK::_write_stream_cancellation(uint64_t stream_id)
 }
 
 int
-QPACK::_read_insert_with_name_ref(IOBufferReader &reader, bool &is_static, uint16_t &index, Arena &arena, char **value,
+QPACK::_read_insert_with_name_ref(IOBufferReader &reader, bool &is_static, uint64_t &index, Arena &arena, char **value,
                                   size_t &value_len)
 {
   size_t   read_len = 0;
@@ -1514,10 +1532,9 @@ QPACK::_read_insert_with_name_ref(IOBufferReader &reader, bool &is_static, uint1
 
   // Name Index
   uint64_t tmp;
-  if ((ret = xpack_decode_integer(tmp, input, input + input_len, 6)) < 0 && tmp > 0xFFFF) {
+  if ((ret = xpack_decode_integer(index, input, input + input_len, 6)) < 0) {
     return -1;
   }
-  index     = tmp;
   read_len += ret;
 
   // Value

src/proxy/http3/test/test_QPACK.cc

@@ -22,10 +22,14 @@
  */
 
 #include <catch2/catch_test_macros.hpp>
+#include <atomic>
+#include <chrono>
 #include <cstdio>
 #include <cstdlib>
 #include <fstream>
 #include <iostream>
+#include <memory>
+#include <thread>
 #include "proxy/hdrs/XPACK.h"
 #include "proxy/http3/QPACK.h"
 #include "proxy/hdrs/HTTP.h"
@@ -70,7 +74,7 @@ class QUICApplicationDriver
 class TestQUICStream : public QUICStream
 {
 public:
-  TestQUICStream(QUICStreamId sid) : QUICStream(new MockQUICConnectionInfoProvider(), sid) {}
+  TestQUICStream(QUICStreamId sid) : TestQUICStream(std::make_unique<MockQUICConnectionInfoProvider>(), sid) {}
 
   void
   write(const uint8_t *buf, size_t buf_len, QUICOffset offset, bool last)
@@ -90,6 +94,14 @@ class TestQUICStream : public QUICStream
     this->_adapter->consume(nread);
     return nread;
   }
+
+private:
+  TestQUICStream(std::unique_ptr<MockQUICConnectionInfoProvider> info, QUICStreamId sid)
+    : QUICStream(info.get(), sid), _info(std::move(info))
+  {
+  }
+
+  std::unique_ptr<MockQUICConnectionInfoProvider> _info;
 };
 
 class TestQPACKEventHandler : public Continuation
@@ -105,15 +117,76 @@ class TestQPACKEventHandler : public Continuation
   }
 
   int
-  last_event()
+  last_event() const
   {
     return this->_event;
   }
 
 private:
-  int _event = 0;
+  // Written on the event thread that runs the QPACK callback and read by the
+  // thread running the test.
+  std::atomic<int> _event = 0;
 };
 
+/** Open a QPACK stream and write to it from an event thread.
+ *
+ * Both steps have to run on an event thread. QUICStreamVCAdapter schedules its
+ * read ready event on this_ethread() and will not schedule another one until
+ * that event is handled, so opening the stream from the thread running the test
+ * would leave the event queued on a thread that never runs an event loop, and
+ * the write that follows would then go unnoticed.
+ */
+class TestQPACKStreamWriter : public Continuation
+{
+public:
+  TestQPACKStreamWriter(QPACK &qpack, TestQUICStream &stream, const uint8_t *buf, size_t buf_len)
+    : Continuation(new_ProxyMutex()), _qpack(qpack), _stream(stream), _buf(buf), _buf_len(buf_len)
+  {
+    SET_HANDLER(&TestQPACKStreamWriter::write_handler);
+  }
+
+  int
+  write_handler(int /* event ATS_UNUSED */, Event * /* data ATS_UNUSED */)
+  {
+    this->_qpack.on_stream_open(this->_stream);
+    this->_stream.write(this->_buf, this->_buf_len, 0, false);
+    return 0;
+  }
+
+private:
+  QPACK          &_qpack;
+  TestQUICStream &_stream;
+  const uint8_t  *_buf;
+  size_t          _buf_len;
+};
+
+/** Wait for @a event_handler to observe @a expected_event.
+ *
+ * QPACK reports decode results by scheduling an event on an event thread, so a
+ * test has to wait for that callback. Polling keeps the common case fast while
+ * still failing, rather than hanging, when the event never arrives.
+ *
+ * @param[in] event_handler The handler that receives the QPACK events.
+ * @param[in] expected_event The event to wait for.
+ * @param[in] timeout The longest time to wait for @a expected_event.
+ *
+ * @return true if @a expected_event was observed, false on timeout.
+ */
+static bool
+wait_for_event(const TestQPACKEventHandler &event_handler, int expected_event,
+               std::chrono::milliseconds timeout = std::chrono::seconds(5))
+{
+  constexpr auto interval = std::chrono::milliseconds(10);
+
+  for (auto waited = std::chrono::milliseconds(0); waited < timeout; waited += interval) {
+    if (event_handler.last_event() == expected_event) {
+      return true;
+    }
+    std::this_thread::sleep_for(interval);
+  }
+  return event_handler.last_event() == expected_event;
+}
+
 static int
 load_qif_file(const char *filename, HTTPHdr **headers)
 {
@@ -405,6 +478,71 @@ test_decode(const char *enc_file, const char *out_file, int dts, int mbs)
   return ret;
 }
 
+TEST_CASE("Decoding out-of-range static table indexes fails", "[qpack-decode]")
+{
+  QUICApplicationDriver driver;
+  QPACK                 qpack(driver.get_connection(), UINT32_MAX, 0, 0, MAX_FIELD_SIZE);
+  TestQPACKEventHandler event_handler;
+  HTTPHdr               hdr;
+
+  hdr.create(HTTPType::REQUEST);
+
+  const uint8_t header_block[] = {
+    0x00, // Required Insert Count.
+    0x00, // Delta Base.
+    0xff, // Indexed static field with an extended 6-bit index.
+    0x25, // Index 100.
+  };
+
+  CHECK(qpack.decode(1, header_block, sizeof(header_block), hdr, &event_handler, eventProcessor.all_ethreads[0]) == 0);
+
+  CHECK(wait_for_event(event_handler, QPACK_EVENT_DECODE_FAILED));
+
+  hdr.destroy();
+}
+
+TEST_CASE("An out-of-range encoder stream name reference invalidates the decoder", "[qpack-decode]")
+{
+  QUICApplicationDriver driver;
+  QPACK                 qpack(driver.get_connection(), UINT32_MAX, 1024, 1, MAX_FIELD_SIZE);
+  TestQUICStream        encoder_stream(0);
+  TestQPACKEventHandler event_handler;
+  HTTPHdr               hdr;
+
+  hdr.create(HTTPType::REQUEST);
+
+  const uint8_t insert_with_name_ref[] = {
+    0xff, // Insert With Name Reference, static, with an extended 6-bit index.
+    0x25, // Index 100, one past the end of the static table.
+    0x01, // A one byte, unencoded value follows.
+    'x',
+  };
+  TestQPACKStreamWriter writer(qpack, encoder_stream, insert_with_name_ref, sizeof(insert_with_name_ref));
+
+  eventProcessor.all_ethreads[0]->schedule_imm(&writer);
+
+  // The rejected insert has to leave the decoder invalid rather than insert an
+  // entry built from an out-of-range lookup. The encoder stream is read on an
+  // event thread, so poll until that has happened.
+  const uint8_t empty_header_block[] = {
+    0x00, // Required Insert Count.
+    0x00, // Delta Base.
+  };
+  int ret = 0;
+
+  for (int i = 0; i < 500; ++i) {
+    ret = qpack.decode(1, empty_header_block, sizeof(empty_header_block), hdr, &event_handler, eventProcessor.all_ethreads[0]);
+    if (ret == -1) {
+      break;
+    }
+    std::this_thread::sleep_for(std::chrono::milliseconds(10));
+  }
+  CHECK(ret == -1);
+  CHECK(wait_for_event(event_handler, QPACK_EVENT_DECODE_FAILED));
+
+  hdr.destroy();
+}
+
 TEST_CASE("Encoding", "[qpack-encode]")
 {
   struct dirent *d;