From 73474e1b518ce91481b7101d68d8a75c7ad67029 Mon Sep 17 00:00:00 2001 From: Lumir Balhar Date: Mon, 4 Mar 2019 12:45:09 +0100 Subject: [PATCH] CVE-2018-20060 Cross-host redirect does not remove Authorization header allow for credential exposure. --- CHANGES.rst | 4 ++++ urllib3/poolmanager.py | 11 ++++++++++- urllib3/util/retry.py | 12 +++++++++++- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 0150d85..34ad6b0 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,6 +1,10 @@ Changes ======= +* Allow providing a list of headers to strip from requests when redirecting + to a different host. Defaults to the ``Authorization`` header. Different + headers can be set via ``Retry.remove_headers_on_redirect``. (Issue #1316) + * Accept ``iPAddress`` subject alternative name fields in TLS certificates. (Issue #258) diff --git a/urllib3/poolmanager.py b/urllib3/poolmanager.py index 4fdae8d..b43b235 100644 --- a/urllib3/poolmanager.py +++ b/urllib3/poolmanager.py @@ -238,8 +238,9 @@ class PoolManager(RequestMethods): kw['assert_same_host'] = False kw['redirect'] = False + if 'headers' not in kw: - kw['headers'] = self.headers + kw['headers'] = self.headers.copy() if self.proxy is not None and u.scheme == "http": response = conn.urlopen(method, url, **kw) @@ -261,6 +262,14 @@ class PoolManager(RequestMethods): if not isinstance(retries, Retry): retries = Retry.from_int(retries, redirect=redirect) + # Strip headers marked as unsafe to forward to the redirected location. + # Check remove_headers_on_redirect to avoid a potential network call within + # conn.is_same_host() which may use socket.gethostbyname() in the future. + if (retries.remove_headers_on_redirect + and not conn.is_same_host(redirect_location)): + for header in retries.remove_headers_on_redirect: + kw['headers'].pop(header, None) + try: retries = retries.increment(method, url, response=response, _pool=conn) except MaxRetryError: diff --git a/urllib3/util/retry.py b/urllib3/util/retry.py index 7e0959d..0cf8eed 100644 --- a/urllib3/util/retry.py +++ b/urllib3/util/retry.py @@ -101,17 +101,25 @@ class Retry(object): :param bool raise_on_redirect: Whether, if the number of redirects is exhausted, to raise a MaxRetryError, or to return a response with a response code in the 3xx range. + + :param iterable remove_headers_on_redirect: + Sequence of headers to remove from the request when a response + indicating a redirect is returned before firing off the redirected + request. """ DEFAULT_METHOD_WHITELIST = frozenset([ 'HEAD', 'GET', 'PUT', 'DELETE', 'OPTIONS', 'TRACE']) + DEFAULT_REDIRECT_HEADERS_BLACKLIST = frozenset(['Authorization']) + #: Maximum backoff time. BACKOFF_MAX = 120 def __init__(self, total=10, connect=None, read=None, redirect=None, method_whitelist=DEFAULT_METHOD_WHITELIST, status_forcelist=None, - backoff_factor=0, raise_on_redirect=True, _observed_errors=0): + backoff_factor=0, raise_on_redirect=True, _observed_errors=0, + remove_headers_on_redirect=DEFAULT_REDIRECT_HEADERS_BLACKLIST): self.total = total self.connect = connect @@ -127,6 +135,7 @@ class Retry(object): self.backoff_factor = backoff_factor self.raise_on_redirect = raise_on_redirect self._observed_errors = _observed_errors # TODO: use .history instead? + self.remove_headers_on_redirect = remove_headers_on_redirect def new(self, **kw): params = dict( @@ -137,6 +146,7 @@ class Retry(object): backoff_factor=self.backoff_factor, raise_on_redirect=self.raise_on_redirect, _observed_errors=self._observed_errors, + remove_headers_on_redirect=self.remove_headers_on_redirect, ) params.update(kw) return type(self)(**params) -- 2.20.1