New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing Content-Type: create critical sibling of 920340 in PL2 #1118

Merged
merged 5 commits into from Jul 9, 2018

Conversation

Projects
None yet
3 participants
@lifeforms
Copy link
Collaborator

lifeforms commented Jun 4, 2018

As per discussion during the June IRC meet, we leave 920340 as a NOTICE (since it is technically allowed by the spec) but create a CRITICAL copy of the rule in paranoia level 2.

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jun 5, 2018

This works as advertised.

Two minor things:

Typo

Could you capitalize "PL2: block" please?

Message

The two rules have identical messages. This is a bit odd:

920340 Request Containing Content, but Missing Content-Type header
920341 Request Containing Content, but Missing Content-Type header

I'm not sure how to bring a clearer message across.

@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Jun 8, 2018

I'm a bit confused. While the severity is different both rules are exactly the same. e.g.

PL2: block on Missing Content-Type Header with Request Body

The part about blocking is true for both rules.

Am I missing something?

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jun 9, 2018

You got this right. Here is why:

920340 only brings 2 points. But it's a major evasion as you can work around URLENCODED bodyprocessor this way. Now @spartantri thinks mobile clients omit the header a lot, while @lifeforms says he hardly sees any FPs. As long as we do not know for sure we do not want to raise the severity of 920340, however, we want to make sure to block this evasion at PL2 by default. Hence the duplication via 920341.

I think it is odd, to bring a stricter twin of a rule, but here, I personally think it is appropriate.

@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Jun 9, 2018

@dune73 Not sure if my point was understood so let me rephrase: based on the comment on 920341, shouldn't 920340 be passing?

Re mobile clients, maybe look at httparchive to confirm?

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jun 9, 2018

Got you now, thanks.

@lifeforms, what do you think?

I am not really into this blocking / passing in CRS as I always run in scoring mode.

@fgsch

This comment has been minimized.

Copy link
Collaborator

fgsch commented Jun 9, 2018

FTR, all the rules in NOTICE are pass, with the exception of this one.
If we're adding the same rule with block, I think it makes sense to change 920340 to pass as well.

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jun 10, 2018

ACK

@lifeforms

This comment has been minimized.

Copy link
Collaborator

lifeforms commented Jun 10, 2018

Action on 920340 set to pass. Comments and message of 920341 updated.

@dune73

This comment has been minimized.

Copy link
Collaborator

dune73 commented Jun 10, 2018

Thank you @lifeforms. Ready to merge AFAICS.

@lifeforms lifeforms merged commit fb26da2 into SpiderLabs:v3.1/dev Jul 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment