classification
Title: Cookie path check returns incorrect results
Type: security Stage: patch review
Components: Library (Lib) Versions: Python 3.8, Python 3.7, Python 3.6, Python 3.4, Python 3.5, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, larry, martin.panter, miss-islington, ned.deily, orsenthil, serhiy.storchaka, xtreak
Priority: normal Keywords: patch, patch, patch

Created on 2019-01-03 07:59 by xtreak, last changed 2019-03-16 23:42 by larry.

Pull Requests
URL Status Linked Edit
PR 11436 merged xtreak, 2019-01-05 06:48
PR 11436 merged xtreak, 2019-01-05 06:48
PR 11436 merged xtreak, 2019-01-05 06:48
PR 12267 merged miss-islington, 2019-03-10 17:12
PR 12268 merged miss-islington, 2019-03-10 17:12
PR 12277 merged xtreak, 2019-03-11 15:26
PR 12278 merged xtreak, 2019-03-11 15:46
Messages (10)
msg332919 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-01-03 07:59
I came across the issue during https://bugs.python.org/issue35121#msg332583 . I think this can be dealt as a separate issue not blocking the original report. I am classifying it as security but can be reclassified as a bug fix given the section on weak confidentiality in RFC 6265. I have a fix implemented at https://github.com/tirkarthi/cpython/tree/bpo35121-cookie-path.

Report : 

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. 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
}


RFC 6265 on weak confidentiality (https://tools.ietf.org/html/rfc6265#section-8.5)

   Cookies do not always provide isolation by path.  Although the
   network-level protocol does not send cookies stored for one path to
   another, some user agents expose cookies via non-HTTP APIs, such as
   HTML's document.cookie API.  Because some of these user agents (e.g.,
   web browsers) do not isolate resources received from different paths,
   a resource retrieved from one path might be able to access cookies
   stored for another path.
