bpo-35121: prefix dot in domain for proper subdomain validation #10258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lib/http/cookiejar.py
Outdated
| @@ -1173,6 +1173,8 @@ def domain_return_ok(self, domain, request): | |||
| req_host = "."+req_host | |||
| if not erhn.startswith("."): | |||
| erhn = "."+erhn | |||
| if not domain.startswith("."): | |||
| domain = "."+domain | |||
There was a problem hiding this comment.
This will affect calls of self.is_blocked(domain) and self.is_not_allowed(domain) below.
There was a problem hiding this comment.
Thanks @serhiy-storchaka . My bad that I looked into fixing the issue and not about the underlying callers that use the dot-prefixed domain. Yes, adding the extra dot makes the comparison to fail where A has an extra dot at start due to my patch at https://github.com/python/cpython/blob/f30060dcd07cd53879226816512ea80bff0d0a78/Lib/http/cookiejar.py#L601 .
Sample program where the domain should be blocked
import urllib
from http.cookiejar import DefaultCookiePolicy
policy = DefaultCookiePolicy(blocked_domains=['xxxfoo.co.jp'])
req = urllib.request.Request('https://xxxfoo.co.jp/')
print(policy.domain_return_ok('xxxfoo.co.jp', req))➜ cpython git:(master) ✗ python3.7 /tmp/bar.py
False
➜ cpython git:(bpo35121) ✗ ./python.exe /tmp/bar.py
True
With patch this returns true but should be false since the domain is blocked and the prefix dot makes the comparison .xxxfoo.co.jp == xxxfoo.co.jp . One fix would be to use dot prefixed domain only for the checks at
I think this needs to be fixed but I am also afraid I might accidentally break something here since the function itself received no changes since 2004.
|
Looking further into this the domain validation makes it little more stricter and can have wider implications. Example requests library uses cookiejar to maintain cookies between sessions. One more case is that A simple server that sets Cookie with value 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 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://footest.com:8000", cookies=cookies)
print(m.request.headers)Before patch : After patch : As with my patch since the cookie is set on I am adding this to the tracker too. |
| @@ -0,0 +1,2 @@ | |||
| Prefix domain with dot for proper subdomain validation in | |||
There was a problem hiding this comment.
This describes what the code does, which is an implementation detail. A news entry should describe the change in the user visible behavior.
There was a problem hiding this comment.
Added a note about it but this is also affects when domain_return_ok is present. I couldn't come up with a better wording since I am not a native speaker. Suggestions welcome. This started as a bug fix but since this
turned out to be a security issue is it okay to move this to security section?
Don't send cookies of domain A without Domain attribute to domain B
when domain A is a suffix match of domain B while using a cookiejar
with :meth:`http.cookiejar.DefaultCookiePolicy` policy. Patch by
Karthikeyan Singaravelan.
Lib/http/cookiejar.py
Outdated
| if not (req_host.endswith(domain) or erhn.endswith(domain)): | ||
| if suffix_check_domain and not suffix_check_domain.startswith("."): | ||
| suffix_check_domain = "." + suffix_check_domain | ||
| if not (req_host.endswith(suffix_check_domain) or erhn.endswith(suffix_check_domain)): |
There was a problem hiding this comment.
New code should conform to PEP 8.
There was a problem hiding this comment.
Maybe use shorter name, e.g. dotdomain?
There was a problem hiding this comment.
dotdomain sounds good to me. Perhaps restructure the clause as below removing the assignment at the start?
if domain and not domain.startswith("."):
dotdomain = "." + domain
else:
dotdomain = domainThere was a problem hiding this comment.
Changed the code to use dotdomain as mentioned in #10258 (comment). Thanks.
Lib/http/cookiejar.py
Outdated
| if not (req_host.endswith(domain) or erhn.endswith(domain)): | ||
| if suffix_check_domain and not suffix_check_domain.startswith("."): | ||
| suffix_check_domain = "." + suffix_check_domain | ||
| if not (req_host.endswith(suffix_check_domain) or erhn.endswith(suffix_check_domain)): |
There was a problem hiding this comment.
Maybe use shorter name, e.g. dotdomain?
| @@ -0,0 +1,4 @@ | |||
| Don't send cookies of domain A without Domain attribute to domain B | |||
| when domain A is a suffix match of domain B while using a cookiejar | |||
| with :meth:`http.cookiejar.DefaultCookiePolicy` policy. Patch by | |||
|
LGTM, but the code style and the wording of a news entry may need some polishing. |
|
This also affects 2.7 as I can see the added tests failing without the fix with the same logic being present. I don't know if it might break anything since 2.7 is there for a long time. I have done a manual backport of this PR in case this is needed for 2.7 and locally tests pass for cookielib. Branch : 2.7...tirkarthi:bpo35121-27 |
serhiy-storchaka
approved these changes
Dec 24, 2018
|
@serhiy-storchaka Is this worth moving news entry into security section instead of library? |
|
Sorry this turned out to be longer since I think I may have found another issue in I have a fix for https://docs.python.org/3/library/http.cookiejar.html#http.cookiejar.CookiePolicy.return_ok pol.set_blocked_domains([])
req = urllib.request.Request("http://acme.com/")
res = FakeResponse(headers, "http://acme.com/")
cookies = c.make_cookies(res, req)
c.extract_cookies(res, req)
self.assertEqual(len(c), 1)
print(cookies[0]) # <Cookie CUSTOMER=WILE_E_COYOTE for acme.com/>
req = urllib.request.Request("http://badacme.com/")
self.assertFalse(pol.return_ok(cookies[0], req)) # This fails returning true though cookie is set on acme.comThe function return_ok calls helper functions for different attributes of a cookie like return_ok_path, return_ok_domain, etc. In Line 1162 in 44a79cc From the docs at https://docs.python.org/3/library/http.cookiejar.html#http.cookiejar.CookiePolicy.domain_return_ok
Now that Line 1251 in 44a79cc return_ok_domain still has the bug though it's not executed. The fix would be to use dotdomain in return_ok_domain similar to current one. I applied the fix and no tests failed.
One more option is that there are stricter versions of the policy that could be enabled with the flags and hence |
|
Fixed |
|
I removed the " needs backport to 3.6" label, the 3.6 branch no long accept bugfixes (only security fixes are accepted): https://devguide.python.org/#status-of-python-branches |
|
@vstinner, This is marked as a security fix. |
serhiy-storchaka
approved these changes
Mar 9, 2019
|
Thanks @serhiy-storchaka . Moved NEWS entry to security. |
alex
approved these changes
Mar 9, 2019
|
Thanks @tirkarthi for the PR, and @ned-deily for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7. |
|
GH-12259 is a backport of this pull request to the 3.7 branch. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Mar 10, 2019
…onGH-10258) Don't send cookies of domain A without Domain attribute to domain B when domain A is a suffix match of domain B while using a cookiejar with `http.cookiejar.DefaultCookiePolicy` policy. Patch by Karthikeyan Singaravelan. (cherry picked from commit ca7fe50) Co-authored-by: Xtreak <tir.karthi@gmail.com>
|
GH-12260 is a backport of this pull request to the 3.6 branch. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Mar 10, 2019
…onGH-10258) Don't send cookies of domain A without Domain attribute to domain B when domain A is a suffix match of domain B while using a cookiejar with `http.cookiejar.DefaultCookiePolicy` policy. Patch by Karthikeyan Singaravelan. (cherry picked from commit ca7fe50) Co-authored-by: Xtreak <tir.karthi@gmail.com>
|
Thanks @tirkarthi for the PR, and @ned-deily for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
|
GH-12261 is a backport of this pull request to the 3.7 branch. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Mar 10, 2019
…onGH-10258) Don't send cookies of domain A without Domain attribute to domain B when domain A is a suffix match of domain B while using a cookiejar with `http.cookiejar.DefaultCookiePolicy` policy. Patch by Karthikeyan Singaravelan. (cherry picked from commit ca7fe50) Co-authored-by: Xtreak <tir.karthi@gmail.com>
ned-deily
pushed a commit
that referenced
this pull request
Mar 10, 2019
…0258) (GH-12261) Don't send cookies of domain A without Domain attribute to domain B when domain A is a suffix match of domain B while using a cookiejar with `http.cookiejar.DefaultCookiePolicy` policy. Patch by Karthikeyan Singaravelan. (cherry picked from commit ca7fe50) Co-authored-by: Xtreak <tir.karthi@gmail.com>
ned-deily
pushed a commit
that referenced
this pull request
Mar 10, 2019
…0258) (GH-12260) Don't send cookies of domain A without Domain attribute to domain B when domain A is a suffix match of domain B while using a cookiejar with `http.cookiejar.DefaultCookiePolicy` policy. Patch by Karthikeyan Singaravelan. (cherry picked from commit ca7fe50) Co-authored-by: Xtreak <tir.karthi@gmail.com>
|
|
|
The buildbot failures seem to be unrelated to the issue . I can see open issues for test_ssl https://bugs.python.org/issue35925 and https://bugs.python.org/issue35136 . Thanks much Serhiy, Ned, Alex and Zackery for the review and merge. |
tirkarthi
added a commit
to tirkarthi/cpython
that referenced
this pull request
Mar 11, 2019
…pythonGH-10258) Don't send cookies of domain A without Domain attribute to domain B when domain A is a suffix match of domain B while using a cookiejar with `http.cookiejar.DefaultCookiePolicy` policy. Patch by Karthikeyan Singaravelan. (cherry picked from commit ca7fe50) Co-authored-by: Xtreak <tir.karthi@gmail.com>
tirkarthi
added a commit
to tirkarthi/cpython
that referenced
this pull request
Mar 11, 2019
…pythonGH-10258) Don't send cookies of domain A without Domain attribute to domain B when domain A is a suffix match of domain B while using a cookiejar with `http.cookiejar.DefaultCookiePolicy` policy. Patch by Karthikeyan Singaravelan. (cherry picked from commit ca7fe50) Co-authored-by: Xtreak <tir.karthi@gmail.com>
larryhastings
pushed a commit
that referenced
this pull request
Mar 16, 2019
…GH-10258) (#12279) Don't send cookies of domain A without Domain attribute to domain B when domain A is a suffix match of domain B while using a cookiejar with `http.cookiejar.DefaultCookiePolicy` policy. Patch by Karthikeyan Singaravelan. (cherry picked from commit ca7fe50) Co-authored-by: Xtreak <tir.karthi@gmail.com>
larryhastings
pushed a commit
that referenced
this pull request
Mar 17, 2019
…GH-10258) (#12281) Don't send cookies of domain A without Domain attribute to domain B when domain A is a suffix match of domain B while using a cookiejar with `http.cookiejar.DefaultCookiePolicy` policy. Patch by Karthikeyan Singaravelan. (cherry picked from commit ca7fe50) Co-authored-by: Xtreak <tir.karthi@gmail.com>
miss-islington
pushed a commit
that referenced
this pull request
Jun 15, 2019
…GH-10258) (GH-13426) This is a manual backport of ca7fe50 since 2.7 has `http.cookiejar` in `cookielib` https://bugs.python.org/issue35121
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Domain related check is done at
cpython/Lib/http/cookiejar.py
Line 1176 in 0353b4e
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.This causes domain "foo.com" to be a valid domain for access of "barfoo.com" since "foo.com" matches the subdomain in substring match with endswith method.
https://bugs.python.org/issue35121