bpo-35121: prefix dot in domain for proper subdomain validation #10258
Conversation
| @@ -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 | |||
serhiy-storchaka
Dec 21, 2018
Member
This will affect calls of self.is_blocked(domain) and self.is_not_allowed(domain) below.
tirkarthi
Dec 21, 2018
Author
Member
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
Line 601 in f30060d
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
Line 1178 in f30060d
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 | |||
serhiy-storchaka
Dec 24, 2018
Member
This describes what the code does, which is an implementation detail. A news entry should describe the change in the user visible behavior.
tirkarthi
Dec 24, 2018
Author
Member
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.| 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)): |
tirkarthi
Dec 24, 2018
Author
Member
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 = domain
tirkarthi
Dec 24, 2018
Author
Member
Changed the code to use dotdomain as mentioned in #10258 (comment). Thanks.
| 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)): |
| @@ -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 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. |
|
Thanks @serhiy-storchaka . Moved NEWS entry to security. |
|
Thanks @tirkarthi for the PR, and @ned-deily for merging it |
|
GH-12259 is a backport of this pull request to the 3.7 branch. |
…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. |
…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 |
|
GH-12261 is a backport of this pull request to the 3.7 branch. |
…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>
…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>
…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. |
…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>
…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>
…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>
…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>
…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.
…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.
…GH-10258) (GH-13426) This is a manual backport of ca7fe50 since 2.7 has `http.cookiejar` in `cookielib` https://bugs.python.org/issue35121
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