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

Replace Cookie parsing method #2201

Closed
wants to merge 1 commit into from

Conversation

@airween
Copy link
Contributor

airween commented Nov 13, 2019

This PR fixes three issues in libmodsecurity3:

  • aligns the cookie parsing for v2: allows the cookie without value, eg Cookie: foo will produces a cookie with name foo; the old method skipped it
  • processes the whole text after the first =, eg. Cookie: foo=bar=something interesting will produces a cookie with name foo and value bar=something interesting; the old method produced the value only bar
  • fix the offset calculation for the logfiles
@airween

This comment has been minimized.

Copy link
Contributor Author

airween commented Nov 13, 2019

I made this patch again to fix the cookie parsing issues, and closed the #2023.

Special thanks for the help to @theMiddleBlue and @fgsch.

@theMiddleBlue

This comment has been minimized.

Copy link

theMiddleBlue commented Nov 13, 2019

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 Show resolved Hide resolved
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());

This comment has been minimized.

Copy link
@martinhsv

martinhsv Nov 13, 2019

Member

The 2nd argument is not required here; it's more straightforward to let the 2nd arg default.

This comment has been minimized.

Copy link
@airween

airween Nov 13, 2019

Author Contributor

Changed in e14f3dc.

// if the key is empty (eg: "Cookie: =bar;" skip it
if (ckey.length() == 0) {
localOffset = localOffset + c.length() + 1;
continue;

This comment has been minimized.

Copy link
@martinhsv

martinhsv Nov 13, 2019

Member

The flow here is more complicated than it needs to be. Consider amending as follows:

  1. move the left-trimming while loop (currently lines 580-583) to be right after line 576
  2. 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.

This comment has been minimized.

Copy link
@airween

airween Nov 13, 2019

Author Contributor

Changed in e14f3dc.

Copy link
Member

martinhsv left a 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.

Copy link
Contributor Author

airween left a 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.

@zimmerle

This comment has been minimized.

Copy link
Member

zimmerle commented Nov 13, 2019

@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.

@zimmerle

This comment has been minimized.

Copy link
Member

zimmerle commented Nov 13, 2019

I understand that this pull request is a continuation of #2023; Since #2023 was on the priority list, I am passing this to the priority list. Wating for the author to continue the review.

@zimmerle zimmerle self-assigned this Nov 13, 2019
@zimmerle zimmerle added the 3.x label Nov 13, 2019
@zimmerle zimmerle added this to In progress in v3.1.1 via automation Nov 13, 2019
@zimmerle zimmerle added this to the v3.1.0 milestone Nov 13, 2019
@zimmerle

This comment has been minimized.

Copy link
Member

zimmerle commented Nov 13, 2019

@theMiddleBlue the patch is marked to be part of the effort towards the upcoming release.

@theMiddleBlue

This comment has been minimized.

Copy link

theMiddleBlue commented Nov 13, 2019

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 airween force-pushed the airween:v3/cookieparsefix branch from e14f3dc to 11bf349 Nov 13, 2019
@airween

This comment has been minimized.

Copy link
Contributor Author

airween commented Nov 13, 2019

hi @zimmerle,

@airween, as you may figure having a commit with the name "Changed @martinhsv remarks" in the ModSecurity tree is not elegant.

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 amend).

Hope now this will be appropriate.

@theMiddleBlue

This comment has been minimized.

Copy link

theMiddleBlue commented Nov 14, 2019

@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 airween force-pushed the airween:v3/cookieparsefix branch from 11bf349 to c7ad6c7 Nov 14, 2019
@martinhsv

This comment has been minimized.

Copy link
Member

martinhsv commented Nov 15, 2019

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.

@theMiddleBlue

This comment has been minimized.

Copy link

theMiddleBlue commented Nov 15, 2019

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.

@zimmerle

This comment has been minimized.

Copy link
Member

zimmerle commented Nov 18, 2019

@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 :)

Thank you @theMiddleBlue. I saw that you guys are already feeding @martinhsv with information.

@zimmerle

This comment has been minimized.

Copy link
Member

zimmerle commented Nov 20, 2019

Closing this on the favor of #2202 --- #2202 includes @airween fixes.

@zimmerle zimmerle closed this Nov 20, 2019
v3.1.1 automation moved this from In progress to Done Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v3.1.1
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

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