Fix Cookie header parsing issues

[martinhsv]

This pull request includes airween's fixes plus the additional issue identified yesterday

[airween]

Hi @martinhsv, I saw your addition part of transaction.cc, but I don't see the point. I assume you want to remove the trailing spaces, but (may be I'm wrong) I don't understand why do you do that only in case of last cookie?

And I think that method does not have effect if the cookie is like this:

Cookie: foo=bar; baz=foo ;

Also note, that the reason why I didn't adopted this feature, that the existing regression tests contains the trim action for the checked variable, so I assume the original intention was to keep the cookie as is, and the rule writer can decide that is it allowed or not.

The leading spaces (in case of all keys) is justifiable from my point, because the modsecurity2 does it, and I tried to align the method like that.

What do you think about it?

[martinhsv]

The reason is this portion of the grammar: 'cookie-header = "Cookie:" OWS cookie-string OWS'.

The OWS (optional whitespace) can occur after the last cookie-pair (and not, for example, between 'baz=foo' and ';' as in your example).

You are correct that this extra change is at least somewhat less urgent than a portion of your fix, since a rule writer could manage this by always using t:trim for cookie values. However, I would argue that that is not the correct approach. Since the RFC explicitly defines whitespace at the end of the cookie header as not being part of the final value, we should not make it the responsibility of the rule writer to finish the parsing correctly.

[airween]

The reason is this portion of the grammar: 'cookie-header = "Cookie:" OWS cookie-string OWS'.

but - in my opinion - then it would be better to place it before the ssplit() method, with argument value. I think it would be more clear for an external viewer.

The OWS (optional whitespace) can occur after the last cookie-pair (and not, for example, between 'baz=foo' and ';' as in your example).

Sure - if you see my previous patch (#2023), then you can see I also reviewed the RFC.

And if you see my example above, you can see the spaces after the last ;. That means if the cookie header looks like that, then the optional whitespaces will remain until the process. But it no matter :).

You are correct that this extra change is at least somewhat less urgent than a portion of your fix, since a rule writer could manage this by always using t:trim for cookie values. However, I would argue that that is not the correct approach. Since the RFC explicitly defines whitespace at the end of the cookie header as not being part of the final value, we should not make it the responsibility of the rule writer to finish the parsing correctly.

Just my 2 cents: I don't want to argue, but would like to discuss. :)

[martinhsv]

Re 'before the ssplit()': Yes, that was my first impulse. But, I didn't want to remove the const-ness of value and preferred not to make yet another copy just for this. (That doesn't mean my choice of implementation is the only correct/reasonable choice, of course.)

And, no worries, such discussion seems entirely reasonable to me.

[dune73]

This is a apparently replacing PR #2201 (which looks reasonable to me).

Can you please give us a timeline for the security release fixing these issues in the libModSecurity3 release line?

[zimmerle]

Merged! Thanks to everybody involved in the issue.

[theMiddleBlue]

Thank you all! 🎉 @zimmerle, @martinhsv are you planning to publish a new release version? Just to know if we can inform our users to upgrade.

[zimmerle]

Hi @theMiddleBlue,

You can inform your users to upgrade, the published code on v3/master is passing all the tests. More updates to came. Thank you for your concern.

[carnil]

Is CVE-2019-19886 relating to this issue?

[airween]

Yes. This is.

[carnil]

@airween, thanks for confirming!