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.

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

http: reject control characters in http.request() #8923

Merged
merged 1 commit into from
Oct 13, 2016

Conversation

bnoordhuis
Copy link
Member

First commit:

Unsanitized paths containing line feed characters can be used for header injection and request splitting so reject them with an exception.

Second commit:

The first commit is the result of nodejs-security@ discussion but I had a change of heart. I can't see any reasonable use case for allowing control characters (characters <= 31) but I can think of several scenarios where they can be used to exploit software bugs so let's ban them altogether.

There is a a potential compatibility issue in that tabs in paths have been observed in the wild, but, to the best of my knowledge, only in requests from buggy HTTP clients. Here too I don't see a reason to allow them in requests that node.js initiates.

@nodejs/http @nodejs/security

@bnoordhuis bnoordhuis added the http Issues or PRs related to the http subsystem. label Oct 4, 2016
@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Oct 4, 2016
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jbergstroem
Copy link
Member

@@ -43,13 +43,14 @@ function ClientRequest(options, cb) {
if (self.agent && self.agent.protocol)
expectedProtocol = self.agent.protocol;

if (options.path && / /.test(options.path)) {
if (options.path && /[\n\r ]/.test(options.path)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to reject tabs here as well? The logic being that when you send an unescaped tab to servers like Apache the server will interpret the request as the tab splitting the path and the HTTP version. Instead of failing silently and allowing malformed requests through, the developer will get an error thrown that clearly says "You have to escape this"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilsken second commit includes the tabs.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lpinca oh sorry I missed that

Copy link
Contributor

@silverwind silverwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with suggestion

// why it only scans for spaces because those are guaranteed to create
// an invalid request.
// well, and b) possibly too restrictive for real-world usage.
// Restrict the filter to control characters and spaces.
throw new TypeError('Request path contains unescaped characters');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RangeError?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, but for a different PR because people may want to be careful and consider that semver-major?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think this would be a unecessary breaking change if we consider error types as API. So, LGTM as-is.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}, /contains unescaped characters/);
function* bad() {
for (let i = 0; i <= 32; i += 1)
yield 'bad' + String.fromCharCode(i) + 'path';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it doesn't add any real value but what about using a template literal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially had that but I felt it was less readable than concatenation. Consider:

yield 'bad' + String.fromCharCode(i) + 'path';

Vs.

yield `bad${String.fromCharCode(i)}path`;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah no big deal.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jasnell jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. error-handling labels Oct 6, 2016
@jasnell
Copy link
Member

jasnell commented Oct 6, 2016

Marking semver-major

@jasnell
Copy link
Member

jasnell commented Oct 6, 2016

@nodejs/ctc ... I'd like to include this in v7 assuming it lands in master. Need ctc approval tho as a semver-major.

@addaleax
Copy link
Member

addaleax commented Oct 6, 2016

Mhhh, I would like to hear other opinions, but this seems like more of a security bug fix than an actual semver-major change, and I Can’t Think Of Any Situation™ in which somebody would rely on the current behaviour?

@jasnell
Copy link
Member

jasnell commented Oct 6, 2016

It can be viewed in both ways. There aren't any good reasons why someone would rely on the current behavior but there are examples of software using tab and space where they do not belong. The other control characters are much more rare and generally indicate buggy or unsafe implementations. I'm happy with this landing as less than a semver-major as long as the change is documented clearly in the release notes of whatever release it lands in.

@addaleax
Copy link
Member

addaleax commented Oct 6, 2016

@jasnell In that case, how about considering the first commit here as a semver-patch one and the second as semver-major?

and space where they do not belong

(That’s forbidden already.)

@jasnell
Copy link
Member

jasnell commented Oct 6, 2016

I'd say if we're going to go that route, let's just treat them both as semver-patch and document the change in the release notes.

@Trott
Copy link
Member

Trott commented Oct 6, 2016

Maybe we can revisit this whole "semver majors need CTC approval to land in v7" thing. Did that even come out of the CTC or was that an LTS WG decision?

@jasnell
Copy link
Member

jasnell commented Oct 6, 2016

@Trott ... let's talk about that in the v7.0.0-proposal PR: #8503

@bnoordhuis
Copy link
Member Author

Mhhh, I would like to hear other opinions, but this seems like more of a security bug fix than an actual semver-major change

Correct. I consider this a security/correctness fix and as such exempt from our regular semver policies. I plan on back-porting it to the release branches once it lands in master. I'll remove the semver-major label.

@bnoordhuis bnoordhuis added lts-watch-v4.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 7, 2016
@bnoordhuis
Copy link
Member Author

Unsanitized paths containing line feed characters can be used for
header injection and request splitting so reject them with an exception.

There seems to be no reasonable use case for allowing control characters
(characters <= 31) while there are several scenarios where they can be
used to exploit software bugs so reject control characters altogether.

PR-URL: nodejs#8923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
@bnoordhuis bnoordhuis closed this Oct 13, 2016
@bnoordhuis bnoordhuis deleted the reject-bad-http-request-chars branch October 13, 2016 11:40
jasnell pushed a commit that referenced this pull request Oct 14, 2016
Unsanitized paths containing line feed characters can be used for
header injection and request splitting so reject them with an exception.

There seems to be no reasonable use case for allowing control characters
(characters <= 31) while there are several scenarios where they can be
used to exploit software bugs so reject control characters altogether.

PR-URL: #8923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
@MylesBorins
Copy link
Contributor

@bnoordhuis @jasnell I've added the v6.x watch label to this. Is this safe to land now or is it something we should let live on v7 a bit longer? Should this come in a patch release or a minor release?

@MylesBorins
Copy link
Contributor

ping @bnoordhuis

@MylesBorins
Copy link
Contributor

this should land with dbf4bf2

@bnoordhuis
Copy link
Member Author

@thealphanerd See #8923 (comment), this can land in a patch release.

MylesBorins pushed a commit that referenced this pull request Mar 7, 2017
Unsanitized paths containing line feed characters can be used for
header injection and request splitting so reject them with an exception.

There seems to be no reasonable use case for allowing control characters
(characters <= 31) while there are several scenarios where they can be
used to exploit software bugs so reject control characters altogether.

PR-URL: #8923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
MylesBorins pushed a commit that referenced this pull request Mar 7, 2017
Unsanitized paths containing line feed characters can be used for
header injection and request splitting so reject them with an exception.

There seems to be no reasonable use case for allowing control characters
(characters <= 31) while there are several scenarios where they can be
used to exploit software bugs so reject control characters altogether.

PR-URL: #8923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Unsanitized paths containing line feed characters can be used for
header injection and request splitting so reject them with an exception.

There seems to be no reasonable use case for allowing control characters
(characters <= 31) while there are several scenarios where they can be
used to exploit software bugs so reject control characters altogether.

PR-URL: #8923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Unsanitized paths containing line feed characters can be used for
header injection and request splitting so reject them with an exception.

There seems to be no reasonable use case for allowing control characters
(characters <= 31) while there are several scenarios where they can be
used to exploit software bugs so reject control characters altogether.

PR-URL: #8923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins added a commit that referenced this pull request Mar 21, 2017
Notable changes

* performance: The performance of several APIs has been improved.
  - `Buffer.compare()` is up to 35% faster on average. (Brian White)
    #10927
  - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
    #10895
  - `fs.*statSync()` functions are now up to 9.3% faster on average.
    (Brian White) #11522
  - `os.loadavg` is up to 151% faster. (Brian White)
    #11516
  - `process.memoryUsage()` is up to 34% faster. (Brian White)
    #11497
  - `querystring.unescape()` for `Buffer`s is 15% faster on average.
    (Brian White) #10837
  - `querystring.stringify()` is up to 7.8% faster on average.
    (Brian White) #10852
  - `querystring.parse()` is up to 21% faster on average. (Brian White)
    #10874

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* child_process:
  - `spawnSync` now returns a null `status` when child is terminated by
    a signal. (cjihrig) #11288
  - This fixes the behavior to act like `spawn()` does.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923
  - Debug messages have been added for cases when headers contain
    invalid values. (Evan Lucas)
    #9195

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186

* timers:
  - Timer callbacks now always maintain order when interacting with
    domain error handling. (John Barboza)
    #10522

PR-URL: #11759
MylesBorins added a commit that referenced this pull request Mar 21, 2017
Notable Changes:

* buffer:
  - The performance of `.toJSON()` is now up to 2859% faster on average
    (Brian White) #10895

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186
MylesBorins added a commit that referenced this pull request Mar 21, 2017
Notable Changes:

* buffer:
  - The performance of `.toJSON()` is now up to 2859% faster on average
    (Brian White) #10895

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186

PR-URL: #11760
MylesBorins added a commit that referenced this pull request Mar 21, 2017
Notable changes

* performance: The performance of several APIs has been improved.
  - `Buffer.compare()` is up to 35% faster on average. (Brian White)
    #10927
  - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
    #10895
  - `fs.*statSync()` functions are now up to 9.3% faster on average.
    (Brian White) #11522
  - `os.loadavg` is up to 151% faster. (Brian White)
    #11516
  - `process.memoryUsage()` is up to 34% faster. (Brian White)
    #11497
  - `querystring.unescape()` for `Buffer`s is 15% faster on average.
    (Brian White) #10837
  - `querystring.stringify()` is up to 7.8% faster on average.
    (Brian White) #10852
  - `querystring.parse()` is up to 21% faster on average. (Brian White)
    #10874

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* child_process:
  - `spawnSync` now returns a null `status` when child is terminated by
    a signal. (cjihrig) #11288
  - This fixes the behavior to act like `spawn()` does.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923
  - Debug messages have been added for cases when headers contain
    invalid values. (Evan Lucas)
    #9195

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186

* timers:
  - Timer callbacks now always maintain order when interacting with
    domain error handling. (John Barboza)
    #10522

PR-URL: #11759
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    Notable Changes:

    * buffer:
      - The performance of `.toJSON()` is now up to 2859% faster on average
        (Brian White) nodejs/node#10895

    * IPC:
      - Batched writes have been enabled for process IPC on platforms that
        support Unix Domain Sockets. (Alexey Orlenko)
        nodejs/node#10677
      - Performance gains may be up to 40% for some workloads.

    * http:
      - Control characters are now always rejected when using
        `http.request()`. (Ben Noordhuis)
        nodejs/node#8923

    * node:
      - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
        nodejs/node#10186

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    Notable changes

    * performance: The performance of several APIs has been improved.
      - `Buffer.compare()` is up to 35% faster on average. (Brian White)
        nodejs/node#10927
      - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
        nodejs/node#10895
      - `fs.*statSync()` functions are now up to 9.3% faster on average.
        (Brian White) nodejs/node#11522
      - `os.loadavg` is up to 151% faster. (Brian White)
        nodejs/node#11516
      - `process.memoryUsage()` is up to 34% faster. (Brian White)
        nodejs/node#11497
      - `querystring.unescape()` for `Buffer`s is 15% faster on average.
        (Brian White) nodejs/node#10837
      - `querystring.stringify()` is up to 7.8% faster on average.
        (Brian White) nodejs/node#10852
      - `querystring.parse()` is up to 21% faster on average. (Brian White)
        nodejs/node#10874

    * IPC:
      - Batched writes have been enabled for process IPC on platforms that
        support Unix Domain Sockets. (Alexey Orlenko)
        nodejs/node#10677
      - Performance gains may be up to 40% for some workloads.

    * child_process:
      - `spawnSync` now returns a null `status` when child is terminated by
        a signal. (cjihrig) nodejs/node#11288
      - This fixes the behavior to act like `spawn()` does.

    * http:
      - Control characters are now always rejected when using
        `http.request()`. (Ben Noordhuis)
        nodejs/node#8923
      - Debug messages have been added for cases when headers contain
        invalid values. (Evan Lucas)
        nodejs/node#9195

    * node:
      - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
        nodejs/node#10186

    * timers:
      - Timer callbacks now always maintain order when interacting with
        domain error handling. (John Barboza)
        nodejs/node#10522

    PR-URL: nodejs/node#11759

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    Notable Changes:

    * buffer:
      - The performance of `.toJSON()` is now up to 2859% faster on average
        (Brian White) nodejs/node#10895

    * IPC:
      - Batched writes have been enabled for process IPC on platforms that
        support Unix Domain Sockets. (Alexey Orlenko)
        nodejs/node#10677
      - Performance gains may be up to 40% for some workloads.

    * http:
      - Control characters are now always rejected when using
        `http.request()`. (Ben Noordhuis)
        nodejs/node#8923

    * node:
      - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
        nodejs/node#10186

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    Notable changes

    * performance: The performance of several APIs has been improved.
      - `Buffer.compare()` is up to 35% faster on average. (Brian White)
        nodejs/node#10927
      - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
        nodejs/node#10895
      - `fs.*statSync()` functions are now up to 9.3% faster on average.
        (Brian White) nodejs/node#11522
      - `os.loadavg` is up to 151% faster. (Brian White)
        nodejs/node#11516
      - `process.memoryUsage()` is up to 34% faster. (Brian White)
        nodejs/node#11497
      - `querystring.unescape()` for `Buffer`s is 15% faster on average.
        (Brian White) nodejs/node#10837
      - `querystring.stringify()` is up to 7.8% faster on average.
        (Brian White) nodejs/node#10852
      - `querystring.parse()` is up to 21% faster on average. (Brian White)
        nodejs/node#10874

    * IPC:
      - Batched writes have been enabled for process IPC on platforms that
        support Unix Domain Sockets. (Alexey Orlenko)
        nodejs/node#10677
      - Performance gains may be up to 40% for some workloads.

    * child_process:
      - `spawnSync` now returns a null `status` when child is terminated by
        a signal. (cjihrig) nodejs/node#11288
      - This fixes the behavior to act like `spawn()` does.

    * http:
      - Control characters are now always rejected when using
        `http.request()`. (Ben Noordhuis)
        nodejs/node#8923
      - Debug messages have been added for cases when headers contain
        invalid values. (Evan Lucas)
        nodejs/node#9195

    * node:
      - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
        nodejs/node#10186

    * timers:
      - Timer callbacks now always maintain order when interacting with
        domain error handling. (John Barboza)
        nodejs/node#10522

    PR-URL: nodejs/node#11759

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.