Merge pull request #2500 in SNORT/snort3 from ~THOPETER/snort3:h2i6 to master

Squashed commit of the following:

commit e7e8f2c22e796db2fe55cc202f02a55f2c76bf80
Author: Tom Peters <thopeter@cisco.com>
Date:   Tue Sep 15 19:49:55 2020 -0400

    http2_inspect: stream state tracking

diff --git a/src/service_inspectors/http2_inspect/dev_notes.txt b/src/service_inspectors/http2_inspect/dev_notes.txt
index 23fb9cdfbb971be0322e5385b8d25cb44b7766cb..83b4daf3104c7ea577199de0345a11d04fbf6923 100644 (file)
--- a/src/service_inspectors/http2_inspect/dev_notes.txt
+++ b/src/service_inspectors/http2_inspect/dev_notes.txt
@@ -22,7 +22,7 @@ frame_data buffer. The frame_data buffer is passed to the function decode_header
 main loop driving HPACK decoding. Each decoded header line is progressively written to a second
 decoded_headers buffer that will ultimately be sent to NHI.
 
-The main loop in decode_headers() finds the cut point for a single header line. The line is is
+The main loop in decode_headers() finds the cut point for a single header line. The line is
 passed to decode_header_line(), which parses the line and calls the appropriate decoding function
 based on the header representation type. If the type is indexed, the full header line is looked up
 in the table and copied to the decoded header buffer. The index may belong to either the static or
@@ -33,6 +33,16 @@ literal. The resulting header line is then added to the dynamic table. The third
 is literal not to be indexed, which is the same as literal to be indexed, except the header line is
 not added to the dynamic table.
 
+H2I has two levels of failure for flow processing. Fatal errors include failures in frame splitting
+and errors in header decoding that compromise the HPACK dictionary. A fatal error will trigger an
+immediate EVENT_MISFORMATTED_HTTP2 and will cause scan() to return ABORT the next time it is called
+in the same direction.
+
+Stream errors affect HTTP/1 processing within a stream. They include errors in frame sequence
+within a stream, errors that cause the HI stream splitter to abort, and errors that cause HI to be
+unable to process a frame. The individual stream will transition to STREAM_ERROR state and HI
+processing of that stream in that direction will end.
+
 H2I supports the NHI test tool. See ../http_inspect/dev_notes.txt for usage instructions.
 
 Memory requirements: Http2FlowData represents all H2I information in a flow. It does not account 

diff --git a/src/service_inspectors/http2_inspect/http2_data_frame.cc b/src/service_inspectors/http2_inspect/http2_data_frame.cc
index ba57bfab4cbe24e509af7e55ced032084285f244..aa1e407810eb2f92404ca5578ccec05755ec52e2 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_data_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_data_frame.cc
@@ -35,18 +35,27 @@ using namespace HttpCommon;
 using namespace snort;
 using namespace Http2Enums;
 
-Http2DataFrame::Http2DataFrame(const uint8_t* header_buffer, const int32_t header_len,
-    const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* session_data_,
+Http2DataFrame::Http2DataFrame(const uint8_t* header_buffer, const uint32_t header_len,
+    const uint8_t* data_buffer_, const uint32_t data_length_, Http2FlowData* session_data_,
     HttpCommon::SourceId source_id_, Http2Stream* stream_) :
     Http2Frame(header_buffer, header_len, nullptr, 0, session_data_, source_id_, stream_),
-    data_length(data_len)
+    data_length(data_length_),
+    data_buffer(data_buffer_)
+{}
+
+bool Http2DataFrame::valid_sequence(Http2Enums::StreamState state)
+{
+    return (state == Http2Enums::STREAM_EXPECT_BODY) || (state == Http2Enums::STREAM_BODY);
+}
+
+void Http2DataFrame::analyze_http1()
 {
-    if ((data_len != 0) || !session_data->flushing_data[source_id])
+    if ((data_length != 0) || !session_data->flushing_data[source_id])
     {
         Http2DummyPacket dummy_pkt;
         dummy_pkt.flow = session_data->flow;
         dummy_pkt.packet_flags = (source_id == SRC_CLIENT) ? PKT_FROM_CLIENT : PKT_FROM_SERVER;
-        dummy_pkt.dsize = data_len;
+        dummy_pkt.dsize = data_length;
         dummy_pkt.data = data_buffer;
         dummy_pkt.xtradata_mask = 0;
         session_data->hi->eval(&dummy_pkt);
@@ -56,8 +65,8 @@ Http2DataFrame::Http2DataFrame(const uint8_t* header_buffer, const int32_t heade
     else
     {
         detection_required = true;
-        HttpFlowData* const http_flow = (HttpFlowData*)session_data_->get_hi_flow_data();
-        http_flow->reset_partial_flush(source_id_);
+        HttpFlowData* const http_flow = (HttpFlowData*)session_data->get_hi_flow_data();
+        http_flow->reset_partial_flush(source_id);
     }
 }
 

diff --git a/src/service_inspectors/http2_inspect/http2_data_frame.h b/src/service_inspectors/http2_inspect/http2_data_frame.h
index b16db7c752cf6db638325386607faaf8dd842d81..86c534328224c8fe9ddb59c236e0bbc1b63f99a6 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_data_frame.h
+++ b/src/service_inspectors/http2_inspect/http2_data_frame.h
@@ -29,24 +29,27 @@ class Http2DataFrame : public Http2Frame
 {
 public:
     ~Http2DataFrame() override {}
+    bool valid_sequence(Http2Enums::StreamState state) override;
+    void analyze_http1() override;
     void clear() override;
 
     uint32_t get_xtradata_mask() override { return xtradata_mask; }
     bool is_detection_required() const override { return detection_required; }
     void update_stream_state() override;
 
-    friend Http2Frame* Http2Frame::new_frame(const uint8_t*, const int32_t, const uint8_t*,
-        const int32_t, Http2FlowData*, HttpCommon::SourceId, Http2Stream* stream);
+    friend Http2Frame* Http2Frame::new_frame(const uint8_t*, const uint32_t, const uint8_t*,
+        const uint32_t, Http2FlowData*, HttpCommon::SourceId, Http2Stream* stream);
 
 #ifdef REG_TEST
     void print_frame(FILE* output) override;
 #endif
 
 private:
-    Http2DataFrame(const uint8_t* header_buffer, const int32_t header_len,
-        const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* ssn_data,
+    Http2DataFrame(const uint8_t* header_buffer, const uint32_t header_len,
+        const uint8_t* data_buffer_, const uint32_t data_length_, Http2FlowData* ssn_data,
         HttpCommon::SourceId src_id, Http2Stream* stream);
-    uint32_t data_length = 0;
+    const uint32_t data_length;
+    const uint8_t* const data_buffer;
     uint32_t xtradata_mask = 0;
     bool detection_required = false;
 };

diff --git a/src/service_inspectors/http2_inspect/http2_enum.h b/src/service_inspectors/http2_inspect/http2_enum.h
index 286077501438a0b692c4503cddcdbcdca681d28a..a57c551b47a89a1531998ab44dff98d2bbf420ae 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_enum.h
+++ b/src/service_inspectors/http2_inspect/http2_enum.h
@@ -33,11 +33,12 @@ static const uint32_t HTTP2_GID = 121;
 
 // Frame type codes (fourth octet of frame header)
 enum FrameType : uint8_t { FT_DATA=0, FT_HEADERS=1, FT_PRIORITY=2, FT_RST_STREAM=3, FT_SETTINGS=4,
-    FT_PUSH_PROMISE=5, FT_PING=6, FT_GOAWAY=7, FT_WINDOW_UPDATE=8, FT_CONTINUATION=9, FT__ABORT=254,
+    FT_PUSH_PROMISE=5, FT_PING=6, FT_GOAWAY=7, FT_WINDOW_UPDATE=8, FT_CONTINUATION=9,
     FT__NONE=255 };
 
-enum StreamState { STREAM_EXPECT_HEADERS, STREAM_EXPECT_BODY, STREAM_BODY, STREAM_IMPLIED_COMPLETE,
-    STREAM_COMPLETE, STREAM_ERROR };
+// Ordered from initial state to terminal state. Do not rearrange without careful consideration.
+enum StreamState { STREAM_EXPECT_HEADERS, STREAM_EXPECT_BODY, STREAM_BODY, STREAM_COMPLETE,
+    STREAM_ERROR };
 
 // Message buffers available to clients
 // This enum must remain synchronized with Http2Api::classic_buffer_names[]
@@ -71,6 +72,7 @@ enum EventSid
     EVENT_PSEUDO_HEADER_AFTER_REGULAR_HEADER = 17,
     EVENT_PSEUDO_HEADER_IN_TRAILERS = 18,
     EVENT_INVALID_PSEUDO_HEADER = 19,
+    EVENT_TRAILERS_NOT_END = 20,
     EVENT__MAX_VALUE
 };
 
@@ -106,8 +108,9 @@ enum Infraction
     INF_INVALID_STARTLINE = 25,
     INF_INVALID_HEADER = 26,
     INF_PADDING_LEN = 27,
-    INF_TRAILERS_AFTER_END_STREAM = 28,
+    INF_UNUSED_2 = 28,
     INF_PSEUDO_HEADER_IN_TRAILERS = 29,
+    INF_TRAILERS_NOT_END = 30,
     INF__MAX_VALUE
 };
 

diff --git a/src/service_inspectors/http2_inspect/http2_flow_data.h b/src/service_inspectors/http2_inspect/http2_flow_data.h
index 57b43fd136ea94963883d6c679434d9b6f081838..98fc4483568cb106fb12b19255bade073754798f 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_flow_data.h
+++ b/src/service_inspectors/http2_inspect/http2_flow_data.h
@@ -160,6 +160,7 @@ protected:
 
     // Used by scan, reassemble and eval to communicate
     uint8_t frame_type[2] = { Http2Enums::FT__NONE, Http2Enums::FT__NONE };
+    bool abort_flow[2] = { false, false };
 
     // Internal to reassemble()
     uint32_t frame_header_offset[2] = { 0, 0 };

diff --git a/src/service_inspectors/http2_inspect/http2_frame.cc b/src/service_inspectors/http2_inspect/http2_frame.cc
index 4d01db28301bed3131db3e10f2a5e6bb4fca85e9..8da6116380fa1a02311fed2dd14756bd62b8cda6 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_frame.cc
@@ -36,19 +36,19 @@ using namespace HttpCommon;
 using namespace Http2Enums;
 using namespace snort;
 
-Http2Frame::Http2Frame(const uint8_t* header_buffer, const int32_t header_len,
-    const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* session_data,
-    SourceId source_id, Http2Stream* stream_) :  session_data(session_data), source_id(source_id),
-    stream(stream_)
+Http2Frame::Http2Frame(const uint8_t* header_buffer, const uint32_t header_len,
+    const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* session_data,
+    SourceId source_id, Http2Stream* stream_) :
+    session_data(session_data), source_id(source_id), stream(stream_)
 {
-    if (header_len > 0)
-        header.set(header_len, header_buffer, true);
+    header.set(header_len, header_buffer, true);
+    // FIXIT-E want to refactor so that zero-length frames are not a special case
     if (data_len > 0)
         data.set(data_len, data_buffer, true);
 }
 
-Http2Frame* Http2Frame::new_frame(const uint8_t* header, const int32_t header_len,
-    const uint8_t* data, const int32_t data_len, Http2FlowData* session_data, SourceId source_id,
+Http2Frame* Http2Frame::new_frame(const uint8_t* header, const uint32_t header_len,
+    const uint8_t* data, const uint32_t data_len, Http2FlowData* session_data, SourceId source_id,
     Http2Stream* stream)
 {
     // FIXIT-E call the appropriate frame subclass constructor based on the type

diff --git a/src/service_inspectors/http2_inspect/http2_frame.h b/src/service_inspectors/http2_inspect/http2_frame.h
index 73380eeb8c232b9e352d199b27da5aa4926739cb..6a278973483dc2d66c68efd4df888c2bc664f60d 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_frame.h
+++ b/src/service_inspectors/http2_inspect/http2_frame.h
@@ -40,8 +40,8 @@ class Http2Frame
 {
 public:
     virtual ~Http2Frame() { }
-    static Http2Frame* new_frame(const uint8_t* header_buffer, const int32_t header_len,
-        const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* session_data,
+    static Http2Frame* new_frame(const uint8_t* header_buffer, const uint32_t header_len,
+        const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* session_data,
         HttpCommon::SourceId source_id, Http2Stream* stream);
     virtual bool valid_sequence(Http2Enums::StreamState state)
         { return state != Http2Enums::STREAM_ERROR; }
@@ -57,8 +57,8 @@ public:
 #endif
 
 protected:
-    Http2Frame(const uint8_t* header_buffer, const int32_t header_len,
-        const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* session_data,
+    Http2Frame(const uint8_t* header_buffer, const uint32_t header_len,
+        const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* session_data,
         HttpCommon::SourceId source_id, Http2Stream* stream);
     uint8_t get_flags();
     uint32_t get_stream_id();

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.cc b/src/service_inspectors/http2_inspect/http2_headers_frame.cc
index b7dddc5a5f488b6b7de35c620a71e8f525bf1b34..02ab6eaaedb2a9abedfd18c0513a8aa631548e07 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame.cc
@@ -40,13 +40,13 @@ using namespace snort;
 using namespace HttpCommon;
 using namespace Http2Enums;
 
-Http2HeadersFrame::Http2HeadersFrame(const uint8_t* header_buffer, const int32_t header_len,
-    const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* session_data_,
+Http2HeadersFrame::Http2HeadersFrame(const uint8_t* header_buffer, const uint32_t header_len,
+    const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* session_data_,
     HttpCommon::SourceId source_id_, Http2Stream* stream_) :
     Http2Frame(header_buffer, header_len, data_buffer, data_len, session_data_, source_id_, stream_)
 {
-    // FIXIT-E check frame validity before creating frame
-    if (!check_frame_validity() or data.length() <= 0)
+    // FIXIT-E zero length should not be a special case
+    if (data.length() <= 0)
     {
         process_frame = false;
         return;
@@ -64,13 +64,12 @@ Http2HeadersFrame::Http2HeadersFrame(const uint8_t* header_buffer, const int32_t
 
 Http2HeadersFrame::~Http2HeadersFrame()
 {
-    delete http1_header;
     delete[] decoded_headers;
 }
 
 void Http2HeadersFrame::clear()
 {
-    if (error_during_decode || hi_abort)
+    if (session_data->abort_flow[source_id] || stream->get_state(source_id) == STREAM_ERROR)
         return;
     Packet dummy_pkt(false);
     dummy_pkt.flow = session_data->flow;
@@ -79,7 +78,7 @@ void Http2HeadersFrame::clear()
 
 void Http2HeadersFrame::process_decoded_headers(HttpFlowData* http_flow)
 {
-    if (error_during_decode)
+    if (session_data->abort_flow[source_id])
         return;
 
     http1_header = hpack_decoder->get_decoded_headers(decoded_headers);
@@ -92,21 +91,21 @@ void Http2HeadersFrame::process_decoded_headers(HttpFlowData* http_flow)
         dummy_pkt.flow = session_data->flow;
         const uint32_t unused = 0;
         const StreamSplitter::Status header_scan_result =
-            session_data->hi_ss[source_id]->scan(&dummy_pkt, http1_header->start(),
-            http1_header->length(), unused, &flush_offset);
+            session_data->hi_ss[source_id]->scan(&dummy_pkt, http1_header.start(),
+            http1_header.length(), unused, &flush_offset);
         assert(header_scan_result == StreamSplitter::FLUSH);
         UNUSED(header_scan_result);
-        assert((int64_t)flush_offset == http1_header->length());
+        assert((int64_t)flush_offset == http1_header.length());
     }
 
     // http_inspect reassemble() of headers
     {
         unsigned copied;
         stream_buf = session_data->hi_ss[source_id]->reassemble(session_data->flow,
-            http1_header->length(), 0, http1_header->start(), http1_header->length(), PKT_PDU_TAIL,
+            http1_header.length(), 0, http1_header.start(), http1_header.length(), PKT_PDU_TAIL,
             copied);
         assert(stream_buf.data != nullptr);
-        assert(copied == (unsigned)http1_header->length());
+        assert(copied == (unsigned)http1_header.length());
     }
 
     // http_inspect eval() of headers
@@ -118,15 +117,14 @@ void Http2HeadersFrame::process_decoded_headers(HttpFlowData* http_flow)
         dummy_pkt.data = stream_buf.data;
         dummy_pkt.xtradata_mask = 0;
         session_data->hi->eval(&dummy_pkt);
-        //Following if condition won't get exercised until finish() is
-        //implemented for H2I. Without finish() H2I will only flush
-        //complete header blocks. Below ABORT is only possible if
-        //tcp connection closes unexpectedly in middle of a header.
+        // Following if condition won't get exercised until finish() (during Headers) is
+        // implemented for H2I. Without finish() H2I will only flush complete header blocks. Below
+        // ABORT is only possible if tcp connection closes unexpectedly in middle of a header.
         if (http_flow->get_type_expected(source_id) == HttpEnums::SEC_ABORT)
         {
             *session_data->infractions[source_id] += INF_INVALID_HEADER;
             session_data->events[source_id]->create_event(EVENT_INVALID_HEADER);
-            hi_abort = true;
+            stream->set_state(source_id, STREAM_ERROR);
             return;
         }
         detection_required = dummy_pkt.is_detection_required();
@@ -140,67 +138,16 @@ const Field& Http2HeadersFrame::get_buf(unsigned id)
     {
     // FIXIT-M need to add a buffer for the decoded start line
     case HTTP2_BUFFER_DECODED_HEADER:
-        return *http1_header;
+        return http1_header;
     default:
         return Http2Frame::get_buf(id);
     }
 }
 
-// Return: continue processing frame
-bool Http2HeadersFrame::check_frame_validity()
-{
-    if (stream->get_state(source_id) == STREAM_COMPLETE)
-    {
-        *session_data->infractions[source_id] += INF_TRAILERS_AFTER_END_STREAM;
-        session_data->events[source_id]->create_event(EVENT_FRAME_SEQUENCE);
-        return false;
-    }
-    else if ((stream->get_state(source_id) != STREAM_EXPECT_HEADERS) and
-        !(get_flags() & END_STREAM))
-    {
-        // Trailers without END_STREAM flag set. Alert but continue to process.
-        *session_data->infractions[source_id] += INF_FRAME_SEQUENCE;
-        session_data->events[source_id]->create_event(EVENT_FRAME_SEQUENCE);
-    }
-    return true;
-}
-
-void Http2HeadersFrame::update_stream_state()
-{
-    switch (stream->get_state(source_id))
-    {
-        case STREAM_EXPECT_HEADERS:
-            if (get_flags() & END_STREAM)
-                stream->set_state(source_id, STREAM_COMPLETE);
-            else
-                stream->set_state(source_id, STREAM_EXPECT_BODY);
-            break;
-        case STREAM_EXPECT_BODY:
-            // fallthrough
-        case STREAM_BODY:
-            // Any trailing headers frame will be processed as trailers regardless of whether the
-            // END_STREAM flag is set
-            if (stream->get_state(source_id) == STREAM_BODY)
-                session_data->concurrent_files -= 1;
-            stream->set_state(source_id, STREAM_COMPLETE);
-            break;
-        case STREAM_COMPLETE:
-            // FIXIT-E frame validity should be checked before creating frame so we should not get
-            // here
-            break;
-        default:
-            // FIXIT-E build this out
-            break;
-    }
-}
-
 #ifdef REG_TEST
 void Http2HeadersFrame::print_frame(FILE* output)
 {
-    if (error_during_decode)
-        fprintf(output, "Error decoding headers.\n");
-    if (http1_header)
-        http1_header->print(output, "Decoded header");
+    http1_header.print(output, "Decoded header");
     Http2Frame::print_frame(output);
 }
 #endif

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.h b/src/service_inspectors/http2_inspect/http2_headers_frame.h
index 4e861b58cda18c00ee24ce0713c2b6c025ad1982..7f7c4904ca659fe51707c7b6425dc604894e9262 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame.h
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame.h
@@ -38,24 +38,19 @@ public:
     const Field& get_buf(unsigned id) override;
     uint32_t get_xtradata_mask() override { return xtradata_mask; }
     bool is_detection_required() const override { return detection_required; }
-    void update_stream_state() override;
 
 #ifdef REG_TEST
     void print_frame(FILE* output) override;
 #endif
 
 protected:
-    Http2HeadersFrame(const uint8_t* header_buffer, const int32_t header_len,
-        const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* ssn_data,
+    Http2HeadersFrame(const uint8_t* header_buffer, const uint32_t header_len,
+        const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* ssn_data,
         HttpCommon::SourceId src_id, Http2Stream* stream);
-    bool check_frame_validity();
     void process_decoded_headers(HttpFlowData* http_flow);
 
     uint8_t* decoded_headers = nullptr; // working buffer to store decoded headers
-    uint32_t decoded_headers_size = 0;
-    const Field* http1_header = nullptr; // finalized headers to be passed to NHI
-    bool error_during_decode = false;
-    bool hi_abort = false;
+    Field http1_header;                 // finalized headers to be passed to NHI
     uint32_t xtradata_mask = 0;
     bool detection_required = false;
     bool process_frame = true;

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc b/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc
index 573466a53b3aa136933cdd33405c79eaa7788bf5..b7bc20bf0e4956d1caea40c0cf2460606a4781cd 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc
@@ -40,9 +40,9 @@ using namespace snort;
 using namespace HttpCommon;
 using namespace Http2Enums;
 
-Http2HeadersFrameHeader::Http2HeadersFrameHeader(const uint8_t* header_buffer, const int32_t header_len,
-    const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* session_data_,
-    HttpCommon::SourceId source_id_, Http2Stream* stream_) :
+Http2HeadersFrameHeader::Http2HeadersFrameHeader(const uint8_t* header_buffer,
+    const uint32_t header_len, const uint8_t* data_buffer, const uint32_t data_len,
+    Http2FlowData* session_data_, HttpCommon::SourceId source_id_, Http2Stream* stream_) :
     Http2HeadersFrame(header_buffer, header_len, data_buffer, data_len, session_data_, source_id_,
         stream_)
 {
@@ -56,27 +56,33 @@ Http2HeadersFrameHeader::Http2HeadersFrameHeader(const uint8_t* header_buffer, c
     if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset), data.length() -
         hpack_headers_offset, decoded_headers, start_line_generator, false))
     {
-        session_data->frame_type[source_id] = FT__ABORT;
-        error_during_decode = true;
+        session_data->abort_flow[source_id] = true;
+        session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2);
+        return;
     }
 
     // process start line
     if (!start_line_generator->generate_start_line(start_line))
     {
-        // FIXIT-E set stream state to error
-        return;
+        // FIXIT-E should only be a stream error
+        session_data->abort_flow[source_id] = true;
+        session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2);
     }
 }
 
 Http2HeadersFrameHeader::~Http2HeadersFrameHeader()
 {
-    delete start_line;
     delete start_line_generator;
 }
 
+bool Http2HeadersFrameHeader::valid_sequence(Http2Enums::StreamState state)
+{
+    return (state == Http2Enums::STREAM_EXPECT_HEADERS);
+}
+
 void Http2HeadersFrameHeader::analyze_http1()
 {
-    if (!process_frame || start_line == nullptr)
+    if (!process_frame)
         return;
 
     // http_inspect scan() of start line
@@ -86,11 +92,11 @@ void Http2HeadersFrameHeader::analyze_http1()
         dummy_pkt.flow = session_data->flow;
         const uint32_t unused = 0;
         const StreamSplitter::Status start_scan_result =
-            session_data->hi_ss[source_id]->scan(&dummy_pkt, start_line->start(),
-                start_line->length(), unused, &flush_offset);
+            session_data->hi_ss[source_id]->scan(&dummy_pkt, start_line.start(),
+                start_line.length(), unused, &flush_offset);
         assert(start_scan_result == StreamSplitter::FLUSH);
         UNUSED(start_scan_result);
-        assert((int64_t)flush_offset == start_line->length());
+        assert((int64_t)flush_offset == start_line.length());
     }
 
     StreamBuffer stream_buf;
@@ -99,10 +105,10 @@ void Http2HeadersFrameHeader::analyze_http1()
     {
         unsigned copied;
         stream_buf = session_data->hi_ss[source_id]->reassemble(session_data->flow,
-            start_line->length(), 0, start_line->start(), start_line->length(), PKT_PDU_TAIL,
+            start_line.length(), 0, start_line.start(), start_line.length(), PKT_PDU_TAIL,
             copied);
         assert(stream_buf.data != nullptr);
-        assert(copied == (unsigned)start_line->length());
+        assert(copied == (unsigned)start_line.length());
     }
 
     HttpFlowData* const http_flow =
@@ -119,7 +125,7 @@ void Http2HeadersFrameHeader::analyze_http1()
         {
             *session_data->infractions[source_id] += INF_INVALID_STARTLINE;
             session_data->events[source_id]->create_event(EVENT_INVALID_STARTLINE);
-            hi_abort = true;
+            stream->set_state(source_id, STREAM_ERROR);
             return;
         }
         session_data->hi->clear(&dummy_pkt);
@@ -128,14 +134,21 @@ void Http2HeadersFrameHeader::analyze_http1()
     process_decoded_headers(http_flow);
 }
 
+void Http2HeadersFrameHeader::update_stream_state()
+{
+    if (stream->get_state(source_id) == STREAM_ERROR)
+        return;
+    if (get_flags() & END_STREAM)
+        stream->set_state(source_id, STREAM_COMPLETE);
+    else
+        stream->set_state(source_id, STREAM_EXPECT_BODY);
+}
+
 #ifdef REG_TEST
 void Http2HeadersFrameHeader::print_frame(FILE* output)
 {
     fprintf(output, "Headers frame\n");
-    if (start_line)
-        start_line->print(output, "Decoded start-line");
-    else
-        fprintf(output, "Error generating start line.\n");
+    start_line.print(output, "Decoded start-line");
     Http2HeadersFrame::print_frame(output);
 }
 #endif

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_header.h b/src/service_inspectors/http2_inspect/http2_headers_frame_header.h
index 28d4778f0ed42cb8491b0226bddb9c02b49d5c11..13b5e3ab039f7395380a01d4dc51aa4d519b40ab 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame_header.h
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame_header.h
@@ -30,21 +30,23 @@ class Http2HeadersFrameHeader : public Http2HeadersFrame
 public:
     ~Http2HeadersFrameHeader() override;
     
-    friend Http2Frame* Http2Frame::new_frame(const uint8_t*, const int32_t, const uint8_t*,
-        const int32_t, Http2FlowData*, HttpCommon::SourceId, Http2Stream* stream);
+    friend Http2Frame* Http2Frame::new_frame(const uint8_t*, const uint32_t, const uint8_t*,
+        const uint32_t, Http2FlowData*, HttpCommon::SourceId, Http2Stream* stream);
 
+    bool valid_sequence(Http2Enums::StreamState state) override;
     void analyze_http1() override;
+    void update_stream_state() override;
 
 #ifdef REG_TEST
     void print_frame(FILE* output) override;
 #endif
 
 private:
-    Http2HeadersFrameHeader(const uint8_t* header_buffer, const int32_t header_len,
-        const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* ssn_data,
+    Http2HeadersFrameHeader(const uint8_t* header_buffer, const uint32_t header_len,
+        const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* ssn_data,
         HttpCommon::SourceId src_id, Http2Stream* stream);
 
     Http2StartLine* start_line_generator = nullptr;
-    const Field* start_line = nullptr;
+    Field start_line;
 };
 #endif

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc
index 622af5138f099a5d3b0aaf08c0e08ec6bcc7f362..4739e65a35da8fa8506208120cd7ef9f7c3b91fe 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc
@@ -39,24 +39,36 @@ using namespace snort;
 using namespace HttpCommon;
 using namespace Http2Enums;
 
-Http2HeadersFrameTrailer::Http2HeadersFrameTrailer(const uint8_t* header_buffer, const int32_t header_len,
-    const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* session_data_,
-    HttpCommon::SourceId source_id_, Http2Stream* stream_) :
+Http2HeadersFrameTrailer::Http2HeadersFrameTrailer(const uint8_t* header_buffer,
+    const uint32_t header_len, const uint8_t* data_buffer, const uint32_t data_len,
+    Http2FlowData* session_data_, HttpCommon::SourceId source_id_, Http2Stream* stream_) :
     Http2HeadersFrame(header_buffer, header_len, data_buffer, data_len, session_data_, source_id_,
         stream_)
 {
     if (!process_frame)
         return;
 
+    if (!(get_flags() & END_STREAM))
+    {
+        // Trailers without END_STREAM flag set.
+        *session_data->infractions[source_id] += INF_TRAILERS_NOT_END;
+        session_data->events[source_id]->create_event(EVENT_TRAILERS_NOT_END);
+    }
+
     // Decode trailers
     if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset), data.length() -
         hpack_headers_offset, decoded_headers, nullptr, true))
     {
-        session_data->frame_type[source_id] = FT__ABORT;
-        error_during_decode = true;
+        session_data->abort_flow[source_id] = true;
+        session_data->events[source_id]->create_event(EVENT_MISFORMATTED_HTTP2);
     }
 }
 
+bool Http2HeadersFrameTrailer::valid_sequence(Http2Enums::StreamState state)
+{
+    return (state == Http2Enums::STREAM_EXPECT_BODY) || (state == Http2Enums::STREAM_BODY);
+}
+
 void Http2HeadersFrameTrailer::analyze_http1()
 {
     if (!process_frame)
@@ -90,7 +102,7 @@ void Http2HeadersFrameTrailer::analyze_http1()
         assert (http_flow->get_type_expected(source_id) == HttpEnums::SEC_TRAILER);
         if (http_flow->get_type_expected(source_id) == HttpEnums::SEC_ABORT)
         {
-            hi_abort = true;
+            stream->set_state(source_id, STREAM_ERROR);
             return;
         }
         session_data->hi->clear(&dummy_pkt);
@@ -99,6 +111,21 @@ void Http2HeadersFrameTrailer::analyze_http1()
     process_decoded_headers(http_flow);
 }
 
+void Http2HeadersFrameTrailer::update_stream_state()
+{
+    switch (stream->get_state(source_id))
+    {
+        case STREAM_BODY:
+            session_data->concurrent_files -= 1;
+            // fallthrough
+        case STREAM_EXPECT_BODY:
+            stream->set_state(source_id, STREAM_COMPLETE);
+            break;
+        default:
+            break;
+    }
+}
+
 #ifdef REG_TEST
 void Http2HeadersFrameTrailer::print_frame(FILE* output)
 {

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.h b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.h
index 1c12ba6bb4b0a2560c1f008eb76bef5e85f20892..a011ad3119b91e5c9fee0370d16528bd86068ca3 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.h
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.h
@@ -26,18 +26,20 @@
 class Http2HeadersFrameTrailer : public Http2HeadersFrame
 {
 public:
-    friend Http2Frame* Http2Frame::new_frame(const uint8_t*, const int32_t, const uint8_t*,
-        const int32_t, Http2FlowData*, HttpCommon::SourceId, Http2Stream* stream);
+    friend Http2Frame* Http2Frame::new_frame(const uint8_t*, const uint32_t, const uint8_t*,
+        const uint32_t, Http2FlowData*, HttpCommon::SourceId, Http2Stream* stream);
 
+    bool valid_sequence(Http2Enums::StreamState state) override;
     void analyze_http1() override;
+    void update_stream_state() override;
 
 #ifdef REG_TEST
     void print_frame(FILE* output) override;
 #endif
 
 private:
-    Http2HeadersFrameTrailer(const uint8_t* header_buffer, const int32_t header_len,
-        const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* ssn_data,
+    Http2HeadersFrameTrailer(const uint8_t* header_buffer, const uint32_t header_len,
+        const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* ssn_data,
         HttpCommon::SourceId src_id, Http2Stream* stream);
 };
 #endif

diff --git a/src/service_inspectors/http2_inspect/http2_hpack.cc b/src/service_inspectors/http2_inspect/http2_hpack.cc
index a9698fc247b50c46bc4f4678efbb118526e998e5..ab40feb8a40dc3079599c50598a0cef247d26ce7 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_hpack.cc
+++ b/src/service_inspectors/http2_inspect/http2_hpack.cc
@@ -49,7 +49,6 @@ bool Http2HpackDecoder::write_decoded_headers(const uint8_t* in_buffer, const ui
     {
         length = decoded_header_length;
         *infractions += INF_DECODED_HEADER_BUFF_OUT_OF_SPACE;
-        events->create_event(EVENT_MISFORMATTED_HTTP2);
         ret = false;
     }
 
@@ -94,7 +93,6 @@ const HpackTableEntry* Http2HpackDecoder::get_hpack_table_entry(
     if (!entry)
     {
         *infractions += INF_HPACK_INDEX_OUT_OF_BOUNDS;
-        events->create_event(EVENT_MISFORMATTED_HTTP2);
     }
     return entry;
 }
@@ -200,10 +198,10 @@ bool Http2HpackDecoder::decode_indexed_header(const uint8_t* encoded_header_buff
     name.set(entry->name);
     value.set(entry->value);
 
+    // FIXIT-E how bad is this?
     if (value.length() <= 0)
     {
         *infractions += INF_LOOKUP_EMPTY_VALUE;
-        events->create_event(EVENT_MISFORMATTED_HTTP2);
     }
 
     if (!write_header_part(name, (const uint8_t*)": ", 2, decoded_header_buffer,
@@ -249,27 +247,25 @@ bool Http2HpackDecoder::handle_dynamic_size_update(const uint8_t* encoded_header
     }
     bytes_consumed += encoded_bytes_consumed;
 
+    // Table size update shenanigans are dangerous because we cannot be sure how the target will
+    // interpret them.
     if (!table_size_update_allowed)
     {
         *infractions += INF_TABLE_SIZE_UPDATE_WITHIN_HEADER;
-        events->create_event(EVENT_MISFORMATTED_HTTP2);
-        return true;
+        return false;
     }
-    if (num_table_size_updates >= 2)
+    if (++num_table_size_updates > 2)
     {
         *infractions += INF_TOO_MANY_TABLE_SIZE_UPDATES;
-        events->create_event(EVENT_MISFORMATTED_HTTP2);
-        return true;
+        return false;
     }
 
     if (!decode_table.hpack_table_size_update(decoded_int))
     {
         *infractions += INF_INVALID_TABLE_SIZE_UPDATE;
-        events->create_event(EVENT_MISFORMATTED_HTTP2);
+        return false;
     }
 
-    num_table_size_updates++;
-
     return true;
 }
 
@@ -344,8 +340,8 @@ bool Http2HpackDecoder::decode_header_line(const uint8_t* encoded_header_buffer,
 
 // Entry point to decode an HPACK-encoded header block. This function returns true on successful
 // decode and false on an unrecoverable decode error. Note that alerts may still be generated for
-// recoverable errors while the function returns true. This function performs all decoding, but does
-// not output the start line or decoded headers - this function must be followed by calls to
+// recoverable errors while the function returns true. This function performs all decoding, but
+// does not output the start line or decoded headers - this function must be followed by calls to
 // generate_start_line() and get_decoded_headers() to generate and obtain these fields.
 bool Http2HpackDecoder::decode_headers(const uint8_t* encoded_headers,
     const uint32_t encoded_headers_length, uint8_t* decoded_headers,
@@ -357,11 +353,11 @@ bool Http2HpackDecoder::decode_headers(const uint8_t* encoded_headers,
     bool success = true;
     start_line = start_line_generator;
     decoded_headers_size = 0;
-    decode_error = false;
     is_trailers = trailers;
     pseudo_headers_allowed = !is_trailers;
 
-    // A maximum of two table size updates are allowed, and must be at the start of the header block
+    // A maximum of two table size updates are allowed, and must be at the start of the header
+    // block
     table_size_update_allowed = true;
     num_table_size_updates = 0;
 
@@ -382,15 +378,11 @@ bool Http2HpackDecoder::decode_headers(const uint8_t* encoded_headers,
             decoded_headers_size, MAX_OCTETS - decoded_headers_size, line_bytes_written);
         decoded_headers_size += line_bytes_written;
     }
-    else
-        decode_error = true;
+
     return success;
 }
 
-const Field* Http2HpackDecoder::get_decoded_headers(const uint8_t* const decoded_headers)
+Field Http2HpackDecoder::get_decoded_headers(const uint8_t* const decoded_headers)
 {
-    if (decode_error)
-        return new Field(STAT_NO_SOURCE);
-    else
-        return new Field(decoded_headers_size, decoded_headers, false);
+    return Field(decoded_headers_size, decoded_headers, false);
 }

diff --git a/src/service_inspectors/http2_inspect/http2_hpack.h b/src/service_inspectors/http2_inspect/http2_hpack.h
index 6f3badb8dd20ca7e401a97af8708ea288e150f97..e7f2a5f6ffc6d535a7cfead78cb2bedd70a1a646 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_hpack.h
+++ b/src/service_inspectors/http2_inspect/http2_hpack.h
@@ -74,14 +74,13 @@ public:
 
     bool finalize_start_line();
     const Field* get_start_line();
-    const Field* get_decoded_headers(const uint8_t* const decoded_headers);
+    Field get_decoded_headers(const uint8_t* const decoded_headers);
     HpackIndexTable* get_decode_table() { return &decode_table; }
 
 private:
     Http2StartLine* start_line;
     bool pseudo_headers_allowed;
     uint32_t decoded_headers_size;
-    bool decode_error;
     Http2EventGen* const events;
     Http2Infractions* const infractions;
 

diff --git a/src/service_inspectors/http2_inspect/http2_inspect.cc b/src/service_inspectors/http2_inspect/http2_inspect.cc
index 09f942f834d04b043729e3e838f4f6201fc005f6..cdd35e94c4f5d874088798c5a831ec79f8043d9f 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_inspect.cc
+++ b/src/service_inspectors/http2_inspect/http2_inspect.cc
@@ -123,8 +123,7 @@ void Http2Inspect::eval(Packet* p)
         return;
 
     // FIXIT-E Workaround for unexpected eval() calls
-    // Avoid eval if scan/reassemble aborts
-    if (session_data->frame_type[source_id] == FT__ABORT or
+    if (session_data->abort_flow[source_id] or
         ((session_data->frame_header[source_id] == nullptr ) and
         (session_data->frame_data[source_id] == nullptr)))
     {
@@ -183,6 +182,7 @@ void Http2Inspect::clear(Packet* p)
     session_data->frame_in_detection = false;
 
     const SourceId source_id = p->is_from_client() ? SRC_CLIENT : SRC_SERVER;
+
     Http2Stream* stream = session_data->get_current_stream(source_id);
     stream->clear_frame();
     session_data->stream_in_hi = NO_STREAM_ID;

diff --git a/src/service_inspectors/http2_inspect/http2_request_line.cc b/src/service_inspectors/http2_inspect/http2_request_line.cc
index 8f7aada3ad1b3aafbee6a1c306fdea6359fc32f2..ec56213d455a9a7d1c2af23fc9ae9312f785a6fb 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_request_line.cc
+++ b/src/service_inspectors/http2_inspect/http2_request_line.cc
@@ -78,7 +78,7 @@ void Http2RequestLine::process_pseudo_header(const Field& name, const Field& val
 
 // This is called on the first non-pseudo-header. Select the appropriate URI form based on the
 // provided pseudo-headers and generate the start line
-bool Http2RequestLine::generate_start_line(const Field*& start_line)
+bool Http2RequestLine::generate_start_line(Field& start_line)
 {
     uint32_t bytes_written = 0;
 
@@ -188,7 +188,7 @@ bool Http2RequestLine::generate_start_line(const Field*& start_line)
     }
     else
     {
-        // FIXIT-L May want to be more lenient than RFC on generating start line
+        // FIXIT-E May want to be more lenient than RFC on generating start line
         *infractions += INF_PSEUDO_HEADER_URI_FORM_MISMATCH;
         events->create_event(EVENT_MISFORMATTED_HTTP2);
         return false;
@@ -198,7 +198,7 @@ bool Http2RequestLine::generate_start_line(const Field*& start_line)
     bytes_written += 2;
     assert(bytes_written == start_line_length);
 
-    start_line = new Field(start_line_length, start_line_buffer, false);
+    start_line.set(start_line_length, start_line_buffer, false);
 
     return true;
 }

diff --git a/src/service_inspectors/http2_inspect/http2_request_line.h b/src/service_inspectors/http2_inspect/http2_request_line.h
index 1b48eeb346e61a5e4519537d0904db8c5aaaff2f..07294b438dfab42f918e745c006b256b53d44da7 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_request_line.h
+++ b/src/service_inspectors/http2_inspect/http2_request_line.h
@@ -30,7 +30,7 @@ class Http2RequestLine : public Http2StartLine
 {
 public:
     void process_pseudo_header(const Field& name, const Field& value) override;
-    bool generate_start_line(const Field*& start_line) override;
+    bool generate_start_line(Field& start_line) override;
 
     friend Http2StartLine* Http2StartLine::new_start_line_generator(HttpCommon::SourceId source_id,
         Http2EventGen* const events, Http2Infractions* const infractions);

diff --git a/src/service_inspectors/http2_inspect/http2_settings_frame.cc b/src/service_inspectors/http2_inspect/http2_settings_frame.cc
index bee617999cf076d9edf5471c1bc7ada5a1608491..c8409ae9b238d161ea670ba753a5e9b484d37ed4 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_settings_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_settings_frame.cc
@@ -46,10 +46,10 @@ static uint32_t get_parameter_value(const uint8_t* data_buffer)
         data_buffer[frame_value_index + 3];
 }
 
-Http2SettingsFrame::Http2SettingsFrame(const uint8_t* header_buffer, const int32_t header_len,
-    const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* ssn_data,
-    HttpCommon::SourceId src_id, Http2Stream* stream_) : Http2Frame(header_buffer, header_len, data_buffer, data_len,
-    ssn_data, src_id, stream_)
+Http2SettingsFrame::Http2SettingsFrame(const uint8_t* header_buffer, const uint32_t header_len,
+    const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* ssn_data,
+    HttpCommon::SourceId src_id, Http2Stream* stream_) : Http2Frame(header_buffer, header_len,
+    data_buffer, data_len, ssn_data, src_id, stream_)
 {
     if (!sanity_check())
     {
@@ -91,6 +91,7 @@ bool Http2SettingsFrame::sanity_check()
 {
     const bool ack = SfAck & get_flags();
 
+    // FIXIT-E this next check should possibly be moved to valid_sequence() 
     if (get_stream_id() != 0)
         bad_frame = true;
     else if (!ack and ((data.length() % 6) != 0))

diff --git a/src/service_inspectors/http2_inspect/http2_settings_frame.h b/src/service_inspectors/http2_inspect/http2_settings_frame.h
index ad910862ead3d5cb5628159a79e23a5df838a60d..544d91f7549f2b7b85e1f1970451594ec1e3da82 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_settings_frame.h
+++ b/src/service_inspectors/http2_inspect/http2_settings_frame.h
@@ -29,8 +29,8 @@ class Http2Frame;
 class Http2SettingsFrame : public Http2Frame
 {
 public:
-    friend Http2Frame* Http2Frame::new_frame(const uint8_t*, const int32_t, const uint8_t*,
-        const int32_t, Http2FlowData*, HttpCommon::SourceId, Http2Stream* stream);
+    friend Http2Frame* Http2Frame::new_frame(const uint8_t*, const uint32_t, const uint8_t*,
+        const uint32_t, Http2FlowData*, HttpCommon::SourceId, Http2Stream* stream);
     bool is_detection_required() const override { return false; }
 
 #ifdef REG_TEST
@@ -38,8 +38,8 @@ public:
 #endif
 
 private:
-    Http2SettingsFrame(const uint8_t* header_buffer, const int32_t header_len,
-        const uint8_t* data_buffer, const int32_t data_len, Http2FlowData* ssn_data,
+    Http2SettingsFrame(const uint8_t* header_buffer, const uint32_t header_len,
+        const uint8_t* data_buffer, const uint32_t data_len, Http2FlowData* ssn_data,
         HttpCommon::SourceId src_id, Http2Stream* stream);
 
     void parse_settings_frame();

diff --git a/src/service_inspectors/http2_inspect/http2_start_line.h b/src/service_inspectors/http2_inspect/http2_start_line.h
index a4dbff2e9e452e374baa0974778129841684339b..0e978bebeee7ce5a7fd1f138ed5b8bc32f000fcd 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_start_line.h
+++ b/src/service_inspectors/http2_inspect/http2_start_line.h
@@ -43,7 +43,7 @@ public:
 
     friend class Http2Hpack;
 
-    virtual bool generate_start_line(const Field*& start_line) = 0;
+    virtual bool generate_start_line(Field& start_line) = 0;
     virtual void process_pseudo_header(const Field& name, const Field& value) = 0;
 
 protected:

diff --git a/src/service_inspectors/http2_inspect/http2_status_line.cc b/src/service_inspectors/http2_inspect/http2_status_line.cc
index f66fee8d3231a532967c8dc4449d3b1884192177..2db7f52f823facbdc18dcbf7b22b525951ab5b44 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_status_line.cc
+++ b/src/service_inspectors/http2_inspect/http2_status_line.cc
@@ -53,7 +53,7 @@ void Http2StatusLine::process_pseudo_header(const Field& name, const Field& valu
 }
 
 // This is called on the first non-pseudo-header.
-bool Http2StatusLine::generate_start_line(const Field*& start_line)
+bool Http2StatusLine::generate_start_line(Field& start_line)
 {
     uint32_t bytes_written = 0;
 
@@ -80,7 +80,7 @@ bool Http2StatusLine::generate_start_line(const Field*& start_line)
     bytes_written += 2;
     assert(bytes_written == start_line_length);
 
-    start_line = new Field(start_line_length, start_line_buffer, false);
+    start_line.set(start_line_length, start_line_buffer, false);
 
     return true;
 }

diff --git a/src/service_inspectors/http2_inspect/http2_status_line.h b/src/service_inspectors/http2_inspect/http2_status_line.h
index 9b497089e0b4750b080923257d6e64d08fcef33a..fd80b908afdda30d6fbac7983e7a21c8cf64f6a6 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_status_line.h
+++ b/src/service_inspectors/http2_inspect/http2_status_line.h
@@ -29,7 +29,7 @@ class Http2StatusLine : public Http2StartLine
 {
 public:
     void process_pseudo_header(const Field& name, const Field& value) override;
-    bool generate_start_line(const Field*& start_line) override;
+    bool generate_start_line(Field& start_line) override;
 
     friend Http2StartLine* Http2StartLine::new_start_line_generator(HttpCommon::SourceId source_id,
         Http2EventGen* const events, Http2Infractions* const infractions);

diff --git a/src/service_inspectors/http2_inspect/http2_stream.cc b/src/service_inspectors/http2_inspect/http2_stream.cc
index 44ffc05b6441eca35d9c0642d0c054f3556debc2..db2b66e2904a22eddc5f4bb4c3eb4138fbe0db32 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream.cc
@@ -51,16 +51,24 @@ Http2Stream::~Http2Stream()
     delete data_cutter[SRC_SERVER];
 }
 
-void Http2Stream::eval_frame(const uint8_t* header_buffer, int32_t header_len,
-    const uint8_t* data_buffer, int32_t data_len, SourceId source_id)
+void Http2Stream::eval_frame(const uint8_t* header_buffer, uint32_t header_len,
+    const uint8_t* data_buffer, uint32_t data_len, SourceId source_id)
 {
     assert(current_frame == nullptr);
     current_frame = Http2Frame::new_frame(header_buffer, header_len, data_buffer,
         data_len, session_data, source_id, this);
-    valid_frame_order[source_id] = valid_frame_order[source_id] &&
-        current_frame->valid_sequence(state[source_id]);
-    current_frame->analyze_http1();
-    current_frame->update_stream_state();
+    if (!session_data->abort_flow[source_id] && (get_state(source_id) != STREAM_ERROR))
+    {
+        if (current_frame->valid_sequence(state[source_id]))
+        {
+            current_frame->analyze_http1();
+            current_frame->update_stream_state();
+        }
+        else
+        {
+            set_state(source_id, STREAM_ERROR);
+        }
+    }
 }
 
 void Http2Stream::clear_frame()

diff --git a/src/service_inspectors/http2_inspect/http2_stream.h b/src/service_inspectors/http2_inspect/http2_stream.h
index 33413687b35492ed9024bd1fa78668936bfb3126..abf761939bdc144f1e42cbff82d4ae276a6cdf75 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream.h
+++ b/src/service_inspectors/http2_inspect/http2_stream.h
@@ -37,8 +37,8 @@ public:
     Http2Stream(uint32_t stream_id, Http2FlowData* session_data_);
     ~Http2Stream();
     uint32_t get_stream_id() { return stream_id; }
-    void eval_frame(const uint8_t* header_buffer, int32_t header_len, const uint8_t* data_buffer,
-        int32_t data_len, HttpCommon::SourceId source_id);
+    void eval_frame(const uint8_t* header_buffer, uint32_t header_len, const uint8_t* data_buffer,
+        uint32_t data_len, HttpCommon::SourceId source_id);
     void clear_frame();
     const Field& get_buf(unsigned id);
     HttpFlowData* get_hi_flow_data() const { return hi_flow_data; }
@@ -78,7 +78,6 @@ public:
 private:
     const uint32_t stream_id;
     Http2FlowData* const session_data;
-    bool valid_frame_order[2] = { true, true };
     Http2Frame* current_frame = nullptr;
     HttpFlowData* hi_flow_data = nullptr;
     HttpMsgSection* hi_msg_section = nullptr;

diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter.cc b/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
index c92808aaaae5c6f54894c0ece7930e3732055fc9..72ed9233764aa8073095a68517b5e1e0d672fabe 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream_splitter.cc
@@ -89,14 +89,14 @@ StreamSplitter::Status Http2StreamSplitter::scan(Packet* pkt, const uint8_t* dat
 #endif
 
     // General mechanism to abort using scan
-    if (session_data->frame_type[source_id] == FT__ABORT)
+    if (session_data->abort_flow[source_id])
         return HttpStreamSplitter::status_value(StreamSplitter::ABORT, true);
 
     const StreamSplitter::Status ret_val =
         implement_scan(session_data, data, length, flush_offset, source_id);
 
     if (ret_val == StreamSplitter::ABORT)
-        session_data->frame_type[source_id] = FT__ABORT;
+        session_data->abort_flow[source_id] = true;
 
 #ifdef REG_TEST
     if (HttpTestManager::use_test_input(HttpTestManager::IN_HTTP2))
@@ -151,6 +151,8 @@ const StreamBuffer Http2StreamSplitter::reassemble(Flow* flow, unsigned total, u
     }
 #endif
 
+    assert(!session_data->abort_flow[source_id]);
+
     // FIXIT-P: scan uses this to discard bytes until StreamSplitter:DISCARD
     // is implemented
     if (session_data->payload_discard[source_id])
@@ -191,6 +193,8 @@ bool Http2StreamSplitter::finish(Flow* flow)
     Http2FlowData* session_data = (Http2FlowData*)flow->get_flow_data(Http2FlowData::inspector_id);
     if (!session_data)
         return false;
+    if (session_data->abort_flow[source_id])
+        return false;
 
 #ifdef REG_TEST
     if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP2))
@@ -214,7 +218,7 @@ bool Http2StreamSplitter::finish(Flow* flow)
     for (const Http2FlowData::StreamInfo& stream_info : session_data->streams)
     {
         if ((stream_info.id == 0)                                                 ||
-            (stream_info.stream->get_state(source_id) == STREAM_COMPLETE)         ||
+            (stream_info.stream->get_state(source_id) >= STREAM_COMPLETE)         ||
             (stream_info.stream->get_hi_flow_data() == nullptr)                   ||
             (stream_info.stream->get_hi_flow_data()->get_type_expected(source_id)
                 != HttpEnums::SEC_BODY_H2))
@@ -251,7 +255,8 @@ bool Http2StreamSplitter::init_partial_flush(Flow* flow)
 
     Http2FlowData* session_data = (Http2FlowData*)flow->get_flow_data(Http2FlowData::inspector_id);
     assert(session_data != nullptr);
-    UNUSED(session_data); // just for a little while
+    if (session_data->abort_flow[source_id])
+        return false;
 
 #ifdef REG_TEST
     if (HttpTestManager::use_test_output(HttpTestManager::IN_HTTP2) &&

diff --git a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc
index df08d4c00566e0bc6123fa94443261721d313fe1..540266e1fc5a979587cc0408e0dd50bd95cae2d0 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc
+++ b/src/service_inspectors/http2_inspect/http2_stream_splitter_impl.cc
@@ -363,9 +363,8 @@ const StreamBuffer Http2StreamSplitter::implement_reassemble(Http2FlowData* sess
         // This is the first reassemble() for this frame and we need to allocate some buffers
         session_data->frame_header_size[source_id] = FRAME_HEADER_LENGTH *
             session_data->num_frame_headers[source_id];
-        if (session_data->frame_header_size[source_id] > 0)
-            session_data->frame_header[source_id] =
-                new uint8_t[session_data->frame_header_size[source_id]];
+        session_data->frame_header[source_id] =
+            new uint8_t[session_data->frame_header_size[source_id]];
 
         session_data->frame_header_offset[source_id] = 0;
     }
@@ -374,7 +373,7 @@ const StreamBuffer Http2StreamSplitter::implement_reassemble(Http2FlowData* sess
     {
         if (session_data->flushing_data[source_id])
         {
-            assert(total  > (FRAME_HEADER_LENGTH - 1));
+            assert(total > (FRAME_HEADER_LENGTH - 1));
             const uint32_t total_data = total - (FRAME_HEADER_LENGTH - 1);
             if (offset+len > total_data)
             {
@@ -392,10 +391,11 @@ const StreamBuffer Http2StreamSplitter::implement_reassemble(Http2FlowData* sess
 
         if (len != 0)
         {
-            Http2DataCutter* data_cutter = stream->get_data_cutter(source_id);
+            Http2DataCutter* const data_cutter = stream->get_data_cutter(source_id);
             StreamBuffer http_frame_buf = data_cutter->reassemble(data, len);
             if (http_frame_buf.data)
             {
+                // FIXIT-L this use of const_cast is worrisome
                 session_data->frame_data[source_id] = const_cast<uint8_t*>(http_frame_buf.data);
                 session_data->frame_data_size[source_id] = http_frame_buf.length;
                 if (!session_data->flushing_data[source_id] && stream->is_partial_buf_pending(

diff --git a/src/service_inspectors/http2_inspect/http2_tables.cc b/src/service_inspectors/http2_inspect/http2_tables.cc
index 4a4808f9b1580bbc8b676c5f1fa7171a0a2a72e8..ae1fae5cfd371271825586557dfd4fe5520fbb90 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_tables.cc
+++ b/src/service_inspectors/http2_inspect/http2_tables.cc
@@ -50,6 +50,7 @@ const RuleMap Http2Module::http2_events[] =
     { EVENT_PSEUDO_HEADER_AFTER_REGULAR_HEADER, "HTTP/2 pseudo-header after regular header" },
     { EVENT_PSEUDO_HEADER_IN_TRAILERS, "HTTP/2 pseudo-header in trailers" },
     { EVENT_INVALID_PSEUDO_HEADER, "invalid HTTP/2 pseudo-header" },
+    { EVENT_TRAILERS_NOT_END, "HTTP/2 trailers without END_STREAM bit" },
     { 0, nullptr }
 };
 

diff --git a/src/service_inspectors/http_inspect/http_field.cc b/src/service_inspectors/http_inspect/http_field.cc
index 2fe4798603c828b0f0b70073903de99a8c27ad58..148d91ec1a6b5312bf0b113c983bf2775eb7f27b 100644 (file)
--- a/src/service_inspectors/http_inspect/http_field.cc
+++ b/src/service_inspectors/http_inspect/http_field.cc
@@ -39,6 +39,16 @@ Field::Field(int32_t length, const uint8_t* start, bool own_the_buffer_) :
     assert(!((start != nullptr) && (length < 0)));
 }
 
+Field& Field::operator=(const Field& rhs)
+{
+    assert(len == STAT_NOT_COMPUTE);
+    assert(strt == nullptr);
+    strt = rhs.strt;
+    len = rhs.len;
+    own_the_buffer = false;    // buffer must not have two owners
+    return *this;
+}
+
 void Field::set(int32_t length, const uint8_t* start, bool own_the_buffer_)
 {
     assert(len == STAT_NOT_COMPUTE);

diff --git a/src/service_inspectors/http_inspect/http_field.h b/src/service_inspectors/http_inspect/http_field.h
index 9d42f957c51571221da9881c76d6904061d1a625..57c656a7ab20424c6b6c2e05ceaf9af9ee4de56b 100644 (file)
--- a/src/service_inspectors/http_inspect/http_field.h
+++ b/src/service_inspectors/http_inspect/http_field.h
@@ -38,6 +38,7 @@ public:
     Field(int32_t length, const uint8_t* start, bool own_the_buffer_ = false);
     explicit Field(int32_t length) : len(length) { assert(length<=0); }
     Field() = default;
+    Field& operator=(const Field& rhs);
     ~Field() { if (own_the_buffer) delete[] strt; }
     int32_t length() const { return len; }
     const uint8_t* start() const { return strt; }
@@ -51,8 +52,6 @@ public:
 #endif
 
 private:
-    Field& operator=(const Field&) = delete;
-
     const uint8_t* strt = nullptr;
     int32_t len = HttpCommon::STAT_NOT_COMPUTE;
     bool own_the_buffer = false;