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.