Feature/http range/v29 #6355

catenacyber · 2021-09-07T18:49:34Z

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/1576

Describe changes:

Uses a generic container (a thread-safe hash table whose key is the url/filename) to store file parts with HTTP Range header
handles reassembly of these parts, while enforcing a global memcap

suricata-verify-pr: 531
OISF/suricata-verify#531

Continues #6354 with

Using SuricataContext so as to compile suricata crate without C for unit tests

For testing : https://github.com/jasonish/suricata-http-range-test

Compares two buffers with their two sizes

adds a container, ie a thread safe hash table whose key is the filename keep a tree of unordered ranges, up to a memcap limit adds HTPFileOpenWithRange to handle like HTPFileOpen if there is a range : open 2 files, one for the whole reassembled, and one only for the current range

Simplify locking by using the THashData lock instead of a separate range lock. Avoid size_t in function arguments. Clean up file handling functions. Implement handling of alloc errors. Rename yaml entry to byterange Unify public api naming

Better structure design to ensure that one flow maximum is owning and appending into the file, adding fileOwning field. Adds also a gap field in a range buffer, so that we can feed the gap on closing, when we are protected from concurrency by a lock, (lock which got removed in the append path) Fixes memcap when encountering a duplicate while inserting in red and black tree Adds many comments

To make concurrency reasoning clearer

so that borrow check gets happy

for future compatibility with rust

A block is determined out of order on opening. But on closing, the gap before it may have been filled. So, we must post-process it, ie iterate over the red and black tree so see what blocks we can get.

Move the content-range parsing code to rust

codecov · 2021-09-07T19:04:57Z

Codecov Report

Merging #6355 (2758109) into master (ca760e3) will decrease coverage by 0.02%.
The diff coverage is 69.26%.

@@            Coverage Diff             @@
##           master    #6355      +/-   ##
==========================================
- Coverage   76.96%   76.94%   -0.03%     
==========================================
  Files         612      613       +1     
  Lines      186410   186796     +386     
==========================================
+ Hits       143477   143723     +246     
- Misses      42933    43073     +140

Flag	Coverage Δ
fuzzcorpus	`52.69% <49.06%> (-0.07%)`	⬇️
suricata-verify	`51.60% <70.68%> (+0.08%)`	⬆️
unittests	`62.91% <3.90%> (-0.13%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

catenacyber · 2021-09-08T09:46:21Z

Includes https://redmine.openinfosecfoundation.org/issues/4117 as well

suricata-qa · 2021-09-08T13:41:22Z

Information:

field	test	baseline	%
	tlpr1_stats_chk
.uptime	1344	1220	110.16%
.flow.mgr.rows_maxlen	450	404	111.39%
.detect.alert	`3090528`	3947262	78.3%

Pipeline 4089

victorjulien · 2021-09-24T08:01:30Z

try: close before free < bad commit msg

victorjulien · 2021-09-24T08:04:28Z

rust/src/http2/http2.rs

+            match unsafe { SC } {
+                None => panic!("BUG no suricata_config"),
+                Some(c) => {
+                    //TODO get a file container instead of NULL


TODO: address or remove comment if its not needed

It will be much easier when we have file per tx

Right now, I should do something like state.files.get(STREAM_TOCLIENT) but I do not have an easy reference to state in HTTP2Transaction::free

So, what should we do ?

victorjulien · 2021-09-24T08:06:06Z

rust/src/http2/http2.rs

@@ -350,6 +402,7 @@ pub struct HTTP2State {
    transactions: Vec<HTTP2Transaction>,
    progress: HTTP2ConnectionState,
    pub files: Files,
+    flow: *const Flow,


ideally we're not adding this if we can just pass it around as a function argument

victorjulien · 2021-09-24T08:09:03Z

rust/src/http2/range.rs

@@ -0,0 +1,241 @@
+/* Copyright (C) 2021 Open Information Security Foundation


Is the parsing of the content-range string the same for http1 and http2? If so I'd like to consolidate the parsing in Rust

whoops already done it seems, please ignore

victorjulien · 2021-09-24T08:10:04Z

src/app-layer-htp-file.c

-
-    return 0;
+    uint32_t len = bstr_len(rawvalue);
+    return rs_http2_parse_content_range(range, bstr_ptr(rawvalue), len);


nit: I don't like that the call has 'http2' in the name

indeed, some leftover from development version when there were 2 parsing functions

catenacyber · 2021-09-24T13:21:30Z

try: close before free < bad commit msg

squashed into the right commit

catenacyber · 2021-09-24T13:23:51Z

Replaced by #6409

Add more details to the tcp.mss keyword engine analysis output Issue: OISF#6355

Issue: OISF#6355

victorjulien and others added 18 commits September 7, 2021 10:35

detect: suppress scan-build warning

1d48d57

util: adds util function SCBufferCmp

e690fcb

Compares two buffers with their two sizes

util: export Djb2 hash string function

18375bc

http/range: fix memory leak on out of order ranges

7e1ab56

http/range: optimize struct layout

e0c6071

http/range: cleanup and simplification

551a2f4

Simplify locking by using the THashData lock instead of a separate range lock. Avoid size_t in function arguments. Clean up file handling functions. Implement handling of alloc errors. Rename yaml entry to byterange Unify public api naming

thash: add debug validation check for use_cnt

5be9789

http: range transfering ownership of file container

7bdf3af

To make concurrency reasoning clearer

http: increase tracked files for ranges as well

33d1ada

http: more consistent return values for HTPFileOpenWithRange

27fcc74

try: close before free

ef8fedc

rust: rename to StreamingBufferConfig as in C

c0c1aa6

http2: better rust lifetimes

ae71206

so that borrow check gets happy

range: no final zero in key

6147dc2

for future compatibility with rust

range: post process out of order blocks

928b29c

A block is determined out of order on opening. But on closing, the gap before it may have been filled. So, we must post-process it, ie iterate over the red and black tree so see what blocks we can get.

http2: follow range requests

2758109

Move the content-range parsing code to rust

catenacyber requested review from jasonish, victorjulien and a team as code owners September 7, 2021 18:49

catenacyber mentioned this pull request Sep 7, 2021

Feature/http range/v28 #6354

Closed

victorjulien marked this pull request as draft September 24, 2021 08:01

victorjulien reviewed Sep 24, 2021

View reviewed changes

catenacyber mentioned this pull request Sep 24, 2021

Feature/http range/v30 #6409

Merged

catenacyber closed this Sep 24, 2021

0xEniola added a commit to 0xEniola/suricata that referenced this pull request Oct 21, 2023

detect/analyzer: add more details for the tcp.mss keyword

818f49d

Add more details to the tcp.mss keyword engine analysis output Issue: OISF#6355

0xEniola added a commit to 0xEniola/suricata that referenced this pull request Oct 23, 2023

detect/analyzer: add more details for tcp_mss

8a90382

Add more details to the tcp.mss keyword engine analysis output Issue: OISF#6355

0xEniola added a commit to 0xEniola/suricata that referenced this pull request Nov 11, 2023

detect/analyzer: add more details for tcp_mss

7d9ef35

Add more details to the tcp.mss keyword engine analysis output Issue: OISF#6355

0xEniola added a commit to 0xEniola/suricata that referenced this pull request Nov 11, 2023

detect/analyzer: add more details for tcp_mss

3cfe59d

Add more details to the tcp.mss keyword engine analysis output Issue: OISF#6355

0xEniola added a commit to 0xEniola/suricata that referenced this pull request Nov 12, 2023

detect/analyzer: add more details for tcp_mss

af87a35

Add more details to the tcp.mss keyword engine analysis output Issue: OISF#6355

0xEniola added a commit to 0xEniola/suricata that referenced this pull request Nov 13, 2023

detect/analyzer: add more details for tcp_mss

8e37698

Add more details to the tcp.mss keyword engine analysis output Issue: OISF#6355

0xEniola added a commit to 0xEniola/suricata that referenced this pull request Nov 13, 2023

detect/analyzer: add more details for tcp_mss

0ae40a8

Add more details to the tcp.mss keyword engine analysis output Issue: OISF#6355

catenacyber pushed a commit to catenacyber/suricata that referenced this pull request Apr 12, 2024

detect/analyzer: add more details for tcp_mss

e945ad9

Add more details to the tcp.mss keyword engine analysis output Issue: OISF#6355

catenacyber pushed a commit to catenacyber/suricata that referenced this pull request Apr 12, 2024

detect/analyzer: add more details for tcp_mss

143d2e2

Add more details to the tcp.mss keyword engine analysis output Issue: OISF#6355

catenacyber pushed a commit to catenacyber/suricata that referenced this pull request Apr 12, 2024

detect/analyzer: add more details for tcp_mss

37b72f9

Add more details to the tcp.mss keyword engine analysis output Issue: OISF#6355

catenacyber added a commit to catenacyber/suricata that referenced this pull request Apr 14, 2024

detect/analyzer: add more details for tcp_mss

bd3bed6

Issue: OISF#6355

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/http range/v29 #6355

Feature/http range/v29 #6355

catenacyber commented Sep 7, 2021

codecov bot commented Sep 7, 2021

catenacyber commented Sep 8, 2021

suricata-qa commented Sep 8, 2021

victorjulien commented Sep 24, 2021

victorjulien Sep 24, 2021

catenacyber Sep 24, 2021

victorjulien Sep 24, 2021

catenacyber Sep 24, 2021

victorjulien Sep 24, 2021

victorjulien Sep 24, 2021

victorjulien Sep 24, 2021

catenacyber Sep 24, 2021

catenacyber commented Sep 24, 2021

catenacyber commented Sep 24, 2021

		@@ -0,0 +1,241 @@
		/* Copyright (C) 2021 Open Information Security Foundation

Feature/http range/v29 #6355

Feature/http range/v29 #6355

Conversation

catenacyber commented Sep 7, 2021

codecov bot commented Sep 7, 2021

Codecov Report

catenacyber commented Sep 8, 2021

suricata-qa commented Sep 8, 2021

victorjulien commented Sep 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catenacyber commented Sep 24, 2021

catenacyber commented Sep 24, 2021