Issue 35647: Cookie path check returns incorrect results (original) (raw)

Issue35647

process

Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, larry, martin.panter, miss-islington, ned.deily, orsenthil, serhiy.storchaka, vstinner, xtreak
Priority: normal Keywords: patch, patch, patch

Created on 2019-01-03 07:59 by xtreak, last changed 2022-04-11 14:59 by admin. This issue is now closed.

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
PR 13427 merged xtreak,2019-05-19 19:22
Messages (15)
msg332919 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) 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 committer) 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
msg344565 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-04 13:10
Python 2.7 is still affected, right? Is there someone interested to backport the fix?
msg344566 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2019-06-04 13:13
> Python 2.7 is still affected, right? Is there someone interested to backport the fix? PR 13427 fixes the issue in 2.7 :)
msg345697 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-06-15 16:29
New changeset ee15aa2b8501718cb77e339381f72409a416f801 by Serhiy Storchaka (Xtreak) in branch '2.7': [2.7] bpo-35647: Fix path check in cookiejar. (GH-11436) (GH-13427) https://github.com/python/cpython/commit/ee15aa2b8501718cb77e339381f72409a416f801
msg345699 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2019-06-15 16:37
Closing this as resolved since the fix was merged to all branches. Thank you all.
msg345735 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-16 07:56
Well done Karthikeyan Singaravelan! I planned to review your 2.7 change, but I didn't manage to find time for that. So thanks Serhiy for merging it!
History
Date User Action Args
2022-04-11 14:59:09 admin set github: 79828
2019-06-16 07:56:57 vstinner set messages: +
2019-06-15 16:37:47 xtreak set status: open -> closedmessages: + keywords:patch, patch, patchresolution: fixedstage: patch review -> resolved
2019-06-15 16:29:32 serhiy.storchaka set messages: +
2019-06-04 13:13:46 xtreak set keywords:patch, patch, patchmessages: +
2019-06-04 13:10:34 vstinner set keywords:patch, patch, patchnosy: + vstinnermessages: +
2019-05-19 19:22:43 xtreak set pull_requests: + <pull%5Frequest13337>
2019-05-10 18:19:51 ned.deily set messages: -
2019-05-10 17:36:38 ned.deily set messages: +
2019-03-16 23:42:15 larry set messages: +
2019-03-16 22:54:05 larry set messages: +
2019-03-14 12:56:53 orsenthil set keywords:patch, patch, patchmessages: +
2019-03-13 06:00:36 larry set keywords:patch, patch, patchassignee: larry -> messages: +
2019-03-13 03:50:12 orsenthil set keywords:patch, patch, patchassignee: larry
2019-03-12 04:28:45 ned.deily set messages: +
2019-03-11 15:47:16 xtreak set keywords:patch, patch, patchversions: + Python 3.4
2019-03-11 15:46:08 xtreak set pull_requests: + <pull%5Frequest12258>
2019-03-11 15:26:07 xtreak set pull_requests: + <pull%5Frequest12257>
2019-03-11 04:44:41 larry set keywords:patch, patch, patchmessages: +
2019-03-10 18:37:35 xtreak set versions: + Python 2.7nosy: + larry, benjamin.petersonmessages: + keywords:patch, patch, patch
2019-03-10 17:30:39 miss-islington set nosy: + miss-islingtonmessages: +
2019-03-10 17:13:43 orsenthil set keywords:patch, patch, patchversions: + Python 3.5, Python 3.6
2019-03-10 17:12:54 miss-islington set pull_requests: + <pull%5Frequest12252>
2019-03-10 17:12:44 miss-islington set pull_requests: + <pull%5Frequest12251>
2019-03-10 17:12:40 orsenthil set messages: +
2019-01-05 06:48:54 xtreak set keywords: + patchstage: patch reviewpull_requests: + <pull%5Frequest10870>
2019-01-05 06:48:47 xtreak set keywords: + patchstage: (no value)pull_requests: + <pull%5Frequest10869>
2019-01-05 06:48:41 xtreak set keywords: + patchstage: (no value)pull_requests: + <pull%5Frequest10868>
2019-01-03 07:59:56 xtreak create