Tornado: Vulnerability CVE-2020-28476 detected by PyUp's Safety scan

[MartinFalatic]

Safety (https://pyup.io/safety/) alerted us to the following vulnerability today:

╘══════════════════════════════════════════════════════════════════════════════╛
│ tornado                    │ 6.0.4     │ <=6.1.0                  │ 39462    │
╞══════════════════════════════════════════════════════════════════════════════╡
│ All versions of package tornado are vulnerable to Web Cache Poisoning by     │
│ using a vector called parameter cloaking. When the attacker can separate     │
│ query parameters using a semicolon (;), they can cause a difference in the   │
│ interpretation of the request between the proxy (running with default        │
│ configuration) and the server. This can result in malicious requests being   │
│ cached as completely safe ones, as the proxy would usually not see the       │
│ semicolon as a separator, and therefore would not include it in a cache key  │
│ of an unkeyed parameter. See CVE-2020-28476.                                 │
╘══════════════════════════════════════════════════════════════════════════════╛

No fixed version exists currently.

This tracks to https://nvd.nist.gov/vuln/detail/CVE-2020-28476 which leads us to https://snyk.io/vuln/SNYK-PYTHON-TORNADO-1017109 , which has a more detailed analysis, pasted below for convenience:

Overview
Affected versions of this package are vulnerable to Web Cache Poisoning by using a vector called parameter cloaking. When the attacker can separate query parameters using a semicolon (;), they can cause a difference in the interpretation of the request between the proxy (running with default configuration) and the server. This can result in malicious requests being cached as completely safe ones, as the proxy would usually not see the semicolon as a separator, and therefore would not include it in a cache key of an unkeyed parameter.

PoC

    GET /?q=legitimate&utm_content=1;q=malicious HTTP/1.1

    Host: example.com

    Upgrade-Insecure-Requests: 1        

    User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36

    Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,imag e/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9 Accept-Encoding: gzip, deflate            

    Accept-Language: en-US,en;q=0.9 Connection: close

The server sees 3 parameters here: q, utm_content and then q again. On the other hand, the proxy considers this full string: 1;q=malicious as the value of utm_content, which is why the cache key would only contain somesite.com/?q=legitimate.

[bdarnell]

This CVE is mis-assigned - the relevant code is in the python standard library, not Tornado.

I'd also argue that this kind of issue is fundamentally a problem of a cache that makes bad assumptions about how the backend handles requests (as in this post discussing the issue as a cache implementation flaw).

they can cause a difference in the │
│ interpretation of the request between the proxy (running with default │
│ configuration) and the server.

Which proxies behave like this in their default configuration?

[MartinFalatic]

So it's clear, I was not involved in creating or assigning the CVE, I just noticed it and wanted to make sure you were aware it exists since I saw no other tickets here about it. According to the NIST CVE it's still under analysis - Snyk has a blog post concerning their methodology (I take it they concocted tests and started opening CVEs).

[friedcell]

The related blog post is a good read, but I think the only thing Tornado could do is to discourage use of methods that can lead to security issues (eg. use get_query_argument vs get_argument) and possibly limit access to request body on get requests (if I understand the article, that's already done, but not released yet).

Quick test of the body override on get (sorry for the old coroutine based code):

@gen.coroutine
def get(self):
    sleep = self.get_argument("sleep", 0)
    yield gen.sleep(float(sleep))

def test_get_with_body(self):
    t0 = time.monotonic()
    response = self.fetch("/test?sleep=1", method="GET", body="sleep=2".encode("utf-8"), headers={"Content-Type": "application/x-www-form-urlencoded"}, allow_nonstandard_methods=True)
    self.assertGreater(time.monotonic() - t0, 1)
    self.assertLess(time.monotonic() - t0, 2)  # fails, takes more than 2 seconds

[bdarnell]

Note that there are two separate issues in the snyk blog post: semicolons as URI parameter delimiters and "fat GETs". Only the first one is covered by CVE-2020-28476. I'm not sure why; the "fat GET" issue is more properly considered misbehavior on the part of Tornado (it's misbehavior that was explicitly requested by multiple users, but I agree we need to get rid of it).

possibly limit access to request body on get requests (if I understand the article, that's already done, but not released yet).

It's not done yet, but I will probably introduce a flag like the allow_nonstandard_methods one on the HTTP clients to make "fat GET" support opt-in.

[debellotti]

Hi guys, any update on this?

[bdarnell]

The issue identified by CVE-2020-28476 is in the python standard library, not Tornado, so any updates will be in https://bugs.python.org/issue42967 and not here. I've never interacted with the CVE system so I don't know how to get this attributed properly (@adamgold can you help with this?).

There is a separate issue around "fat GETs" which doesn't appear to have a CVE but is discussed in the snyk blog post. There will be changes to address this in a future release of Tornado but there is no timeline for this (it does not, IMO, warrant an emergency security release).

If you are affected by web cache poisoning issues you should address this primarily by looking at your cache - any caching proxy which forwards information to the backend which is not to be used in the construction of the response is inherently vulnerable to poisoning; this should be fixed in the cache to block the entire class of vulnerabilities rather than expecting absolute consistency of behavior across all possible backends. For example, proxies that receive fat GETs should either reject those requests or incorporate the body into their cache key. For the semicolon issue it may be a little more complicated but the general idea is that the cache should transform the request url to its cache-key form before forwarding it to the backend.

[AdamGold]

Hey @bdarnell 😄

The CVE is not mis-assigned, we do think that Tornado should be using a different method for parsing the query args (see Werkzeug implementation for example). This is because once upstream Python is fixed, all implementations under Python 3.10 would still be vulnerable.

[bdarnell]

This is because once upstream Python is fixed, all implementations under Python 3.10 would still be vulnerable.

Conversely, when it is fixed in python 3.10, and the next security patch releases of 3.9.x, 3.8.x, etc, it will be fixed for everyone without any changes on the Tornado side.

Are you implying that the python security team does not intend to patch this change into older branches? If they do not accept this issue as a security bug requiring a backported fix, why should Tornado decide differently?

If tornado changes our code to not use parse_qsl but the vulnerable parse_qsl function still exists, we cannot guarantee that anyone's application is safe. (it's unlikely, but there is no guarantee that Tornado's URL parsing is the only time this function is used in a relevant context. For example, for anyone using Tornado's WSGIContainer, the URL parsing will be in some other WSGI package. Maybe that's one you've found, like Bottle. Maybe it's one you didn't check, or one that is closed source and invisible on the internet). It's insufficient to file CVEs against Tornado and Bottle without also filing one against the upstream source in python itself. (Is there such a CVE? I haven't found one but I'm not sure I'm looking in the right places)

