classification
Title: Cookie domain check returns incorrect results
Type: security Stage: patch review
Components: Library (Lib) Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Windson Yang, martin.panter, ned.deily, orsenthil, serhiy.storchaka, xtreak, 西田雄治
Priority: release blocker Keywords: patch, security_issue

Created on 2018-10-31 06:52 by 西田雄治, last changed 2019-01-03 08:01 by xtreak.

Pull Requests
URL Status Linked Edit
PR 10258 open xtreak, 2018-10-31 10:29
Messages (10)
msg328973 - (view) Author: 西田雄治 (西田雄治) Date: 2018-10-31 06:52
http.cookiejar.DefaultPolicy.domain_return_ok returns incorrect results.

So, HTTP clients send cookies which issued from wrong server.

policy = http.cookiejar.DefaultCookiePolicy()
req = urllib.request.Request('https://xxxfoo.co.jp/')
print(policy.domain_return_ok('foo.co.jp', req)   # should be False, but it returns True
msg328975 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-10-31 08:15
The current set of tests are at https://github.com/python/cpython/blob/0353b4eaaf451ad463ce7eb3074f6b62d332f401/Lib/test/test_http_cookiejar.py#L406 . A simple set of tuple that can be added based on the report as below : 

("http://barfoo.com", ".foo.com", False)
("http://barfoo.com", "foo.com", False) # Fails on master

The check is done at https://github.com/python/cpython/blob/0353b4eaaf451ad463ce7eb3074f6b62d332f401/Lib/http/cookiejar.py#L1176 . There is no check to add '.' before domain if absent. Hence it performs a substring match with the values req_host = ".barfoo.com" and erhn = ".barfoo.com" and domain = "foo.com" so the condition `not (req_host.endswith(domain) or erhn.endswith(domain))` fails and doesn't return False. I would suggest adding a check to make sure domain also starts with '.' similar to req_host and erhn thus fixing the issue. I tried the fix and existing tests along with the reported case works fine.

diff --git a/Lib/http/cookiejar.py b/Lib/http/cookiejar.py
index 0ba8200f32..da7462701b 100644
--- a/Lib/http/cookiejar.py
+++ b/Lib/http/cookiejar.py
@@ -1173,6 +1173,8 @@ class DefaultCookiePolicy(CookiePolicy):
             req_host = "."+req_host
         if not erhn.startswith("."):
             erhn = "."+erhn
+        if not domain.startswith("."):
+            domain = "."+domain
         if not (req_host.endswith(domain) or erhn.endswith(domain)):
             #_debug("   request domain %s does not match cookie domain %s",
             #       req_host, domain)

("http://barfoo.com", ".foo.com", False)
("http://barfoo.com", "foo.com", False) # Tests pass with fix

Also tried the script attached in the report

$ cat ../backups/bpo35121.py

import urllib
from http.cookiejar import DefaultCookiePolicy

policy = DefaultCookiePolicy()
req = urllib.request.Request('https://xxxfoo.co.jp/')
print(policy.domain_return_ok('foo.co.jp', req))

# without fix

$ ./python.exe ../backups/bpo35121.py
True

# With domain fix

$ ./python.exe ../backups/bpo35121.py
False

The check was added in 2004 with commit 2a6ba9097ee3942ae328befaf074ce9722b93ca0 . If my fix is correct I am willing to raise a PR for this with test.

Hope it helps!
msg328981 - (view) Author: 西田雄治 (西田雄治) Date: 2018-10-31 10:24
I think that is desired result. thanks!
msg328985 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-10-31 11:27
Thanks for the confirmation. I have created a PR (https://github.com/python/cpython/pull/10258) with test and added 3.8 as affected version. Please add in if I have missed anything in the PR.
msg329176 - (view) Author: Windson Yang (Windson Yang) * Date: 2018-11-03 02:12
I wonder https://github.com/python/cpython/blob/master/Lib/test/test_http_cookiejar.py#L420 

("http://foo.bar.com/", "com", True),
("http://foo.com/", "com", True),

are expected behavior?
msg329179 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-11-03 04:02
Good catch Windson! I overlooked the tests. There is also a comment that it's liberal in the test function. Since the code was added in 2006 I don't if it's ok broken to fix this or not. I will let the reviewers take a call then. There is also domain_match and user_domain_match that perform more tests.

> This is only a rough check for performance reasons, so it's not too critical as long as it's sufficiently liberal.

Thanks for your input!
msg332299 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-12-21 20:57
Looking further into this the domain validation makes it little more stricter and can have wider implications. For example requests library uses cookiejar to maintain cookies between sessions. One more case is that `domain` can be empty so only non-empty domains can be prefixed with dot.

A simple server that sets Cookie with value `A=LDJDSFLKSDJLDSF`

import SimpleHTTPServer
import logging

class MyHTTPRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler):
    def do_GET(self):
        self.cookieHeader = self.headers.get('Cookie')
        SimpleHTTPServer.SimpleHTTPRequestHandler.do_GET(self)

    def end_headers(self):
        self.send_my_headers()
        SimpleHTTPServer.SimpleHTTPRequestHandler.end_headers(self)

    def send_my_headers(self):
        self.send_header('Set-Cookie', 'A=LDJDSFLKSDJLDSF')

if __name__ == '__main__':
    SimpleHTTPServer.test(HandlerClass=MyHTTPRequestHandler)


Add below host entry to `/etc/hosts` 

127.0.0.1 test.com
127.0.0.1 1.test.com
127.0.0.1 footest.com


Sample script to demonstrate requests behavior change

import requests

with requests.Session() as s:
    cookies = dict(cookies_are='working')
    m = s.get("http://test.com:8000", cookies=cookies)
    print(m.request.headers)
    m = s.get("http://1.test.com:8000", cookies=cookies)
    print(m.request.headers)
    m = s.get("http://footest.com:8000", cookies=cookies)
    print(m.request.headers)


Before patch : 


{'User-Agent': 'python-requests/2.11.1', 'Accept-Encoding': 'gzip, deflate', 'Accept': '*/*', 'Connection': 'keep-alive', 'Cookie': 'cookies_are=working'}
{'User-Agent': 'python-requests/2.11.1', 'Accept-Encoding': 'gzip, deflate', 'Accept': '*/*', 'Connection': 'keep-alive', 'Cookie': 'A=LDJDSFLKSDJLDSF; cookies_are=working'}
{'User-Agent': 'python-requests/2.11.1', 'Accept-Encoding': 'gzip, deflate', 'Accept': '*/*', 'Connection': 'keep-alive', 'Cookie': 'A=LDJDSFLKSDJLDSF; cookies_are=working'}

After patch :


{'User-Agent': 'python-requests/2.11.1', 'Accept-Encoding': 'gzip, deflate', 'Accept': '*/*', 'Connection': 'keep-alive', 'Cookie': 'cookies_are=working'}
{'User-Agent': 'python-requests/2.11.1', 'Accept-Encoding': 'gzip, deflate', 'Accept': '*/*', 'Connection': 'keep-alive', 'Cookie': 'A=LDJDSFLKSDJLDSF; cookies_are=working'}
{'User-Agent': 'python-requests/2.11.1', 'Accept-Encoding': 'gzip, deflate', 'Accept': '*/*', 'Connection': 'keep-alive', 'Cookie': 'cookies_are=working'}


As with my patch since the cookie is set on `test.com` while making a request to `footest.com` the cookie is skipped as part of the patch since footest is not a subdomain of test.com but 1.test.com is a subdomain. This is a behavior change to be decided whether worth doing or to document this since in a client with session like requests module connecting to lot of hosts this can potentially pass cookies of test.com to footest.com. A discussion on requests repo on providing the option for user to set a stricter cookie policy : https://github.com/requests/requests/issues/2576

On testing with curl cookie-jar it seems that the cookies are passed even for the subdomain only when it's set and not as part of top level domain.
msg332576 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-12-27 06:58
Also looking at the docs for different frameworks like [Flask](http://flask.pocoo.org/docs/1.0/api/#flask.Response.set_cookie) and [Django](https://docs.djangoproject.com/en/2.1/ref/request-response/#django.http.HttpResponse.set_cookie) they recommend setting Domain attribute only for cross-domain cookies.

From Django docs

> Use domain if you want to set a cross-domain cookie. For example, domain="example.com" will set a cookie that is readable by the domains www.example.com, blog.example.com, etc. Otherwise, a cookie will only be readable by the domain that set it.

When there is no domain specified then the frameworks seem to set the cookie only for the host only as per [RFC 6265](https://tools.ietf.org/html/rfc6265#section-5.3). So domain attribute is set all the time and it's just that if the domain attribute is set explicitly by the server with the set_cookie function in the frameworks then the cookiejar has domain_specified set along with dot prefixed for the domain enabling stricter validations. I don't know about the metrics of setting the domain attribute vs not setting it. Checking with a simple Flask app and set_cookie without domain parameter the cookies are passed to suffix domains. With domain passed to set_cookie has dot prefixed and the cookies are not passed to suffix domains.

I also looked into other implementations

* aiohttp - uses cookiejar but has custom domain checks and update cookie methods at https://github.com/aio-libs/aiohttp/blob/49261c192ff225372dffb39056c3c311714b12c5/aiohttp/cookiejar.py#L141 . Thus it's not affected when tested.
* golang implementation - https://github.com/golang/go/blob/50bd1c4d4eb4fac8ddeb5f063c099daccfb71b26/src/net/http/cookiejar/jar.go#L123
msg332583 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-12-27 10:21
I have come across another behavior change between path checks while using the cookie jar implementation available in Python. This is related to incorrect cookie validation but with respect to path so let me know if this needs a separate ticket. I observed the following difference : 

1. Make a request to "/" that sets a cookie with "path=/any"
2. Make a request to "/any" and the set cookie is passed since the path matches
3. Make a request to "/anybad" and cookie with "path=/any" is also passed too.

On using golang stdlib implementation of cookiejar the cookie "path=/any" is not passed when a request to "/anybad" is made. So I checked with RFC 6265 where the path match check is defined in section-5.1.4 . RFC 6265 also obsoletes RFC 2965 upon which cookiejar is based I hope since original implementation of cookiejar is from 2004 and RFC 6265 was standardized later. So I think it's good to enforce RFC 6265 since RFC 2965 is obsolete at least in Python 3.8 unless this is considered as a security issue. I think this is a security issue. The current implementation can potentially cause cookies to be sent to incorrect paths in the same domain that share the same prefix. This is a behavior change with more strict checks but I could see no tests failing with RFC 6265 implementation too.

RFC 2965 also gives a loose definition of path-match without mentioning about / check in the paths based on which Python implementation is based as a simple prefix match.

> For two strings that represent paths, P1 and P2, P1 path-matches P2
> if P2 is a prefix of P1 (including the case where P1 and P2 string-
> compare equal).  Thus, the string /tec/waldo path-matches /tec.

RFC 6265 path-match definition : https://tools.ietf.org/html/rfc6265#section-5.1.4

   A request-path path-matches a given cookie-path if at least one of
   the following conditions holds:

   o  The cookie-path and the request-path are identical.

   o  The cookie-path is a prefix of the request-path, and the last
      character of the cookie-path is %x2F ("/").

   o  The cookie-path is a prefix of the request-path, and the first
      character of the request-path that is not included in the cookie-
      path is a %x2F ("/") character.

The current implementation in cookiejar is as below : 

def path_return_ok(self, path, request):
    _debug("- checking cookie path=%s", path)
    req_path = request_path(request)
    if not req_path.startswith(path):
        _debug("  %s does not path-match %s", req_path, path)
        return False
    return True

Translating the RFC 6265 steps (a literal translation of go implementation) would have something like below and no tests fail on master. So the python implementation goes in line with the RFC not passing cookies of "path=/any" to /anybody

def path_return_ok(self, path, request):
    req_path = request_path(request)
    if req_path == path:
        return True
    elif req_path.startswith(path) and ((path.endswith("/") or req_path[len(path)] == "/")):
        return True
        
    return False


The golang implementation is as below which is a literal translation of RFC 6265 steps at https://github.com/golang/go/blob/50bd1c4d4eb4fac8ddeb5f063c099daccfb71b26/src/net/http/cookiejar/jar.go#L130

// pathMatch implements "path-match" according to RFC 6265 section 5.1.4.
func (e *entry) pathMatch(requestPath string) bool {
	if requestPath == e.Path {
		return true
	}
	if strings.HasPrefix(requestPath, e.Path) {
		if e.Path[len(e.Path)-1] == '/' {
			return true // The "/any/" matches "/any/path" case.
		} else if requestPath[len(e.Path)] == '/' {
			return true // The "/any" matches "/any/path" case.
		}
	}
	return false
}


Feel free to correct me if I am wrong on the above.
msg332920 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-01-03 08:01
I have opened issue35647 for path related checks as a separate report.
History
Date User Action Args
2019-01-03 08:01:23xtreaksetmessages: + msg332920
2018-12-27 10:21:23xtreaksetmessages: + msg332583
2018-12-27 06:58:31xtreaksetmessages: + msg332576
2018-12-24 09:59:22serhiy.storchakasetpriority: high -> release blocker
nosy: + ned.deily
type: behavior -> security
2018-12-24 03:44:50ned.deilysetnosy: + serhiy.storchaka
2018-12-24 03:42:28ned.deilysetkeywords: + security_issue
priority: normal -> high
2018-12-21 20:57:07xtreaksetmessages: + msg332299
2018-11-03 04:02:30xtreaksetmessages: + msg329179
2018-11-03 02:12:48Windson Yangsetnosy: + Windson Yang
messages: + msg329176
2018-10-31 12:36:05xtreaksetnosy: + orsenthil, martin.panter
2018-10-31 11:27:17xtreaksetmessages: + msg328985
versions: + Python 3.8
2018-10-31 10:29:16xtreaksetkeywords: + patch
stage: patch review
pull_requests: + pull_request9569
2018-10-31 10:24:45西田雄治setmessages: + msg328981
2018-10-31 08:15:02xtreaksetnosy: + xtreak
messages: + msg328975
2018-10-31 06:52:48西田雄治create