Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-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 |
This comment has been minimized.
This comment has been minimized.
serhiy-storchaka
Dec 21, 2018
Member
This will affect calls of self.is_blocked(domain)
and self.is_not_allowed(domain)
below.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
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 |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
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)): |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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)): |
This comment has been minimized.
This comment has been minimized.
@@ -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 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
LGTM, but the code style and the wording of a news entry may need some polishing. |
This comment has been minimized.
This comment has been minimized.
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 |
This comment has been minimized.
This comment has been minimized.
@serhiy-storchaka Is this worth moving news entry into security section instead of library? |
This comment has been minimized.
This comment has been minimized.
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.com The 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 |
This comment has been minimized.
This comment has been minimized.
Fixed |
This comment has been minimized.
This comment has been minimized.
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 |
This comment has been minimized.
This comment has been minimized.
@vstinner, This is marked as a security fix. |
This comment has been minimized.
This comment has been minimized.
Thanks @serhiy-storchaka . Moved NEWS entry to security. |
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Mar 10, 2019
Thanks @tirkarthi for the PR, and @ned-deily for merging it |
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Mar 10, 2019
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>
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Mar 10, 2019
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>
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Mar 10, 2019
Thanks @tirkarthi for the PR, and @ned-deily for merging it |
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Mar 10, 2019
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>
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Mar 10, 2019
|
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Mar 10, 2019
|
This comment has been minimized.
This comment has been minimized.
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
tirkarthi commentedOct 31, 2018
•
edited by bedevere-bot
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