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

Cookie name CRLF injection #859

Closed
vlet opened this issue Nov 15, 2012 · 12 comments
Closed

Cookie name CRLF injection #859

vlet opened this issue Nov 15, 2012 · 12 comments

Comments

@vlet
Copy link

vlet commented Nov 15, 2012

Today was announced new CGI.pm 3.63 were was fixed issue (marked as
security issue in Changes) with CRLF injection in cookies:
https://github.com/markstos/CGI.pm/pull/23

As I can see Dancer::Cookie also do not validate cookie name for
CRLF and other invalid symbols in headers. This may be a security issue if
cookie name taken from untrusted source.

$ perl -MDancer -e 'get "/" => sub { cookie "test\n\rX-Evil-Header: " => "evil" };dance' &

$ echo "GET / HTTP/1.0\r\n\r\n" | netcat 127.0.0.1 3000

HTTP/1.0 200 OK
Server: Perl Dancer 1.311
Content-Length: 4
Content-Type: text/html
Set-Cookie: test
X-Evil-Header: =evil; path=/; HttpOnly
X-Powered-By: Perl Dancer 1.311

evil

@vlet
Copy link
Author

vlet commented Nov 27, 2012

Continue

Dancer::Cookie also do not validate values of options of cookie() "path", "expires" and "domain" for invalid characters.
Moreover if value of cookie is utf8 string cookie() will crash ( uri_escape() die if symbol code > 255)

header(), headers() and push_header() functions have same problem with invalid characters in name and value.
Header value validated for CRLF in Dancer::Response. headers_to_array() adds space after first CRLF (multiline header body allowed with TAB or SPACE in the beginig of each line). But if there are two or more CRLF in a header body than you have a problem...

As side effect was noticed, that utf8 string in header value can crash web-server HTTP::Server::PSGI

@ppisar
Copy link

ppisar commented Nov 27, 2012

Actually the first new line gets escaped. But you cannot see it on terminal because the two spaces are followed by \r from input value.

@bigpresh
Copy link
Member

Hmm, yes, this stuff needs fixing.

I see most of it as reasonably low-impact, as it's fairly unlikely that the value comes from user input for most of them; header values and cookie values are more of a concern, though.

If I get a moment today I'll try to implement fixes for these problems (unless someone else gets there first :) )

@bigpresh bigpresh reopened this Nov 27, 2012
@bigpresh
Copy link
Member

(Whoops, closed this one by accident, thought I was looking at a different issue. That'll teach me to pay attention and slow down until my caffeination level has increased.)

@ppisar
Copy link

ppisar commented Nov 27, 2012

This flaw has been assigned CVE-2012-5572 identifier (http://www.openwall.com/lists/oss-security/2012/11/26/10).

@xsawyerx
Copy link
Member

This blocks the next release.

@bigpresh
Copy link
Member

This blocks the next release.

Agreed! Needs a fix ASAP.

@carnil
Copy link

carnil commented Jan 5, 2013

Hi

Are there news on this issue?

Thanks for your work!

@onur
Copy link

onur commented May 20, 2013

did you abandon this issue?

@yanick
Copy link
Contributor

yanick commented May 20, 2013

No, it is still in the queue. We'll get to it eventually, I swear. :-)

@ppisar
Copy link

ppisar commented Jun 4, 2013

Just released version 1.3114 is missing test t/12_response/11_CVE-2012-5572.t added in commit:

commit d21a0983fa95ffea2b50ad5af84cc93f4ce5f4d2
Author: Colin Keith <colinmkeith@gmail.com>
Date:   Sat May 25 00:46:53 2013 -0400

    test and resolution for CVE-2012-5572, \r\n sequence being allowed in a cookie name fixes PerlDancer/Dancer#859

yanick added a commit that referenced this issue Jun 4, 2013
@yanick
Copy link
Contributor

yanick commented Jun 4, 2013

Ooops, forgot to update the MANIFEST. Did that now, test is going to pop up in next release.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants