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

redirect() doesn't filter "\r\n" leads to CRLF attack #913

Closed
5alt opened this issue Dec 8, 2016 · 11 comments

Comments

Projects
None yet
5 participants
@5alt
Copy link

commented Dec 8, 2016

Hi,

redirect() doesn't filter "\r\n" which leads to CRLF attack.

For example, I use redirect("233\r\nSet-Cookie: name=salt") can set a new cookie in the client side.

:P

@eric-wieser

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2016

Is this a flaw present when setting any header?

@5alt

This comment has been minimized.

Copy link
Author

commented Dec 8, 2016

Yes, response.set_header function is also affected

@defnull

This comment has been minimized.

Copy link
Member

commented Dec 8, 2016

It might be a good idea to check against illegal characters in redirect(), but I'm not so sure about Response.set_header() and friends. Control characters are not allowed in header values, and if you put them in anyway, things will break. Bottle should be allowed to trust the application enough to not intentionally break HTTP or WSGI standards. If untrusted user input ends up in a header field, then that's a serious flaw in the application.

@5alt

This comment has been minimized.

Copy link
Author

commented Dec 9, 2016

I just tested webpy and flask, they raise an exception when there are invalid characters in header. If developer want to set two response headers, he should call Response.set_header() twice instead of using \r\n

@defnull defnull added the Accepted label Dec 9, 2016

@defnull

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

On a second thought, a simple check for '\n' won't hurt much.

defnull added a commit that referenced this issue Dec 10, 2016

fix #913: Harden bottle against malformed headers.
Bottle now checks against certain control characters (\n, \r and \0) in header names or values and raises a ValueError if the application tries to set an invalid header.

@defnull defnull closed this in 6d7e13d Dec 10, 2016

@FedericoCeratto

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2016

Related to CVE-2016-9964

@defnull using the redirect shown by 5alt the header value check is not being performed.
BaseResponse.set_header() is setting str(value) into a regular dictionary:
https://github.com/bottlepy/bottle/blob/master/bottle.py#L1737

defnull added a commit that referenced this issue Dec 17, 2016

defnull added a commit that referenced this issue Dec 17, 2016

@defnull

This comment has been minimized.

Copy link
Member

commented Dec 17, 2016

Oh, yes, I forgot that BaseResponse bypasses HeaderDict if it's not needed. Fixed.

I'm a bit surprised that this spawned a CVE. Bottle on its own is not vulnerable. You need a (very) vulnerable application to trigger this behavior. If you pass untrusted user-input to library functions then it should not be a surprise that bad thinks might happen.

@defnull defnull reopened this Dec 17, 2016

@defnull

This comment has been minimized.

Copy link
Member

commented Dec 17, 2016

I'll re-open until the release with the fix is published.

defnull added a commit that referenced this issue Dec 17, 2016

Release of 0.12.11
* fix #913: redirect() doesn't filter "\r\n" leads to CRLF attack (CVE-2016-9964)
  #913
@defnull

This comment has been minimized.

Copy link
Member

commented Dec 17, 2016

Done :) https://pypi.python.org/pypi/bottle/0.12.11

Thanks for the report and help.

@defnull defnull closed this Dec 17, 2016

buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this issue Dec 21, 2016

python-bottle: security bump to 0.12.11
"\r\n" sequences were not properly filtered when handling redirections.
This allowed an attacker to perform CRLF attacks such as HTTP header
injection:

bottlepy/bottle#913

Python-bottle now uses setuptools instead of distutils.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
@sairon

This comment has been minimized.

Copy link

commented Dec 21, 2016

Btw. this issue has been reported almost three years ago in #573 without any official response.

@defnull

This comment has been minimized.

Copy link
Member

commented Dec 21, 2016

I still agree with @RonRothman on that old issue, and his response was probably the reason why I did not feel the need to respond 'officially':

Not sure I agree that this should be addressed at the framework level. It might just give developers a false sense of security. Better to have the web app explicitly sanitize /all/ user input (not just headers), and keep the separation of responsibilities clear.

In the end, the additional check was easy to do and the overhead does not hurt that much. The CVE was still unnecessary IMHO, as this is not a bug in bottle, but in a hypothetical application using bottle in an notoriously wrong way. You would not issue a CVE for an SQL server because it happily accepts SQL injected queries either.

woodsts pushed a commit to woodsts/buildroot that referenced this issue Dec 21, 2016

python-bottle: security bump to 0.12.11
"\r\n" sequences were not properly filtered when handling redirections.
This allowed an attacker to perform CRLF attacks such as HTTP header
injection:

bottlepy/bottle#913

Python-bottle now uses setuptools instead of distutils.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
(cherry picked from commit aa64e33)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.