Skip to content

Http form 2487 v3#14710

Closed
catenacyber wants to merge 4 commits intoOISF:mainfrom
catenacyber:http-form-2487-v3
Closed

Http form 2487 v3#14710
catenacyber wants to merge 4 commits intoOISF:mainfrom
catenacyber:http-form-2487-v3

Conversation

@catenacyber
Copy link
Contributor

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

Describe changes:

  • DRAFT : begins to add http.form keyword
  • Fix http_header keyword behavior on trailers !

SV_BRANCH=OISF/suricata-verify#2860

#14621 with more...

Needs to :

instead of a single progress.

Will help for keywords such as http.header which can act on
headers and trailers progress
reuse generic DetectEngineInspectBufferGeneric
even if the http request does not come into one packet
and the http_header is not the fast pattern
int sig_list = 0;
if (list_id == app_state_list_id)
sig_list = app_state_list_id;
// TODO check if we need to pass max_progress
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Advices here before I dive ?

uint16_t sm_list;
uint16_t sm_list_base; /**< base buffer being transformed */
int16_t progress;
// TODO move to u8 see BUG_ON vs 48
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a reason for signed i16 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc previously progress was an int and this was just an attempt to compress it. Progress should probably be converted to u8

@catenacyber catenacyber mentioned this pull request Jan 28, 2026
@victorjulien
Copy link
Member

I would like to see an explanation in the commit of what max_progress does and an explanation of what problem it solves (concrete issue analysis)

@catenacyber
Copy link
Contributor Author

I would like to see an explanation in the commit of what max_progress does and an explanation of what problem it solves (concrete issue analysis)

Just an exploratory draft here...

As said in the previous PR :

So, I am getting to the difficulty that we want a keyword which acts on 2 different progresses (request_line and request_body)

Solutions I see so far :

  • Divide the field progress of DetectEngineAppInspectionEngine into min_progress and max_progress
  • Have 2 DetectEngineAppInspectionEngine, one for each progress, and add some link between them, that either must match (not both)

Without this max_progress, we would think a signature with http.form will never match when it is at request_headers progress. Or it would match too late in IPS mode if we have min_progress request_body

Should I create a ticket for the http_header vs trailer bug ? (or desired behavior)

@suricata-qa
Copy link

ERROR:

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

Pipeline = 29327

@catenacyber
Copy link
Contributor Author

Something cleaner to start in #14717

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants