bottlepy / bottle

bottle.py is a fast and simple micro-framework for python web-applications.
http://bottlepy.org/
MIT License
8.33k stars 1.46k forks source link

Relevant commits for CVE-2020-28473 #1331

Closed utkarsh2102 closed 3 years ago

utkarsh2102 commented 3 years ago

Hi @defnull,

I'm trying to backport the patch for CVE-2020-28473 to v12.7 and v12.13 (as a part of Debian LTS and ELTS) and thus I am trying to get relevant commits for doing so.

As I see https://github.com/bottlepy/bottle/compare/0.12.18...0.12.19, I am trying to figure what should be enough to backport to truly fix the CVE? Could you, therefore, point me to those commits?

defnull commented 3 years ago

It is just https://github.com/bottlepy/bottle/commit/57a2f22e0c1d2b328c4f54bf75741d74f47f1a6b really.

As an explanation: Bottle (as many other frameworks) followed an old standard that allowed ; in addition to & as a query parameter separator. A hypothetical caching proxy (in front of bottle) that is configured to cache based on specific query parameters (instead of the entire request url as usual) might interpret query parameters differently than bottle. A malicious user could in theory create a request with specific query parameters that is then cached and served to a target user, even if the target user submits different parameter values.

I have no idea on how to actually exploit this, really, and I disagree with the classification as a security vulnerability in Bottle. An exploit would require a strangely configured caching proxy and an application that is vulnerable in a very very specific way. Both are outside of the control or responsibility of a web framework. I'd really be interested in a real-world example... But since ; as a separator is no longer a standard, and very rarely used in real-world nowadays, it was easy to fix.

utkarsh2102 commented 3 years ago

Hello 👋🏻

It is just 57a2f22 really.

Aah, thanks! :)

As an explanation: Bottle (as many other frameworks) followed an old standard that allowed ; in addition to & as a query parameter separator. A hypothetical caching proxy (in front of bottle) that is configured to cache based on specific query parameters (instead of the entire request url as usual) might interpret query parameters differently than bottle. A malicious user could in theory create a request with specific query parameters that is then cached and served to a target user, even if the target user submits different parameter values.

I have no idea on how to actually exploit this, really, and I disagree with the classification as a security vulnerability in Bottle. An exploit would require a strangely configured caching proxy and an application that is vulnerable in a very very specific way. Both are outside of the control or responsibility of a web framework. I'd really be interested in a real-world example... But since ; as a separator is no longer a standard, and very rarely used in real-world nowadays, it was easy to fix.

That does make sense! However, though you'd know, the POC is available at https://snyk.io/vuln/SNYK-PYTHON-BOTTLE-1017108 (and the backstory in the first paragraph of https://snyk.io/blog/cache-poisoning-in-popular-open-source-packages/).

That said, even though it has been classified as a security vulnerability, it has a lower CVSS score (of 5.9). So hopefully it's not something very exploitable but for us, it's always safer to backport this to older releases of Debian! :)

Either way, thank you so much for your help! ❤️

defnull commented 3 years ago

I know the POC and the reasoning behind the blog post (we discussed this topic with synk) but my issue is with the wording:

To conclude, this research shows that open source frameworks are vulnerable to web cache poisoning attacks almost regardless of the proxy being used (excluding some cases). While it is possible to mitigate these attacks at the proxy-level, many developers are not aware of these attack vectors and are not implementing the required safeguards at the cache/proxy level.

This sounds like any application using bottle and a caching proxy is "vulnerable" by default. That is not true. There are two very important and also IMHO unlikely requirements that are only mentioned very vague or simply assumed to be given by the blog article and CVE:

1) The application itself must be vulnerable. It must produce cache-able output triggered by an unauthorized GET request that is in some way harmful to the target user. To achieve a stored XSS using cache poisoning for example, there must be an XSS vulnerability in the application in the first place. 2) And even more importantly, the caching proxy must be configured to use or not use specific query parameters instead of the entire query string as cache keys. This is NOT the default configuration for nginx or varnish (correct me if I am wrong).

The whole issue feels fabricated and a bit clickbaity to be honest. This CVE is not about a vulnerability in bottle, it is about a quirk that happens to be part of a complex and somewhat theoretical attack on a (purely hypothetical) application that is vulnerable in itself and also sits behind a misconfigured caching proxy. This could have been a simple bug report, but there is no fun in that I suppose.

utkarsh2102 commented 3 years ago

Hi @defnull,

The whole issue feels fabricated and a bit clickbaity to be honest. This CVE is not about a vulnerability in bottle, it is about a quirk that happens to be part of a complex and somewhat theoretical attack on a (purely hypothetical) application that is vulnerable in itself and also sits behind a misconfigured caching proxy. This could have been a simple bug report, but there is no fun in that I suppose.

I hear you. I really do. Do you think you want to apply for rejection of this CVE?

defnull commented 3 years ago

No, that's be unfair on my side. There was a lot of communication with Snyk, and I acknowledge this issue by patching it. There was non-standard behavior in bottle that allowed this scenario. So, the CVE or article are not untrue, just far-fetched and strangely worded (IMHO). Also, the CVE is out already. All the people that have to burn time on this already burned it. People searching for it will find this issue and get the full story. That's fine.

We discussed other attach scenarios as well. There were even less related to bottle, I expressed my concerns, and they did not release a CVE. So, Snyk was communicative and professional, you have to give them that. For the curious: One of them involved "parameter cloaking" if a GET request contained form-data in its body, and the application used request.params instead of request.query to access query parameters. Form parameters override query parameters in this scenario. My objection was that request.params is clearly documented as containing both query and form parameters, GET requests are indeed allowed to have a body so bottle conforms to RFC, and request.query exists if you do not want that.

I am so annoyed by this because Bottle now has three CVEs attached to it. All three are based on attack scenarios that involve a vulnerable application (plus a small quirk in bottle), but there were not filed against the vulnerable applications. Instead, there were filed against a web framework that does not magically prevent developers from writing vulnerable applications. I disagree with this kind of CVEs, but they are common. There is a whole industry based in CVEs, and even websites that award points to 'security researchers' based on published CVEs. I's a game and a business now. The meaning of CVEs is watered down (IMHO).

Again, I am tankful that Snyk contacted me, that they were communicative and the process was very professional. They listened, I listened, and now there is a CVE that I would not have issued, but I can accept that they did. I'm disappointed by the state of security research and CVE handling in general, not by Snyk in particular. Comparing the three CVEs for bottle, Snyk handled it the best. No issues there.

utkarsh2102 commented 3 years ago

Aha, thanks for filling me/us in here! I get it, your PoV and reasoning behind this and it makes sense, really.

However, what's done is done and hopefully next time onward, there'll be at least a lesser number of such incidents (if not none).

Either way, thank you for your work and handling this! \o/