| msg332919 - (view) |
Author: Karthikeyan Singaravelan (xtreak) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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! |
|
|