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

path: fix regression in posix.normalize #19520

Closed
wants to merge 2 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Mar 21, 2018

Fixes a regression introduced in
4ae320f
The posix version of normalize should not treat backslash as a path
separator.

Fixes: #19519

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

Fixes a regression introduced in
nodejs@4ae320f
The posix version of normalize should not treat backslash as a path
separator.

Fixes: nodejs#19519
@nodejs-github-bot nodejs-github-bot added the path Issues and PRs related to the path subsystem. label Mar 21, 2018
lib/path.js Outdated
@@ -364,7 +369,8 @@ const win32 = {

var tail;
if (rootEnd < len)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can braces be added for the if and else here if we start having multi-line conditional bodies?

@targos
Copy link
Member Author

targos commented Mar 22, 2018

@targos targos mentioned this pull request Mar 22, 2018
4 tasks
@MylesBorins
Copy link
Contributor

Should we fast track this patch / a 9.9.1 or can this wait for next week?

@targos
Copy link
Member Author

targos commented Mar 22, 2018

It's difficult to estimate the impact (I suppose backslashes are not common in unixes paths). It can probably wait and land with the next security release.

@targos
Copy link
Member Author

targos commented Mar 23, 2018

Landed in a0adf56

@targos targos closed this Mar 23, 2018
@targos targos deleted the fix-path-normalize-posix branch March 23, 2018 18:48
targos added a commit that referenced this pull request Mar 23, 2018
Fixes a regression introduced in [1].
The posix version of normalize should not treat backslash as a path
separator.

[1] 4ae320f2

PR-URL: #19520
Fixes: #19519
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this pull request Mar 24, 2018
Fixes a regression introduced in [1].
The posix version of normalize should not treat backslash as a path
separator.

[1] 4ae320f2

PR-URL: #19520
Fixes: #19519
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@SimenB
Copy link
Member

SimenB commented Mar 26, 2018

Any plans for a patch with this?

@targos
Copy link
Member Author

targos commented Mar 26, 2018

This is ready to land in the next patch release, which should happen tomorrow: https://nodejs.org/en/blog/vulnerability/march-2018-security-releases/

targos added a commit that referenced this pull request Mar 27, 2018
Notable changes:

* cluster:
  - Add support for `NODE_OPTIONS="--inspect"` (Sameer Srivastava)
    #19165
* crypto:
  - Expose the public key of a certificate (Hannes Magnusson)
    #17690
* n-api:
  - Add `napi_fatal_exception` to trigger an `uncaughtException` in
    JavaScript (Mathias Buus)
    #19337
* path:
  - Fix regression in `posix.normalize` (Michaël Zasso)
    #19520
* stream:
  - Improve stream creation performance (Brian White)
    #19401
* Added new collaborators
  - [BethGriggs](https://github.com/BethGriggs) Beth Griggs
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
This is a security release. All Node.js users should consult the
security release summary at:

https://nodejs.org/en/blog/vulnerability/march-2018-security-releases/

for details on patched vulnerabilities.

Fixes for the following CVEs are included in this release:

* CVE-2018-7158
* CVE-2018-7159
* CVE-2018-7160

Notable changes:

* Upgrade to OpenSSL 1.0.2o: Does not contain any security fixes that
  are known to impact Node.js.
* **Fix for inspector DNS rebinding vulnerability (CVE-2018-7160)**:
  A malicious website could use a DNS rebinding attack to trick a web
  browser to bypass same-origin-policy checks and allow HTTP
  connections to localhost or to hosts on the local network,
  potentially to an open inspector port as a debugger, therefore
  gaining full code execution access. The inspector now only allows
  connections that have a browser `Host` value of `localhost` or
  `localhost6`.
* **Fix for `'path'` module regular expression denial of service
  (CVE-2018-7158)**: A regular expression used for parsing POSIX an
  Windows paths could be used to cause a denial of service if an
  attacker were able to have a specially crafted path string passed
  through one of the impacted `'path'` module functions.
* **Reject spaces in HTTP `Content-Length` header values
  (CVE-2018-7159)**: The Node.js HTTP parser allowed for spaces inside
  `Content-Length` header values. Such values now lead to rejected
  connections in the same way as non-numeric values.
* **Update root certificates**: 5 additional root certificates have
  been added to the Node.js binary and 30 have been removed.

* cluster:
  - Add support for `NODE_OPTIONS="--inspect"` (Sameer Srivastava)
    #19165
* crypto:
  - Expose the public key of a certificate (Hannes Magnusson)
    #17690
* n-api:
  - Add `napi_fatal_exception` to trigger an `uncaughtException` in
    JavaScript (Mathias Buus)
    #19337
* path:
  - Fix regression in `posix.normalize` (Michaël Zasso)
    #19520
* stream:
  - Improve stream creation performance (Brian White)
    #19401
* Added new collaborators
  - [BethGriggs](https://github.com/BethGriggs) Beth Griggs

PR-URL: https://github.com/nodejs-private/node-private/pull/111
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants