Http2 header 5780 v4 #8775

catenacyber · 2023-04-27T14:51:32Z

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

Describe changes:

http2: rename http2.header to http.request_header and http.response_header as it is confusing with http.header

Replaces #8621 with needed rebase (because DetectBufferSetActiveList wants now de_ctx as argument)

I have lots of questions...

Should this be one commit ?
Even if it is renaming + adding HTTP1 functionality

Did I get right the use of HttpHeaderBuffer ?

Is src/detect-http-header.c the right file do put it ?

I removed the validation callback as it is HTTP2-specific
Should we just remove the escaping being done in HTTP2, and not be able to match differently on a HTTP2 header name having comma in it like x: y versus a HTTP2 header having x as its name and y as its value ?

How do we merger with S-V having some test with rules with the keyword being deprecated ?

Do we keep the deprecated keyword in 6 ?

OISF/suricata-verify#1153

SV_BRANCH=pr/1153

Or http.response_header based on the direction http2.header had a different behavior than http.header and this was confusing. Ticket: OISF#5780

So that it is generic for HTTP1 and HTTP2 Ticket: OISF#5780

suricata-qa · 2023-04-27T19:10:03Z

WARNING:

field	baseline	test	%
	SURI_TLPW1_stats_chk
.tcp.rst	113274	145114	128.11%

Pipeline 13443

codecov · 2023-04-27T22:02:57Z

Codecov Report

Merging #8775 (00373c3) into master (cb66a1e) will decrease coverage by 0.03%.
The diff coverage is 49.51%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8775      +/-   ##
==========================================
- Coverage   82.30%   82.28%   -0.03%     
==========================================
  Files         969      969              
  Lines      272771   272846      +75     
==========================================
- Hits       224509   224499      -10     
- Misses      48262    48347      +85

Flag	Coverage Δ
fuzzcorpus	`64.59% <18.26%> (-0.09%)`	⬇️
suricata-verify	`60.31% <49.51%> (-0.05%)`	⬇️
unittests	`62.85% <18.26%> (-0.01%)`	⬇️

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

jufajardini

Doc changes look good to me :)

victorjulien · 2023-06-05T07:21:40Z

src/detect-http-header.c

+        headers = tx->response_headers;
+    }
+    if (cbdata->local_id < htp_table_size(headers)) {
+        htp_header_t *h = htp_table_get_index(headers, cbdata->local_id, NULL);


htp_table_get_index can return NULL I think

Well, it is not NULL-checked in detect-http-header-names.c

victorjulien · 2023-06-05T07:22:53Z

doc/userguide/rules/http-keywords.rst

+Match on the name and value of a HTTP request header (HTTP1 or HTTP2).
+
+For HTTP2, name and value get concatenated by ": ", colon and space.
+Each colon in the name or the value should be escaped as a double colon "::" for detection


I don't understand this escaping logic. The normal way to escape : in content is \:

But then, I would also need to escape backslash itself ;-)

victorjulien · 2023-06-05T07:30:07Z

Do we keep the deprecated keyword in 6 ?

Yes. But maybe it will be good to add a warning that it is removed in 7.

victorjulien · 2023-06-05T07:31:35Z

How do we merger with S-V having some test with rules with the keyword being deprecated ?

Old keyword test can go lt-version: 6, tests for new stuff min-version: 7

victorjulien · 2023-06-05T07:32:03Z

Is src/detect-http-header.c the right file do put it ?

Yeah seems good.

victorjulien · 2023-06-05T07:37:17Z

Did I get right the use of HttpHeaderBuffer ?

Looks good. Pattern is equal to http.start for example.

victorjulien · 2023-06-05T07:39:08Z

Should this be one commit ?
Even if it is renaming + adding HTTP1 functionality

Looks fine as is.

victorjulien · 2023-06-05T07:40:37Z

Should we just remove the escaping being done in HTTP2, and not be able to match differently on a HTTP2 header name having comma in it like x: y versus a HTTP2 header having x as its name and y as its value ?

Is this about the :: escaping? I didn't immediately see how it works, but I don't really like it. Would like buffers to be as "raw" as possible. Does that help?

victorjulien · 2023-06-05T07:41:30Z

src/detect-http-header.c

+}
+
+static int PrefilterMpmHttp1HeaderRegister(DetectEngineCtx *de_ctx, SigGroupHead *sgh,
+        MpmCtx *mpm_ctx, const DetectBufferMpmRegistery *mpm_reg, int list_id)


rebase hint: was renamed to DetectBufferMpmRegistry

catenacyber · 2023-06-06T20:25:36Z

Is this about the :: escaping? I didn't immediately see how it works, but I don't really like it. Would like buffers to be as "raw" as possible. Does that help?

Yes, this is about :: escaping.
Yes, this helps.

The reason for :: escaping was to be able to detect HTTP2 header names having :

catenacyber · 2023-06-07T14:25:08Z

Old keyword test can go lt-version: 6, tests for new stuff min-version: 7

That is more complex, as this is about rules, not S-V checks...

catenacyber · 2023-06-07T14:44:47Z

Replaced by #8986

catenacyber added 2 commits April 27, 2023 16:47

http2: rename http2.header to http.request_header

5427461

Or http.response_header based on the direction http2.header had a different behavior than http.header and this was confusing. Ticket: OISF#5780

http1: implement http.request_header

00373c3

So that it is generic for HTTP1 and HTTP2 Ticket: OISF#5780

catenacyber mentioned this pull request Apr 27, 2023

Http2 header 5780 v3 #8621

Closed

catenacyber added the needs rebase Needs rebase to master label May 4, 2023

victorjulien self-assigned this May 5, 2023

catenacyber added the needs review label May 12, 2023

jufajardini reviewed May 29, 2023

View reviewed changes

victorjulien requested changes Jun 5, 2023

View reviewed changes

victorjulien reviewed Jun 5, 2023

View reviewed changes

catenacyber mentioned this pull request Jun 7, 2023

Http2 header 5780 v5.3 #8986

Closed

catenacyber closed this Jun 7, 2023

This was referenced Jun 12, 2023

detect/http2: warning that http2.header is removed #9013

Closed

detect/http2: warning that http2.header is removed #9019

Closed

catenacyber mentioned this pull request Jun 26, 2023

detect/http2: warning that http2.header is removed #9074

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Http2 header 5780 v4 #8775

Http2 header 5780 v4 #8775

catenacyber commented Apr 27, 2023

suricata-qa commented Apr 27, 2023

codecov bot commented Apr 27, 2023 •

edited

jufajardini left a comment

victorjulien Jun 5, 2023

catenacyber Jun 7, 2023

victorjulien Jun 5, 2023

catenacyber Jun 6, 2023

victorjulien commented Jun 5, 2023

victorjulien commented Jun 5, 2023

victorjulien commented Jun 5, 2023

victorjulien commented Jun 5, 2023

victorjulien commented Jun 5, 2023

victorjulien commented Jun 5, 2023

victorjulien Jun 5, 2023

catenacyber commented Jun 6, 2023

catenacyber commented Jun 7, 2023

catenacyber commented Jun 7, 2023

Http2 header 5780 v4 #8775

Http2 header 5780 v4 #8775

Conversation

catenacyber commented Apr 27, 2023

suricata-qa commented Apr 27, 2023

codecov bot commented Apr 27, 2023 • edited

Codecov Report

jufajardini left a comment

Choose a reason for hiding this comment

victorjulien Jun 5, 2023

Choose a reason for hiding this comment

catenacyber Jun 7, 2023

Choose a reason for hiding this comment

victorjulien Jun 5, 2023

Choose a reason for hiding this comment

catenacyber Jun 6, 2023

Choose a reason for hiding this comment

victorjulien commented Jun 5, 2023

victorjulien commented Jun 5, 2023

victorjulien commented Jun 5, 2023

victorjulien commented Jun 5, 2023

victorjulien commented Jun 5, 2023

victorjulien commented Jun 5, 2023

victorjulien Jun 5, 2023

Choose a reason for hiding this comment

catenacyber commented Jun 6, 2023

catenacyber commented Jun 7, 2023

catenacyber commented Jun 7, 2023

codecov bot commented Apr 27, 2023 •

edited