Repository: micropython/micropython-lib
Issue category: Security Vulnerability

Issue:
SECURITY: Requests module leaks passwords & usernames for HTTP Basic Auth
While looking at the MicroPython `requests` module (on the git HEAD), I noticed this:

If you make a request with HTTP basic auth (a username/password) and did not specify a headers dict, then I believe the username and password would be added to the default headers to be used for **every subsequent HTTP request**.  Even if that request is to a completely different server, which you don't trust with your username and password.  That's probably not a good idea.

I haven't verified this, it's just from reading the code, but someone should probably look into it.

This is because there is `headers={}` in the function prototype, specifying a default for the `headers` parameter.  But (at least in cPython) that same dictionary will get reused for every call that doesn't explicitly specify a `headers` parameter.  So if the function changes the `headers` dictionary - such as by adding an `Authorization` header - that change will be there for every future call of the function.  This is a known dangerous part of the Python language, you're not the first people to write this kind of bug.

To fix this, you could keep the auth headers separate from the `headers` variable.  Something like this (totally untested!) commit: https://github.com/jonfoster/micropython-lib/commit/92e9b2208814faa22ba3ff2a19cbc8e0c5210a47 - feel free to use that as a starting point.
SECURITY: Requests module leaks passwords & usernames for HTTP Basic Auth
While looking at the MicroPython `requests` module (on the git HEAD), I noticed this:

If you make a request with HTTP basic auth (a username/password) and did not specify a headers dict, then I believe the username and password would be added to the default headers to be used for **every subsequent HTTP request**.  Even if that request is to a completely different server, which you don't trust with your username and password.  That's probably not a good idea.

I haven't verified this, it's just from reading the code, but someone should probably look into it.

This is because there is `headers={}` in the function prototype, specifying a default for the `headers` parameter.  But (at least in cPython) that same dictionary will get reused for every call that doesn't explicitly specify a `headers` parameter.  So if the function changes the `headers` dictionary - such as by adding an `Authorization` header - that change will be there for every future call of the function.  This is a known dangerous part of the Python language, you're not the first people to write this kind of bug.

To fix this, you could keep the auth headers separate from the `headers` variable.  Something like this (totally untested!) commit: https://github.com/jonfoster/micropython-lib/commit/92e9b2208814faa22ba3ff2a19cbc8e0c5210a47 - feel free to use that as a starting point.


Hints:
The MicroPython way aiui would be to mirror CPython's solution to this problem, which uses a `None` default value and then sets it to an empty dict at runtime:

https://github.com/psf/requests/blob/0e322af87745eff34caffe4df68456ebc20d9068/src/requests/models.py#L258-L276


And I see this is exactly what #823 does. That'll teach me not to look at PRs first 😆 
The MicroPython way aiui would be to mirror CPython's solution to this problem, which uses a `None` default value and then sets it to an empty dict at runtime:

https://github.com/psf/requests/blob/0e322af87745eff34caffe4df68456ebc20d9068/src/requests/models.py#L258-L276


And I see this is exactly what #823 does. That'll teach me not to look at PRs first 😆 

Patch:
diff --git a/python-ecosys/requests/requests/__init__.py b/python-ecosys/requests/requests/__init__.py
index a9a183619..2951035f7 100644
--- a/python-ecosys/requests/requests/__init__.py
+++ b/python-ecosys/requests/requests/__init__.py
@@ -46,6 +46,8 @@ def request(
 ):
     if headers is None:
         headers = {}
+    else:
+        headers = headers.copy()
 
     redirect = None  # redirection url, None means no redirection
     chunked_data = data and getattr(data, "__next__", None) and not getattr(data, "__len__", None)


Test patch:
diff --git a/python-ecosys/requests/test_requests.py b/python-ecosys/requests/test_requests.py
index 513e533a3..ac77291b0 100644
--- a/python-ecosys/requests/test_requests.py
+++ b/python-ecosys/requests/test_requests.py
@@ -102,11 +102,11 @@ def chunks():
 
 def test_overwrite_get_headers():
     response = requests.request(
-        "GET", "http://example.com", headers={"Connection": "keep-alive", "Host": "test.com"}
+        "GET", "http://example.com", headers={"Host": "test.com", "Connection": "keep-alive"}
     )
 
     assert response.raw._write_buffer.getvalue() == (
-        b"GET / HTTP/1.0\r\n" + b"Host: test.com\r\n" + b"Connection: keep-alive\r\n\r\n"
+        b"GET / HTTP/1.0\r\n" + b"Connection: keep-alive\r\n" + b"Host: test.com\r\n\r\n"
     ), format_message(response)
 
 
@@ -145,6 +145,14 @@ def chunks():
     ), format_message(response)
 
 
+def test_do_not_modify_headers_argument():
+    global do_not_modify_this_dict
+    do_not_modify_this_dict = {}
+    requests.request("GET", "http://example.com", headers=do_not_modify_this_dict)
+
+    assert do_not_modify_this_dict == {}, do_not_modify_this_dict
+
+
 test_simple_get()
 test_get_auth()
 test_get_custom_header()
@@ -153,3 +161,4 @@ def chunks():
 test_overwrite_get_headers()
 test_overwrite_post_json_headers()
 test_overwrite_post_chunked_data_headers()
+test_do_not_modify_headers_argument()