search

MagicStack / httptools

Fast HTTP parser
MIT License
1.21k stars 83 forks source link

llhttp critical CVE's #82

Closed nlsj1985 closed 2 years ago

nlsj1985 commented 2 years ago

Hi! (Again),

Thanks for fixing the previous CVE's which i reported here last year. Today i noticed 3 CVE's (9.1 score) which are a bit obscurely reported by the nodeJS release notes, and which have been fixed in llhttp 6.0.7. [https://www.opencve.io/cve?vendor=llhttp]

This morning I tried to prepare a merge-request to bump llhttp to 6.0.9, but I came to the conclusion it's best to get some advice. It seems there's some controversie about llhttp dropping support for LF delimiting headers and requiring CR delimiters.

I'm not sure how httptools and other projects using httptools potentially could get impacted by such a change, and if there are better ways to get arround this.. or if it's not an issue.

So i stopped the process of refactoring the test-cases, to get them to just pass, and ask for help.. (I'd hoped to be of use, but this is a too big of a question for me / I'll likely hold up the process of getting this fixed properly and timely)

notes: 1) Add the release branch to .gitmodules: [submodule "vendor/llhttp"] path = vendor/llhttp url = https://github.com/nodejs/llhttp.git branch = release

2) Use a recent git version to run: git submodule update --remote (or use some other methods if you prefer not to update your git client).

3) test_parser.py contains 'HTTP/1.2' in some of the validations causing tests to fail with an exception about invalid version .. change the version to HTTP/1.1 everywhere

4) then all the remaining exceptions are about the CR 's that are required File "httptools\parser\parser.pyx", line 212, in httptools.parser.parser.HttpParser.feed_data httptools.parser.errors.HttpParserError: Missing expected CR after header value

Thanks again!

Kludex commented 2 years ago

@elprans @fantix not sure how to bring stuff to your attention, so just pinging 👀

elprans commented 2 years ago

I think the plan is fine. CRLF is per HTTP spec, let's just not forget to mention this in the release notes.

nlsj1985 commented 2 years ago

83 please review this.. it's my first github pull request in a long time ;-)

nlsj1985 commented 2 years ago

@elprans I hope it's OK to ping you.. Thx!