This repository has been archived by the owner on Jan 30, 2020. It is now read-only.
Fix invalid exception from urlencoded cookie value #23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -302,7 +302,7 @@ public function getName() | |||
| */ | |||
| public function setValue($value) | |||
| { | |||
| HeaderValue::assertValid($value); | |||
| HeaderValue::assertValid(urlencode($value)); | |||
There was a problem hiding this comment.
When this won't be valid? if there no case exists then remove whole line.
zerocrates
force-pushed
the
setcookie-urlencode
branch
from
96290bd
to
83bcfc1
Compare
|
I see, yeah that's a much better solution. I've replaced my initial commit with one that follows your suggestions. |
| $header = new SetCookie("leo_auth_token", "example\r\n\r\nevilContent"); | ||
| $this->assertTrue(HeaderValue::isValid($header->getFieldValue())); |
There was a problem hiding this comment.
I preffer if you assertEquals against the __toString() output. Will be more clear to understand because the encoded characters will be visible while reading this code.
There was a problem hiding this comment.
Could you add an additional test method for unit check of setValue?
|
Okay. (I assume the extra test method is what you were looking for, if not let me know) |
|
@weierophinney Please add this one to 2.4.8 LTS |
weierophinney
added a commit
that referenced
this pull request
Sep 14, 2015
Fix invalid exception from urlencoded cookie value
weierophinney
added a commit
that referenced
this pull request
Sep 14, 2015
weierophinney
added a commit
that referenced
this pull request
Sep 14, 2015
|
Tagged in release-2.5.2 and 2.4.8. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is my alternative solution to #15, as a replacement for the existing PR #16.
This PR fixes the problem by simply re-urlencoding the cookie value before validating it. This is allowable because the actual cookie as output also urlencodes the value (see SetCookie::getFieldValue.
As a concrete example, this PR removes the test
testPreventsCRLFAttackViaConstructor. The change makes that test fail, but the test was already failing for an input that wouldn't have actually enabled a CRLF attack. The "evil" example is:However, under both the existing code and after the commit in this PR, that SetCookie header creates no CRLF injection problems, because SetCookie already automatically urlencodes values on output. So, the output of
$header->toString()on the above "evil" cookie is already safe:I think this change is the best way to fix the regression without compromising the security the validation was introduced to provide and without surprising users by changing the header's behavior.
Fix #15
Supersede & close #16