Skip to content
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

Aligned Cookie parsing method to ModSecurity2 style #2023

Closed
wants to merge 6 commits into from

Conversation

@airween
Copy link
Contributor

airween commented Feb 11, 2019

May be it's not a bug, but the behavior is differ from ModSec v2. Here is an example, take a look to this header request:

Cookie: foo
This is invalid, because the RFC allows the Cookie as like this:

cookie-header = "Cookie:" OWS cookie-string OWS
cookie-string = cookie-pair *( ";" SP cookie-pair )
...
cookie-pair       = cookie-name "=" cookie-value
cookie-name       = token
cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
...
cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash
token             = <token, defined in [RFC2616], Section 2.2>

But ModSec2 interprets this as there is a cookie with name "foo" with empty value.

This patch aligns the behavior of ModSec3.

@victorhora victorhora self-assigned this Feb 11, 2019
@victorhora victorhora requested review from victorhora and zimmerle Feb 11, 2019
@victorhora victorhora added this to the v3.0.4 milestone Feb 11, 2019
@victorhora victorhora added this to In progress in v3.0.4 via automation Feb 11, 2019
@airween

This comment has been minimized.

Copy link
Contributor Author

airween commented Feb 11, 2019

Note, that this bug triggered by this CRS test.

@@ -571,6 +571,14 @@ int Transaction::addRequestHeader(const std::string& key,
m_variableRequestCookies.set(s[0], s[1], localOffset);
localOffset = localOffset + s[1].size() + 2;
}
else {

This comment has been minimized.

Copy link
@zimmerle

zimmerle Mar 11, 2019

Member

Maybe we want to check if it is bigger than 0. Just to avoid a memory access violation.

This comment has been minimized.

Copy link
@airween

airween Mar 11, 2019

Author Contributor

But if we checked pos1 > 0, then what if the Cookie header starts with '='?

Should be check pos >= 0?

v3.0.4 automation moved this from In progress to Needs review Mar 11, 2019
@zimmerle

This comment has been minimized.

Copy link
Member

zimmerle commented Mar 11, 2019

Hi @airween, I remember that we were discussing this issue over hangout a few days ago. Are the changes ready?

Copy link
Contributor Author

airween left a comment

I didn't modified the affected code since, just make a small program to demonstrate the differences between old method and new one. Here is it:

https://gist.github.com/airween/efb8a737193910b5ae893d93e0325902

@@ -571,6 +571,14 @@ int Transaction::addRequestHeader(const std::string& key,
m_variableRequestCookies.set(s[0], s[1], localOffset);
localOffset = localOffset + s[1].size() + 2;
}
else {

This comment has been minimized.

Copy link
@airween

airween Mar 11, 2019

Author Contributor

But if we checked pos1 > 0, then what if the Cookie header starts with '='?

Should be check pos >= 0?

@airween

This comment has been minimized.

Copy link
Contributor Author

airween commented Jun 1, 2019

I've merged the newest version from upstream to this branch.

Could we continue the discussion about the patch? First, please review the snippet:
https://gist.github.com/airween/efb8a737193910b5ae893d93e0325902

The C++ code contains the old (current) and the new method of cookie parsing. You can pass the cookie string to the compiled binary as argument (see output.txt).

The first line is the output of current version, the second is the last product. The ^ markers showed, how catches the algorithm the cookie names and its values. You can see, if the cookie string is "not well-formed", the parsing is wrong with the current mechanism, for eg. the parsed cookie value is just a substring(!), eg. ange instead of orange, or a cookie has two values (or I don't know, how interprets it), eg. bar=a and pple instead of bar=apple.

@zimmerle zimmerle removed this from Needs review in v3.0.4 Oct 15, 2019
@zimmerle zimmerle added this to In progress in v3.1.0 via automation Oct 15, 2019
@airween airween closed this Nov 13, 2019
v3.1.0 automation moved this from In progress to Done Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v3.1.0
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.