Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update http parser 2.9.1 v12.x #30473

Closed
wants to merge 4 commits into from
Closed

Update http parser 2.9.1 v12.x #30473

wants to merge 4 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Nov 13, 2019 •
edited
Loading

We don't use nodejs/http-parser in 13.x and master, but it exists on 8, 10, and 12, and has security fixes.

I suggest we update it, I PRed all three branches:

I'm not sure if this is right way, maybe I should have just PRed 12.x, and the backports would flow down? Except 8.x doesn't get a lot of updates, its likely worth getting these known sec fixes in before it's EOL.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. v12.x labels Nov 13, 2019
@sam-github
Copy link
Contributor Author

Should not be included until it can be released with a backport of #30567

@sam-github sam-github force-pushed the update-http_parser-2.9.1-v12.x branch from 2fec4f5 to 9ac0a2c Compare January 7, 2020 20:37
@BridgeAR BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Jan 7, 2020
@sam-github
Copy link
Contributor Author

Since this introduces a breaking change in HTTP parsing I backported 02a0c74 on top of it.

See:

Original commit message below, since it now applies to both parsers (lhttp-parser aka "legacy" and llhttp), I changed the description to:

http: opt-in insecure HTTP header parsing

    http: llhttp opt-in insecure HTTP header parsing
    
    Allow insecure HTTP header parsing. Make clear it is insecure.
    
    See:
    - https://github.com/nodejs/node/pull/30553
    - https://github.com/nodejs/node/issues/27711#issuecomment-556265881
    - https://github.com/nodejs/node/issues/30515

@sam-github sam-github force-pushed the update-http_parser-2.9.1-v12.x branch from ffc453a to 945711e Compare January 7, 2020 21:20
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28271/

@sam-github sam-github force-pushed the update-http_parser-2.9.1-v12.x branch from 945711e to f094fab Compare January 7, 2020 21:47
@sam-github
Copy link
Contributor Author

#31253 should also be backported onto this, I'll do it once it has been approved.

@MylesBorins
Copy link
Contributor

@sam-github is this ready to land now?

@sam-github
Copy link
Contributor Author

It lacks code review, and also see #30473 (comment), it lacks a unit test (as does master).

12.x just went out, I think this can wait a couple days to get the above in order, it'll land in time for the next 12.x release.

@targos targos force-pushed the v12.x-staging branch 2 times, most recently from 3900f4a to 32e5c39 Compare January 8, 2020 16:34
@sam-github sam-github force-pushed the update-http_parser-2.9.1-v12.x branch 2 times, most recently from 11c2aac to d694bdb Compare January 9, 2020 23:58
sam-github and others added 4 commits January 9, 2020 16:08
@sam-github
deps: upgrade http-parser to v2.9.1
d48a1b5
@mcollina @sam-github
deps,http: http_parser set max header size to 8KB
fbe3c63 
Reapplying HTTP_MAX_HEADER_SIZE=8192 to http_parser.gyp.

CVE-2018-12121

PR-URL: https://github.com/nodejs-private/node-private/pull/143
Ref: https://github.com/nodejs-private/security/issues/139
Ref: https://github.com/nodejs-private/http-parser-private/pull/2
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@sam-github
http: opt-in insecure HTTP header parsing
d4bcc5f 
Allow insecure HTTP header parsing. Make clear it is insecure.

See:
- nodejs#30553
- nodejs#27711 (comment)
- nodejs#30515

PR-URL: nodejs#30567
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@sam-github
test: check that --insecure-http-parser works
2e5037b 
Test that using --insecure-http-parser will disable validation of
invalid characters in HTTP headers.

See:
- nodejs#30567

PR-URL: nodejs#31253
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@sam-github sam-github force-pushed the update-http_parser-2.9.1-v12.x branch from d694bdb to 2e5037b Compare January 10, 2020 00:09
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28337/

@sam-github sam-github removed the blocked PRs that are blocked by other issues or PRs. label Jan 10, 2020
@sam-github
Copy link
Contributor Author

@nodejs/lts PTAL. Note that in the last commit, the backport of #31253 , I duplicated the test so that it runs with the legacy as well as the llhttp parser.

@sam-github sam-github mentioned this pull request Jan 10, 2020
Closed
4 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28342/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28348/

@sam-github
Copy link
Contributor Author

This is ready to land on v12.x-staging @nodejs/lts

@sam-github sam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 13, 2020
@sam-github
Copy link
Contributor Author

Marked semver-minor, see discussion leading to #30471 (comment)

@targos
Copy link
Member

targos commented Jan 14, 2020 •
edited
Loading

Landed in c6600d1, 25f2df4, 3a7a3be, ccf9038. Thank you!

@targos targos closed this Jan 14, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
@sam-github @targos
deps: upgrade http-parser to v2.9.1
c6600d1 
PR-URL: #30473
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
@mcollina @targos
deps,http: http_parser set max header size to 8KB
25f2df4 
Reapplying HTTP_MAX_HEADER_SIZE=8192 to http_parser.gyp.

CVE-2018-12121

PR-URL: nodejs-private/node-private#143
Backport-PR-URL: #30473
Ref: nodejs-private/security#139
Ref: nodejs-private/http-parser-private#2
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Jan 14, 2020
@sam-github @targos
http: opt-in insecure HTTP header parsing
3a7a3be 
Allow insecure HTTP header parsing. Make clear it is insecure.

See:
- #30553
- #27711 (comment)
- #30515

PR-URL: #30567
Backport-PR-URL: #30473
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
@sam-github @targos
test: check that --insecure-http-parser works
ccf9038 
Test that using --insecure-http-parser will disable validation of
invalid characters in HTTP headers.

See:
- #30567

PR-URL: #31253
Backport-PR-URL: #30473
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@sam-github sam-github deleted the update-http_parser-2.9.1-v12.x branch January 15, 2020 00:13
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@mcollina @BethGriggs
deps,http: http_parser set max header size to 8KB
614ce0c 
Reapplying HTTP_MAX_HEADER_SIZE=8192 to http_parser.gyp.

CVE-2018-12121

PR-URL: nodejs-private/node-private#143
Backport-PR-URL: #30473
Ref: nodejs-private/security#139
Ref: nodejs-private/http-parser-private#2
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 11, 2020
@zsw007
deps: upgrade http-parser to v2.9.1
52c955e 
Backport d41314e

Original commit message:

    PR-URL: nodejs/node#30473
    Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: Richard Lau <riclau@uk.ibm.com>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 11, 2020
@zsw007
http: opt-in insecure HTTP header parsing
11fdae4 
Backport 496736f

Original commit message:

    Allow insecure HTTP header parsing. Make clear it is insecure.

    See:
    - nodejs/node#30553
    - nodejs/node#27711 (comment)
    - nodejs/node#30515

    PR-URL: nodejs/node#30567
    Backport-PR-URL: nodejs/node#30473
    Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
    Reviewed-By: Denys Otrishko <shishugi@gmail.com>
    Reviewed-By: James M Snell <jasnell@gmail.com>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 11, 2020
@zsw007
test: check that --insecure-http-parser works
b696d8d 
Backport ab1fcb8

Original commit message:

    Test that using --insecure-http-parser will disable validation of
    invalid characters in HTTP headers.

    See:
    - nodejs/node#30567

    PR-URL: nodejs/node#31253
    Backport-PR-URL: nodejs/node#30473
    Reviewed-By: Richard Lau <riclau@uk.ibm.com>
    Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 12, 2020
@zsw007
http: opt-in insecure HTTP header parsing
f0049c4 
Backport 496736f

Original commit message:

    Allow insecure HTTP header parsing. Make clear it is insecure.

    See:
    - nodejs/node#30553
    - nodejs/node#27711 (comment)
    - nodejs/node#30515

    PR-URL: nodejs/node#30567
    Backport-PR-URL: nodejs/node#30473
    Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
    Reviewed-By: Denys Otrishko <shishugi@gmail.com>
    Reviewed-By: James M Snell <jasnell@gmail.com>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 12, 2020
@zsw007
test: check that --insecure-http-parser works
b9eeafe 
Backport ab1fcb8

Original commit message:

    Test that using --insecure-http-parser will disable validation of
    invalid characters in HTTP headers.

    See:
    - nodejs/node#30567

    PR-URL: nodejs/node#31253
    Backport-PR-URL: nodejs/node#30473
    Reviewed-By: Richard Lau <riclau@uk.ibm.com>
    Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 12, 2020
@zsw007
deps: upgrade http-parser to v2.9.1
8672cc6 
Backport d41314e

Original commit message:

    PR-URL: nodejs/node#30473
    Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: Richard Lau <riclau@uk.ibm.com>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 12, 2020
@zsw007
http: opt-in insecure HTTP header parsing
942e61e 
Backport 496736f

Original commit message:

    Allow insecure HTTP header parsing. Make clear it is insecure.

    See:
    - nodejs/node#30553
    - nodejs/node#27711 (comment)
    - nodejs/node#30515

    PR-URL: nodejs/node#30567
    Backport-PR-URL: nodejs/node#30473
    Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
    Reviewed-By: Denys Otrishko <shishugi@gmail.com>
    Reviewed-By: James M Snell <jasnell@gmail.com>
zsw007 added a commit to ibmruntimes/node that referenced this pull request Feb 12, 2020
@zsw007
test: check that --insecure-http-parser works
ec48296 
Backport ab1fcb8

Original commit message:

    Test that using --insecure-http-parser will disable validation of
    invalid characters in HTTP headers.

    See:
    - nodejs/node#30567

    PR-URL: nodejs/node#31253
    Backport-PR-URL: nodejs/node#30473
    Reviewed-By: Richard Lau <riclau@uk.ibm.com>
    Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@sam-github @nodejs-github-bot @MylesBorins @targos @jasnell @gengjiawen @richardlau @BridgeAR @mcollina