msg337623 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2019-03-10 17:12
New changeset 0e1f1f01058bd4a9b98cfe443214adecc019a38c by Senthil Kumaran (Xtreak) in branch 'master':
bpo-35647: Fix path check in cookiejar (#11436)
https://github.com/python/cpython/commit/0e1f1f01058bd4a9b98cfe443214adecc019a38c
msg337624 - (view) Author: miss-islington (miss-islington) Date: 2019-03-10 17:30
New changeset 97c7d78fda49e03fc773c171ce0c736d02bb73f5 by Miss Islington (bot) in branch '3.7':
bpo-35647: Fix path check in cookiejar (GH-11436)
https://github.com/python/cpython/commit/97c7d78fda49e03fc773c171ce0c736d02bb73f5
msg337632 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-03-10 18:37
The backport to 3.5 might require manual work since I used f-strings for tests that are not present in 3.5 and below. 2.7 is also affected and as I backported the tests and cookie set with path=/foo is sent on request to /foobad/foo . The module is present under Lib/cookielb.py and might also require a different backport. Since this applies RFC 6265 definition that is more stricter and concrete than RFC 2965 I am not sure if this might break someone's code though there is a bug in the paths to which the cookie is sent. I am adding Larry and Benjamin who can take a call on backport and if a backport is needed I will be happy to open respective PRs.

The code in 2.7 also performs the same prefix match at https://github.com/python/cpython/blob/55438d713978a1913ef12c8a801848626228aad6/Lib/cookielib.py#L1182 that was fixed as per RFC 6265 .

    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


$ ./python.exe
Python 2.7.16+ (remotes/upstream/2.7-dirty:55438d7139, Mar 10 2019, 23:35:15)
[GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>>
$ ./python.exe -m unittest -v test.test_cookielib.CookieTests.test_path_prefix_match
test_path_prefix_match (test.test_cookielib.CookieTests) ... FAIL

======================================================================
FAIL: test_path_prefix_match (test.test_cookielib.CookieTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/test/test_cookielib.py", line 673, in test_path_prefix_match
    self.assertNotIn('spam=eggs', h, "cookie set for {0}".format(path))
AssertionError: cookie set for /foobad/foo

----------------------------------------------------------------------
Ran 1 test in 0.010s

FAILED (failures=1)
msg337644 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2019-03-11 04:44
Yes, I'd like backports to 3.5 and 3.4.  Note that I tag and release 3.4.10 final this weekend, which will be the final release ever of 3.4, so if it can't be ready for merging before then, you might as well skip the 3.4 PR.
msg337719 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2019-03-12 04:28
New changeset 5565b1db6f37f244890369e0d68a3e906aca28b9 by Ned Deily (Miss Islington (bot)) in branch '3.6':
bpo-35647: Fix path check in cookiejar (GH-11436) (GH-12268)
https://github.com/python/cpython/commit/5565b1db6f37f244890369e0d68a3e906aca28b9
msg337836 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2019-03-13 06:00
You should not assign bugs to the RM who will merge the PR.
msg337911 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2019-03-14 12:56
Got it, Larry. I wanted to track it and make sure you don't miss it. Looks like we have the PR from @xtreak for 3.4 and 3.5 branches.
Thank you!
msg338108 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2019-03-16 22:54
New changeset e260f092cd0d8975c777e73ca6fb549d59b5d452 by larryhastings (Xtreak) in branch '3.4':
bpo-35647: Fix path check in cookiejar (#11436) (#12278)
https://github.com/python/cpython/commit/e260f092cd0d8975c777e73ca6fb549d59b5d452
msg338111 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2019-03-16 23:42
New changeset 382981b25092b5e9285f1e4894142af1e8f2ca86 by larryhastings (Xtreak) in branch '3.5':
bpo-35647: Fix path check in cookiejar (#11436) (#12277)
https://github.com/python/cpython/commit/382981b25092b5e9285f1e4894142af1e8f2ca86
History
Date User Action Args
2019-03-16 23:42:15larrysetmessages: + msg338111
2019-03-16 22:54:05larrysetmessages: + msg338108
2019-03-14 12:56:53orsenthilsetkeywords: patch, patch, patch

messages: + msg337911
2019-03-13 06:00:36larrysetkeywords: patch, patch, patch
assignee: larry ->
messages: + msg337836
2019-03-13 03:50:12orsenthilsetkeywords: patch, patch, patch
assignee: larry
2019-03-12 04:28:45ned.deilysetmessages: + msg337719
2019-03-11 15:47:16xtreaksetkeywords: patch, patch, patch
versions: + Python 3.4
2019-03-11 15:46:08xtreaksetpull_requests: + pull_request12258
2019-03-11 15:26:07xtreaksetpull_requests: + pull_request12257
2019-03-11 04:44:41larrysetkeywords: patch, patch, patch

messages: + msg337644
2019-03-10 18:37:35xtreaksetversions: + Python 2.7
nosy: + larry, benjamin.peterson

messages: + msg337632

keywords: patch, patch, patch
2019-03-10 17:30:39miss-islingtonsetnosy: + miss-islington
messages: + msg337624
2019-03-10 17:13:43orsenthilsetkeywords: patch, patch, patch
versions: + Python 3.5, Python 3.6
2019-03-10 17:12:54miss-islingtonsetpull_requests: + pull_request12252
2019-03-10 17:12:44miss-islingtonsetpull_requests: + pull_request12251
2019-03-10 17:12:40orsenthilsetmessages: + msg337623
2019-01-05 06:48:54xtreaksetkeywords: + patch
stage: patch review
pull_requests: + pull_request10870
2019-01-05 06:48:47xtreaksetkeywords: + patch
stage: (no value)
pull_requests: + pull_request10869
2019-01-05 06:48:41xtreaksetkeywords: + patch
stage: (no value)
pull_requests: + pull_request10868
2019-01-03 07:59:56xtreakcreate