bpo-39603: Prevent header injection in http methods #18485

amiremohamadi · 2020-02-12T13:54:55Z

reject control chars in http method in http.client.putrequest to prevent http header injection

https://bugs.python.org/issue39603

Automerge-Triggered-By: @gvanrossum

codecov · 2020-02-12T14:25:26Z

Codecov Report

Merging #18485 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #18485     +/-   ##
=========================================
  Coverage   82.12%   82.12%             
=========================================
  Files        1955     1955             
  Lines      588958   583810   -5148     
  Branches    44428    44449     +21     
=========================================
- Hits       483663   479453   -4210     
+ Misses      95652    94713    -939     
- Partials     9643     9644      +1

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	`30.00% <0.00%> (-65.00%)`	⬇️
Lib/distutils/command/bdist_rpm.py	`7.63% <0.00%> (-56.88%)`	⬇️
Lib/test/test_urllib2net.py	`76.92% <0.00%> (-13.85%)`	⬇️
Lib/test/test_smtpnet.py	`78.57% <0.00%> (-7.15%)`	⬇️
Lib/ftplib.py	`63.85% <0.00%> (-6.06%)`	⬇️
Lib/test/test_ftplib.py	`87.11% <0.00%> (-4.72%)`	⬇️
Tools/scripts/db2pickle.py	`17.82% <0.00%> (-3.97%)`	⬇️
Tools/scripts/pickle2db.py	`16.98% <0.00%> (-3.78%)`	⬇️
Lib/test/test_socket.py	`71.94% <0.00%> (-3.77%)`	⬇️
Lib/test/test_asyncio/test_base_events.py	`91.84% <0.00%> (-3.30%)`	⬇️
... and 343 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b138dd2...5d9825f. Read the comment docs.

amiremohamadi · 2020-02-12T14:32:26Z

Should I write some tests??!

…cpython into fix-http-header-injection

alex

This looks correct to me, however I'm far from an expert in the http module, so I'd appreciate a second review.

jaraco

This looks good to me. By providing an _validate method, it enables subclasses an escape hatch to suppress or customize the validation.

gvanrossum

So this basically disallows control characters. It still allows spaces, which seems questionable, and non-ASCII bytes, which seems even more questionable (though probably disallowed by the ASCII encoding requirement), as well as ASCII punctuation characters (which I'm not sure would be useful). Could we just match the entire verb against r"\A\w+\Z"? Or are there use cases for having punctuation or spaces in the HTTP verb( method)?

gvanrossum · 2020-07-17T23:21:33Z

Lib/http/client.py

@@ -1086,6 +1090,7 @@ def putrequest(self, method, url, skip_host=False,
            raise CannotSendRequest(self.__state)

        # Save the method for use later in the response phase
+        self._validate_method(method)


Placing the validation right behind this comment is somewhat misleading -- the comment explains the assignment, but the validation is not just for the assignment -- its primary use is for the construction of the request variable a few lines nelow (L1099 in this version). I recommend moving this up and placing blank lines around it.

amiremohamadi · 2020-07-18T18:32:31Z

So this basically disallows control characters. It still allows spaces, which seems questionable, and non-ASCII bytes, which seems even more questionable (though probably disallowed by the ASCII encoding requirement), as well as ASCII punctuation characters (which I'm not sure would be useful). Could we just match the entire verb against r"\A\w+\Z"? Or are there use cases for having punctuation or spaces in the HTTP verb( method)?

@gvanrossum Thank you for the review!
I'm not sure but _encode_request method (L1102) encodes the request (including method) to ASCII. So do we need to restrict non-ASCII characters?

gvanrossum

Thanks. I'm approving this. The security issue is fixed by this, if people want to screw around with weird verbs I guess the server will just reject it.

miss-islington · 2020-07-18T20:16:12Z

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

miss-islington · 2020-07-18T20:16:12Z

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

miss-islington · 2020-07-18T20:16:12Z

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

miss-islington · 2020-07-18T20:16:12Z

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

reject control chars in http method in http.client.putrequest to prevent http header injection (cherry picked from commit 8ca8a2e) Co-authored-by: AMIR <31338382+amiremohamadi@users.noreply.github.com>

bedevere-bot · 2020-07-18T20:16:46Z

GH-21537 is a backport of this pull request to the 3.8 branch.

bedevere-bot · 2020-07-18T20:16:53Z

GH-21538 is a backport of this pull request to the 3.7 branch.

bedevere-bot · 2020-07-18T20:17:01Z

GH-21539 is a backport of this pull request to the 3.6 branch.

reject control chars in http method in http.client.putrequest to prevent http header injection (cherry picked from commit 8ca8a2e) Co-authored-by: AMIR <31338382+amiremohamadi@users.noreply.github.com>

) reject control chars in http method in http.client.putrequest to prevent http header injection (cherry picked from commit 8ca8a2e) Co-authored-by: AMIR <31338382+amiremohamadi@users.noreply.github.com>

orsenthil · 2020-07-20T18:00:22Z

Thank you, everyone - who got involved with review of this patch. The review comments that were mentioned in bpo ticket seems to have been addressed. The changes look good to me.

reject control chars in http method in http.client.putrequest to prevent http header injection

vstinner · 2020-08-24T15:53:04Z

I created a backport to 3.5, can someone please review it? PR #21946.

reject control chars in http method in http.client.putrequest to prevent http header injection (cherry picked from commit 8ca8a2e) Co-authored-by: AMIR <31338382+amiremohamadi@users.noreply.github.com>

00354 # Reject control chars in HTTP method in httplib.putrequest to prevent HTTP header injection Backported from Python 3.5-3.10 (and adjusted for py2's single-module httplib): - https://bugs.python.org/issue39603 - python#18485 (3.10) - python#21946 (3.5) Co-authored-by: AMIR <31338382+amiremohamadi@users.noreply.github.com>

reject control chars in http method in http.client.putrequest to prevent http header injection

00354 # Reject control chars in HTTP method in httplib.putrequest to prevent HTTP header injection Backported from Python 3.5-3.10 (and adjusted for py2's single-module httplib): - https://bugs.python.org/issue39603 - python#18485 (3.10) - python#21946 (3.5) Co-authored-by: AMIR <31338382+amiremohamadi@users.noreply.github.com>

reject control characters within http method name

afb7ee6

the-knights-who-say-ni added the CLA signed label Feb 12, 2020

bedevere-bot added the awaiting review label Feb 12, 2020

Update 2020-02-12-14-17-39.bpo-39603.Gt3RSg.rst

b14bd48

amiremohamadi requested review from brandtbucher, gpshead and jaraco February 12, 2020 14:20

amiremohamadi added 2 commits February 12, 2020 22:54

add test for rejecting bad HTTP method names

31ae513

Merge branch 'fix-http-header-injection' of github.com:amiremohamadi/…

5d9825f

…cpython into fix-http-header-injection

amiremohamadi requested a review from vstinner February 20, 2020 10:47

alex approved these changes Jul 17, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jul 17, 2020

jaraco approved these changes Jul 17, 2020

View reviewed changes

gvanrossum reviewed Jul 17, 2020

View reviewed changes

fix comment position

26ae130

gvanrossum approved these changes Jul 18, 2020

View reviewed changes

gvanrossum added 🤖 automerge needs backport to 3.8 only security fixes needs backport to 3.9 only security fixes labels Jul 18, 2020

miss-islington merged commit 8ca8a2e into python:master Jul 18, 2020

bedevere-bot removed the needs backport to 3.8 only security fixes label Jul 18, 2020

bedevere-bot removed the needs backport to 3.7 label Jul 18, 2020

bedevere-bot removed the needs backport to 3.6 label Jul 18, 2020

arun-mani-j pushed a commit to arun-mani-j/cpython that referenced this pull request Jul 21, 2020

bpo-39603: Prevent header injection in http methods (pythonGH-18485)

39373d2

reject control chars in http method in http.client.putrequest to prevent http header injection

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 4, 2020

bpo-39603: Prevent header injection in http methods (pythonGH-18485)

d6258af

reject control chars in http method in http.client.putrequest to prevent http header injection

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 20, 2020

bpo-39603: Prevent header injection in http methods (pythonGH-18485)

35cf638

reject control chars in http method in http.client.putrequest to prevent http header injection

xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020

bpo-39603: Prevent header injection in http methods (pythonGH-18485)

de41c3d

reject control chars in http method in http.client.putrequest to prevent http header injection

farazs-github pushed a commit to MediaTek-Labs/cpython that referenced this pull request Nov 12, 2021

Issue python#18485: MINGW: configure for shared build

7f90a47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-39603: Prevent header injection in http methods #18485

bpo-39603: Prevent header injection in http methods #18485

amiremohamadi commented Feb 12, 2020 •

edited by miss-islington

Loading

codecov bot commented Feb 12, 2020 •

edited

Loading

amiremohamadi commented Feb 12, 2020

alex left a comment

jaraco left a comment

gvanrossum left a comment

gvanrossum Jul 17, 2020

amiremohamadi commented Jul 18, 2020

gvanrossum left a comment

miss-islington commented Jul 18, 2020

miss-islington commented Jul 18, 2020

miss-islington commented Jul 18, 2020

miss-islington commented Jul 18, 2020

bedevere-bot commented Jul 18, 2020

bedevere-bot commented Jul 18, 2020

bedevere-bot commented Jul 18, 2020

orsenthil commented Jul 20, 2020

vstinner commented Aug 24, 2020

bpo-39603: Prevent header injection in http methods #18485

bpo-39603: Prevent header injection in http methods #18485

Conversation

amiremohamadi commented Feb 12, 2020 • edited by miss-islington Loading

codecov bot commented Feb 12, 2020 • edited Loading

Codecov Report

amiremohamadi commented Feb 12, 2020

alex left a comment

Choose a reason for hiding this comment

jaraco left a comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum Jul 17, 2020

Choose a reason for hiding this comment

amiremohamadi commented Jul 18, 2020

gvanrossum left a comment

Choose a reason for hiding this comment

miss-islington commented Jul 18, 2020

miss-islington commented Jul 18, 2020

miss-islington commented Jul 18, 2020

miss-islington commented Jul 18, 2020

bedevere-bot commented Jul 18, 2020

bedevere-bot commented Jul 18, 2020

bedevere-bot commented Jul 18, 2020

orsenthil commented Jul 20, 2020

vstinner commented Aug 24, 2020

amiremohamadi commented Feb 12, 2020 •

edited by miss-islington

Loading

codecov bot commented Feb 12, 2020 •

edited

Loading