There are two ways to solve the web cache poisoning problem: either every web server everywhere parses URLs in exactly the same way, or proxies code defensively and don't send backends information that is not a part of the cache key. Asking Tornado to reimplement URL parsing seems like a counterproductive effort towards the former goal. It may help push semicolon parsing out of existence faster, but in the long run it'd be another implementation that would have to be tested for conformance with the specs and consistency with the proxies.

[AdamGold]

Conversely, when it is fixed in python 3.10, and the next security patch releases of 3.9.x, 3.8.x, etc, it will be fixed for everyone without any changes on the Tornado side.

That's a fair point.

It's insufficient to file CVEs against Tornado and Bottle without also filing one against the upstream source in python itself. (Is there such a CVE? I haven't found one but I'm not sure I'm looking in the right places)

I absolutely agree. Python manages their own advisories and CVEs, one will come when they fix it I assume.

It may help push semicolon parsing out of existence faster, but in the long run it'd be another implementation that would have to be tested for conformance with the specs and consistency with the proxies.

I'm thinking about revoking the CVE as soon as Python publishes their own CVE. What do you think?

[AdamGold]

Python CVE has been published: https://snyk.io/vuln/SNYK-UPSTREAM-PYTHONCPYTHON-1074933
I revoked the Snyk advisory for Tornado and asked Mitre to revoke the CVE.

[bdarnell]

Thanks, Adam. I'm going to leave this issue open for a little while in case people are still searching for it until the CVE is revoked.

[leberknecht]

For the logs: i pinged pyup.io about this, got this answer:

We have now marked the vulnerability as not valid.
This will update immediately in our proprietary DB, but not until March 1st on our free DB.

:)