Skip to content

llhttp critical CVE's #82

Closed
Closed

Description

@nlsj1985

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!

Activity

  1. Kludex commented on Aug 31, 2022

    @Kludex
    Contributor

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

  2. elprans commented on Aug 31, 2022

    @elprans
    Member

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

  3. nlsj1985 commented on Sep 1, 2022

    @nlsj1985
    ContributorAuthor

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

  4. nlsj1985 commented on Sep 5, 2022

    @nlsj1985
    ContributorAuthor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions