Replace Cookie parsing method #2201
Conversation
|
I made this patch again to fix the cookie parsing issues, and closed the #2023. Special thanks for the help to @theMiddleBlue and @fgsch. |
|
Thanks @airween Due to the criticality, is it possible to merge this PR as soon as possible and create a new "v3.0.something" release? |
src/transaction.cc
Outdated
| ckey = c.substr(0, pos); | ||
| // value will contains the next '=' chars if exists | ||
| // eg. foo=bar=baz -> key: foo, value: bar=baz | ||
| cval = c.substr(pos+1, c.length()); |
There was a problem hiding this comment.
The 2nd argument is not required here; it's more straightforward to let the 2nd arg default.
src/transaction.cc
Outdated
| // if the key is empty (eg: "Cookie: =bar;" skip it | ||
| if (ckey.length() == 0) { | ||
| localOffset = localOffset + c.length() + 1; | ||
| continue; |
There was a problem hiding this comment.
The flow here is more complicated than it needs to be. Consider amending as follows:
- move the left-trimming while loop (currently lines 580-583) to be right after line 576
- follow that with a single 'if (ckey.empty()) {' where the 'if' block has what's currently at line 587 and the 'else' block has the 591-598 content.
There was a problem hiding this comment.
I don't see any logic problems (and I definitely like that this addresses the ';;' use case!)
I have made a few suggestions below for your consideration. I'll defer to @zimmerle as to whether any of them should be considered necessary.
There was a problem hiding this comment.
Hi @martinhsv, thanks for the suggestions. As you can see, I modified all of them. The regression tests are passed at my side, hope you will allow this patch.
|
@airween, as you may figure having a commit with the name "Changed @martinhsv remarks" in the ModSecurity tree is not elegant. As stated before, please do not pull request changes on your own pull request. Make the fix in your own commit. |
|
@theMiddleBlue the patch is marked to be part of the effort towards the upcoming release. |
|
Thanks @zimmerle, I understand and I really appreciate all efforts. This PR addresses multiple vulnerabilities and one of them really critical. That's why I see the need for a security release, what do you think about? 3.1.0 is going to be released soon? otherwise, I think that all 3.0 users should patch asap. |
airween
force-pushed
the
v3/cookieparsefix
branch
from
e14f3dc to
11bf349
Compare
|
hi @zimmerle,
I'm sorry, I just wanted to keep track of every step what I made. I think it's more clear when I make a separated commit which contains the modification for the suggestions. Anyway, I revoked the last commit, and added the changes to the previous commit (with Hope now this will be appropriate. |
|
@zimmerle if we can meet in a private slack chat, we can show you why this patch is critical and why we think that a security release should be published. I don't want to share more details in a public PR :) |
airween
force-pushed
the
v3/cookieparsefix
branch
from
11bf349 to
c7ad6c7
Compare
|
Hi @theMiddleBlue, Just as an FYI, if you have sensitive, security-related concerns, one way to communicate these is to use the method mentioned in https://github.com/SpiderLabs/ModSecurity under the 'Security issue' heading. |
|
Hi @martinhsv I just sent an e-mail with all the details. If you want, and if it helps to accelerate a bit, with @airween we can discuss it on Slack. |
Thank you @theMiddleBlue. I saw that you guys are already feeding @martinhsv with information. |
This PR fixes three issues in libmodsecurity3:
Cookie: foowill produces a cookie with namefoo; the old method skipped it=, eg.Cookie: foo=bar=something interestingwill produces a cookie with namefooand valuebar=something interesting; the old method produced the value onlybar