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

deps: backport f617ccc644 from uv upstream #6392

Closed
wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

protect against realpath(3) exploit

ref: isaacs/node-glob#259 (comment)

Original commit message:

unix: error on realpath if PATH_MAX is undefined

Currently when PATH_MAX is undefined realpath will default to using 4096.
There is a potential stack overflow attack that can be mitigated by having
PATH_MAX defined. This change conservatively errors if a system does not
have PATH_MAX defined.

This change also explicitly includes `limits.h` to ensure that all platforms
have PATH_MAX defined if it is available.

Ref: http://pubs.opengroup.org/onlinepubs/9699919799/functions/realpath.html

Refs: nodejs/node#2680 (comment)
PR-URL: #843
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

protect against realpath(3) exploit

ref: isaacs/node-glob#259 (comment)

Original commit message:

    unix: error on realpath if PATH_MAX is undefined

    Currently when PATH_MAX is undefined realpath will default to using 4096.
    There is a potential stack overflow attack that can be mitigated by having
    PATH_MAX defined. This change conservatively errors if a system does not
    have PATH_MAX defined.

    This change also explicitly includes `limits.h` to ensure that all platforms
    have PATH_MAX defined if it is available.

    Ref: http://pubs.opengroup.org/onlinepubs/9699919799/functions/realpath.html

    Refs: nodejs#2680 (comment)
    PR-URL: nodejs#843
    Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
    Reviewed-By: Fedor Indutny <fedor@indutny.com>
    Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Apr 26, 2016

/cc @jasnell I'd love to see this land for tomorrow's v6 release

@MylesBorins MylesBorins added this to the 6.0.0 milestone Apr 26, 2016
@MylesBorins MylesBorins added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Apr 26, 2016
@MylesBorins
Copy link
Contributor Author

/cc @saghul

we should have a @nodejs/libuv 😄

@benjamingr
Copy link
Member

I'm missing something - shouldn't it land in v6, but also in v5 and v4? Isn't this is a fix for a potential exploit and not a new feature or API breakage? Why would it be semver-major?

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Apr 26, 2016

@benjamingr this is only relevant to libuv 1.9 which hasn't landed in v4 or v5. If it is ever backported it would be a future version that came with this fix

edit: i've updated my comment above to be more explicit that I'd like to see included in the v6 release tomorrow

@ChALkeR
Copy link
Member

ChALkeR commented Apr 26, 2016

+1 to including this into v6 as a safeguard.
Note that it doesn't break anything on our tested platforms.

@jbergstroem
Copy link
Member

Shouldn't we wait for a new upstream and have that land instead? This will create discrepancies with using shared libraries.

@MylesBorins
Copy link
Contributor Author

@bnoordhuis
Copy link
Member

See libuv/libuv#843 (comment) - I'd be chagrined if we get reports of broken builds because of a fix for an academic issue.

@ChALkeR
Copy link
Member

ChALkeR commented Apr 26, 2016

@bnoordhuis But this issue isn't academic on systems where the build would break — those have PATH_MAX undefined.

Btw, is there a way to determine using ifdefs if passing a NULL to realpath(3) is supported? False negatives would be fine, false positive wouldn't.

@bnoordhuis
Copy link
Member

It's academic because there is no platform that we support where PATH_MAX > 4096.

Btw, is there a way to determine using ifdefs if passing a NULL to realpath(3) is supported?

Not in a way that is 100% foolproof.

@saghul
Copy link
Member

saghul commented Apr 26, 2016

I've said this more than once already: please do not backport libuv fixes into Node.

The reasons are obvious: users won't know what is backported and what isn't, double bug reports, and so on. Let's not set that precedent.

@ChALkeR
Copy link
Member

ChALkeR commented Apr 26, 2016

@bnoordhuis You are talking only about the platforms that we support, bu if this passes CI, then no platform that we support has PATH_MAX undefined. And we can't be sure about all the other platforms out there having PATH_MAX ≤ 4096.

@MylesBorins MylesBorins removed this from the 6.0.0 milestone Apr 26, 2016
@MylesBorins
Copy link
Contributor Author

@saghul my apologies. I got over zealous in trying to chase down failures in our smoke tests (specifically the ones in node-glob).

I'm going to go ahead and close this since there is a possibility of it being reverted (or at least augmented).

libuv/libuv#843 (comment)

@saghul
Copy link
Member

saghul commented Apr 26, 2016

No problem. I know you had the best intentions, and sorry for my terse wording :-S

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants