Error out when REAL_PATH is undefined on a system by MylesBorins · Pull Request #843 · libuv/libuv

MylesBorins · 2016-04-25T16:28:02Z

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 comes with a new header specifically for freeBSD
to ensure that PATH_MAX will be available on that platform.

ref: http://pubs.opengroup.org/onlinepubs/9699919799/functions/realpath.html
ref: nodejs/node#2680 (comment)
ref: isaacs/node-glob#259 (comment)

MylesBorins · 2016-04-25T16:28:18Z

/cc @saghul

saghul · 2016-04-25T16:41:26Z

    return PATH_MAX;
 #else
-    return 4096;
+#error "PATH_MAX undefined"


"PATH_MAX undefined in the current platform"

saghul · 2016-04-25T16:48:23Z

 # include <sys/filio.h>
 # include <sys/ioctl.h>
 # include <sys/wait.h>
+# include <sys/syslimits.h> /* PATH_MAX, IOV_MAX */


You need to include this in src/unix/fs.c, which is where we need it.

src/unix/fs.c doesn’t actually include limits.h by itself, does it? Maybe that’s all that’s needed?

I'll move it over. I had it there originally but noticed that limits.h was not being included in src/unix/fs.c but in core.c, so I thought if it was included there

Other bits like IOV_MAX are only in syslimits on freeBSD.

I'll drop the first commit and move this all into fs.c

That was what weirded me out. limits.h doesn't include MAX_PATH on freebsd

MylesBorins · 2016-04-25T17:06:01Z

This change passes the node.js CI --> https://ci.nodejs.org/job/node-test-commit-freebsd/2188/nodes=freebsd10-64/console

saghul · 2016-04-25T17:24:36Z

LGTM

/cc @libuv/collaborators WDYT?

indutny · 2016-04-25T19:31:23Z

    defined(__FreeBSD__)    ||                                            \
    defined(__OpenBSD__)    ||                                            \
    defined(__NetBSD__)
+# include <sys/syslimits.h> /* PATH_MAX, IOV_MAX */


Does it needs to be included on OS X?

There was no failure compiling on osx without this include. PATH_MAX is defined in limits.h on osx AFAIK, but I would appreciate verification on that

@thealphanerd You could run another CI against just including limits.h in this file (without sys/syslimits.h), unconditionally, and across platforms, right? I have to admit I can’t really believe FreeBSD ignores POSIX on something like where PATH_MAX is defined…

hmmm... so changing it to limits works

--> https://ci.nodejs.org/job/node-test-commit-freebsd/2195/

So why does every system but bsd have the macros here?

@thealphanerd … doesn’t that CI run imply that BSD does have the macros there?

I think there should be just an #include <limits.h> where the other includes are. On systems that are not FreeBSD, things just worked because one of the other system headers includes limits.h itself.

On my Linux machine, the chain is uv.h -> uv-unix.h -> dirent.h -> limits.h, and I guess something similar is going on for OS X.

I've pushed an update that explicitly includes limits.h with the other headers. Seems like a good defensive move

indutny · 2016-04-25T19:31:34Z

One question, otherwise LGTM

cjihrig · 2016-04-25T20:21:25Z

LGTM

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 ref: nodejs/node#2680 (comment)

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>

saghul · 2016-04-26T07:25:02Z

Landed in f617ccc, thanks!

MylesBorins · 2016-04-26T07:34:26Z

🎉

@saghul should we backport this to node prior to the v6 release? I can't see any past instance of a floating patch from uv -> node

saghul · 2016-04-26T07:40:15Z

Please, don't. Next libuv release will have and Node will get it, I don't see the need for it as it doesn't solve a real problem found in the wild.

bnoordhuis · 2016-04-26T07:53:15Z

I'm not 100% convinced this fix is correct. On FreeBSD (and, I think, the BSDs in general), PATH_MAX doesn't have to be defined unless _POSIX_C_SOURCE is defined. We only define it on Linux currently, and only for the gyp build.

EDIT: Also, there are long lines in the commit log. :-(

saghul · 2016-04-26T08:10:55Z

Ouch, mea culpa :-( If I was wrong and this is not the right fix please feel free to revert. I relied on the CI runs since I have no FreeBSD at hand right now, but completely forgot about GYP/autotools, sorry.

MylesBorins · 2016-04-26T08:13:51Z

Sorry for leaving out context. The new realpath implementation was causing a breakage in glob

--> isaacs/node-glob#259 (comment)

@isaacs was hesitant to include a fix as he was worried about how secure the new implementation was and wouldn't land the fix.

tbqh I just wanted to make sure we were being extra safe with the release tomorrow

I do think that if there is any chance this is incorrect and will break builds, as alluded to by @bnoordhuis that this should be reverted. I've closed the PR on node and will not attempt to backport in the future.

saghul · 2016-04-26T08:18:05Z

No worries.

While the new realpath implementation surfaces some platform differences in some edge cases (IIRC) in glob, this fix does nothing to change that.

Now, if people feel super strongly about this (FTR, I don't) and a proper fix lands today I don't mind cutting a 1.9.1 release this evening. /cc @libuv/collaborators

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: libuv#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 mentioned this pull request Apr 25, 2016

fs.realpath 70x slower than native nodejs/node#2680

Closed

saghul reviewed Apr 25, 2016
View reviewed changes

MylesBorins force-pushed the real-path-fix branch from 10b029b to 5433911 Compare April 25, 2016 16:45

saghul reviewed Apr 25, 2016
View reviewed changes

MylesBorins force-pushed the real-path-fix branch 2 times, most recently from 3a75034 to 09b3f05 Compare April 25, 2016 16:56

MylesBorins changed the title ~~WIP: Error out when REAL_PATH is undefined on a system~~ Error out when REAL_PATH is undefined on a system Apr 25, 2016

indutny reviewed Apr 25, 2016
View reviewed changes

MylesBorins force-pushed the real-path-fix branch from 09b3f05 to 22d582b Compare April 25, 2016 21:31

saghul closed this Apr 26, 2016

bnoordhuis mentioned this pull request Apr 26, 2016

deps: backport f617ccc644 from uv upstream nodejs/node#6392

Closed

addaleax mentioned this pull request Apr 27, 2016

fs: optimize realpath using uv_fs_realpath() nodejs/node#3594

Closed

Conversation

MylesBorins commented Apr 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Apr 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MylesBorins Apr 25, 2016 via email

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Apr 25, 2016

Uh oh!

saghul commented Apr 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MylesBorins Apr 25, 2016 via email

Choose a reason for hiding this comment

Uh oh!

addaleax Apr 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MylesBorins Apr 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indutny commented Apr 25, 2016

Uh oh!

cjihrig commented Apr 25, 2016

Uh oh!

saghul commented Apr 26, 2016

Uh oh!

MylesBorins commented Apr 26, 2016

Uh oh!

saghul commented Apr 26, 2016

Uh oh!

bnoordhuis commented Apr 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saghul commented Apr 26, 2016

Uh oh!

MylesBorins commented Apr 26, 2016

Uh oh!

saghul commented Apr 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MylesBorins commented Apr 25, 2016 •

edited

Loading

addaleax Apr 25, 2016 •

edited

Loading

MylesBorins Apr 25, 2016 •

edited

Loading

bnoordhuis commented Apr 26, 2016 •

edited

Loading