Skip to content

htp: fixes warning about bad delimiter in URI#371

Closed
catenacyber wants to merge 1 commit intoOISF:0.5.xfrom
catenacyber:htp-spaces-warning-v1
Closed

htp: fixes warning about bad delimiter in URI#371
catenacyber wants to merge 1 commit intoOISF:0.5.xfrom
catenacyber:htp-spaces-warning-v1

Conversation

@catenacyber
Copy link
Contributor

With allow_space_uri, the protocol was checked for bad delimiter instead of the URI

See OISF/suricata#8134

// reset bad_delim found in protocol part
bad_delim = 0;
for (size_t i = start; i < pos; i++) {
if (data[i] != 0x20 && htp_is_space(data[i])) {
Copy link
Member

Choose a reason for hiding this comment

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

I assume 0x20 is covered by htp_is_space. Are you adding it explicitly to avoid the func call?

Copy link
Member

Choose a reason for hiding this comment

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

hmm no I guess I'm failing to understand the logic

Copy link
Member

Choose a reason for hiding this comment

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

So the goal is to set a warning if there are "spaces" in the URI other than 0x20, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the goal is to set a warning if there are "spaces" in the URI other than 0x20, right?

It looks so

I assume 0x20 is covered by htp_is_space.

Indeed, we want htp_is_space except for 0x20

With allow_space_uri, the protocol was checked for bad delimiter
instead of the URI
@catenacyber catenacyber force-pushed the htp-spaces-warning-v1 branch from 18c4501 to 0282f0d Compare November 14, 2022 11:07
@victorjulien victorjulien mentioned this pull request Nov 22, 2022
@victorjulien
Copy link
Member

Merged in #373, thanks!

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.

2 participants