No abort on require() with null bytes

[rvagg]

1. Handle errno in InternalModuleReadFile() returning Undefined so we don't get an abort() when uv_fs_read() raises an error (currently it's only on \u0000 that I can find an error but perhaps there are others).
2. Apply the nullBytes() check in readPackage() so we get a proper error rather than a truncated one. This is also consistent with the behaviour of require() on null bytes prior to the introduction of the fast-path reads that now exist in v4.x+.

This comes from 1bbf8d0 which is in v4.x and v6.x. In v0.12 and prior you'd get a normal "null bytes not allowed" error thrown when using a require() involving \u0000.

[bnoordhuis]

The changes to node_file.cc don't look correct to me at all. What error does uv_fs_read() return? If it's UV_EBADF, it means you did something wrong and it should fail.

[addaleax]

This is unfortunately, if I have understood the situation correctly, a breaking change à la #6413?

[ChALkeR]

Blocked by #6413.

[rvagg]

@bnoordhuis if uv_fs_read() fails with < 0 then it continues on with path resolution as per Module._findPath() and then if none is found results in a new Error("Cannot find module '" + request + "'") (& err.code = 'MODULE_NOT_FOUND).

This matches the behaviour before 1bbf8d0 in readPackage():

  try {
    var jsonPath = path.resolve(requestPath, 'package.json');
    var json = fs.readFileSync(jsonPath, 'utf8');
  } catch (e) {
    return false;
  }

i.e. this is equivalent to:

  const json = internalModuleReadFile(path._makeLong(jsonPath));

  if (json === undefined) {
    return false;
  }

[rvagg]

I should also note that this is failing on all machines in CI but is fine on my machine so I suspect there's something wonky going on with git. Will try again too.

I'll also get rid of the internal/ stuff and just duplicate the function with a note about the duplication as this needs to get fixed in v4.x too. @addaleax & @ChALkeR do you think that's an acceptable solution to undo the block?

[ChALkeR]

@rvagg Imo there is nothing preventing landing #6413 (or it's alternative) for v7.0, as graceful-fs@3 was updated to not trigger the warning, and hence gulp is fine. See #6413 (comment). I don't think that we should introduce more hacks because of that at the moment.

If that won't happen for some reason, then duplicating would be sufficient as a work-around. It should also be fine for v4.x.

[rvagg]

OK, well given the current status what I might do is adjust this PR to duplicate, then introduce a new PR that undoes the duplication that can land when it's unblocked, that way the original can go through to v4 and the new can update for v7+.

[bnoordhuis]

if uv_fs_read() fails with < 0

That doesn't answer my question on why it's failing. What error does it return?

[rvagg]

That doesn't answer my question on why it's failing. What error does it return?

I haven't cared so far because it's irelevant for the sake of restoring previous functionality. There is no abort() in the old code path and the introduction of an abort() in a non-semver-major is clearly unintentional.

Testing it now with \u0000 shows a EISDIR and that's because it starts resolution with ./.node_modules/. I still don't see the relevance of this line of enquiry though, perhaps you can spell it out more clearly why it would matter now?

[rvagg]

I have no idea why this is failing on CI but working fine for me locally on both OSX and Linux. I'm guessing it's a fixture thing.

https://ci.nodejs.org/job/node-test-commit/4782/ is the latest attempt if anyone else wants to poke at it.

[rvagg]

Figured out CI, it's cause I was running with a NODE_PATH locally but of course CI is clean. With the checks in place as they are now, the performance regression seems fairly minimal, at least it looks statistically insignificant when using the http://benchmarking.nodejs.org/ benchmarks for before/after this change.

Here's CI: https://ci.nodejs.org/job/node-test-commit/4788/

PTAL

[ChALkeR]

#6413 should land on Monday.

[ChALkeR]

Ah, I see a follow-up #8292, thanks.

[ChALkeR]

After splitting this in two PRs, this one is no longer blocked on #6413, #8292 is.

[bnoordhuis]

Testing it now with \u0000 shows a EISDIR and that's because it starts resolution with ./.node_modules/.

Okay, that means you need to do the null byte check before passing it on to C++ land. Either that or it should be uv_fs_open() that gets (and deals with) the error, not uv_fs_read().

The change as it is would cause divergence across platforms. FreeBSD allows reading from a file descriptor that refers to a directory, other Unices don't.

I still don't see the relevance of this line of enquiry though, perhaps you can spell it out more clearly why it would matter now?

The changes to node_file.cc look like daubing on more code to make the problem go away without really understanding what the problem is in the first place. More or less confirmed by the above.

[rvagg]

@bnoordhuis I still don't quite get where the problem is, and I entirely disagree with your assessment of "without really understanding what the problem is", so I'll explain that.

Okay, that means you need to do the null byte check before passing it on to C++ land. Either that or it should be uv_fs_open() that gets (and deals with) the error, not uv_fs_read().

It is, that's what this addition is for:

Module._findPath = function(request, paths, isMain) {
+  nullCheck(request);

That will throw in the event of a null byte so InternalModuleReadFile in node_file.cc should not get a null byte now, that's precisely what's being fixed.

There may be additional errors, however, I haven't introduced tests for any but consider this. The previous incarnation before the optimisations were introduced did:

  try {
    var jsonPath = path.resolve(requestPath, 'package.json');
    var json = fs.readFileSync(jsonPath, 'utf8');
  } catch (e) {
    return false;
  }

i.e. catching any error thrown by fs.

The new version effectively did this with uv_fs_open() thanks to:

   if (fd < 0) {
     return;
  }

which ends up being caught at by the replacement of the try/catch:

  const json = internalModuleReadFile(path._makeLong(jsonPath));

  if (json === undefined) {
    return false;
  }

but any errors occuring within uv_fs_read() would result in a plain abort() thanks to the CHECK_GE(numchars, 0); which has now been replaced by:

-    CHECK_GE(numchars, 0);
+    if (numchars < 0) {
+      break;
+    }

followed by:

+  if (numchars < 0) {
+    args.GetReturnValue().Set(Undefined(env->isolate()));
+  } else {

which perhaps could be just a return; or perhaps the other return could be an explicit undefined return, I don't know (they are inconsistent and I'm fine with making them consistent if you have a recommendation).

So, previously, errors encountered by uv_fs_open() would result in an error bubbling up to either continue the resolution algorithm with different files or "Cannot find module" if the resolution has concluded, but errors arising out of uv_fs_read() would result in a plain abort(), regardless of where it was in the resolution algorithm and what type of error it was. Now, errors caused by null bytes should not be encountered because they are handled in JavaScript first and result in the correct error being thrown (the "no null bytes" v0.12 style error) and further errors encountered by both uv_fs_open() and uv_fs_read() are handled in the same way, that is, they cancel this pass in the resolution algorithm which results in continuing on to the next type of file or a "Cannot find module" error.

I believe this is consistent with the previous behaviour which used fs.readFileSync() which uses fs.readSync() which uses uv_fs_read() which does a ThrowUVException() when called synchronously on receiving a <0 result.

So, there are essentially two problems being fixed here, and two commits to fix them. One is to get the abort() out of the way no matter what happens and make the module resolution & read algorithm behave as it previously did. The other is to restore the nullCheck() to the fast reads in module.js so that if null bytes are encountered they result in the same error as if you use fs directly and the same error that occurred before (and also make consistent any platform-inconsistencies as you mentioned).

[bnoordhuis]

If I understand you correctly, the node_file.cc changes fix the case of trying to read a package.json that is a directory, not a file, but going by your description that's accidental more than intentional?

(InternalModuleReadFile() is only used for reading package.json files and uv_fs_read() can realistically only fail with EIO or EISDIR. The tests in this PR don't exercise that scenario, therefore QED.)

The changes also don't fix the freebsd/non-freebsd divergence but that is ameliorated by the fact that the directory entry is unlikely to be valid JSON. :-)

I've filed #8307 because I think this is a separate issue.

[rvagg]

There's no accidental fix here and you're still missing the point I think. The two things being fixed here are the null bytes check not being in place and leading to an abort, any \u0000 in require can crash a program. It's a stretch but I could concoct a scenario where this couod be a DoS vector if require depends at all on user input (bad practice of course but still). The second is the fact that all the usual cases of fs read failure lead to an abort in the new require path. The is-directory failure I mentioned is simply one that can come up by using the test cases we have (edit: the test cases introduced here if you remove the null bytes check, i.e. null bytes can result in trying to read a directory in some cases on some platforms but the null bytes check doesn't even let it it get that far), not something I have actually experienced.

I don't understand what the hesitation is here, it seems that there is a suggestion that I somehow don't understand what I'm fixing (an assessment I don't agree with) and therefore it's not good enough. I don't get what couod be controversial about fixing this.

In lieu of making forward progress here I'd prefer to just revert the commit(s) that introduced this, I'll put those in when I have time so that option can be considered too. Having an abort() in the path of reasonable failure cases should not be acceptable. They should be reserved for the unexpected and surprising (to us).

[jasnell]

Rather than returning undefined.... can we throw here? Yes, I know that's semver-major but it's better than an abort and it's likely more correct than undefined.

[rvagg]

That would short circuit the require lookup algorithm which may potentially find a working match in a further iteration. We can do it but it's not a small breaking change and IMO would be unnecessarily disruptive for little gain.

[addaleax]

See also the comment above the function:

// Used to speed up module loading.  Returns the contents of the file as
// a string or undefined when the file cannot be opened.  The speedup
// comes from not creating Error objects on failure.

[jasnell]

+1 works for me.

On Sunday, September 4, 2016, Anna Henningsen notifications@github.com
wrote:

In src/node_file.cc
#8277 (comment):

@@ -566,17 +569,21 @@ static void InternalModuleReadFile(const FunctionCallbackInfo& args) {
CHECK_EQ(0, uv_fs_close(loop, &close_req, fd, nullptr));
uv_fs_req_cleanup(&close_req);

• size_t start = 0;
• if (chars.size() >= 3 && 0 == memcmp(&chars[0], "\xEF\xBB\xBF", 3)) {
• start = 3; // Skip UTF-8 BOM.
• }
• if (numchars < 0) {
• args.GetReturnValue().Set(Undefined(env->isolate()));

See also the comment above the function:

// Used to speed up module loading. Returns the contents of the file as// a string or undefined when the file cannot be opened. The speedup// comes from not creating Error objects on failure.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/pull/8277/files/39db98ca5e20b1c28b5058c7b049747324d955bd#r77449453,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAa2eWcj_tAWKN-vBdhw8Czm5djPYWULks5qmpA1gaJpZM4Jt1xz
.

[jasnell]

ping @rvagg @bnoordhuis ...

[bnoordhuis]

Needs a rebase first, 3 out of 4 touched files conflict. However, I'm not convinced this is a) the right fix, and b) necessary.

The threat scenario seems to be require(untrustedUnsanitizedInput) which strikes me as highly implausible. Who in their right mind does that?

[fhinkel]

ping @rvagg Are you still working on this or have we found a different solution?

[jasnell]

Closing due to lack of forward progress. We can reopen if necessary.

[rvagg]

Still needs resolution, just need to find a path forward that satisfies the concerns here, tricky to navigate it seems but really needs attention. If someone else wants to pick this up in the meantime then please do.

[jasnell]

I believe @bnoordhuis had been dabbling with this lately. I'll verify

[cjihrig]

#14854?

[saper]

@cjihrig #14854 is about require('\u0000ab') and this one is about file that contains NUL bytes in the contents.

[cjihrig]

You said something similar in #14905 (comment), but it doesn't seem to be clear to anyone else. Quoting @bnoordhuis: Why are nul bytes in a file relevant?

[saper]

@cjihrig one thing is to make sure to understand those are two different issues. A quick look at the proposed test should make it clear (it unpack JSON file containing nulls). Crashing node just because the input file has some funny characters is not good. But I misread the test, it seems to me it does not test this fix.

[BridgeAR]

@saper I am not sure what you mean with "unpack JSON file containing null". There is no JSON file in the tests and as I pointed out before (#14905 (comment)) the tests work fine and Node.js is not crashing anymore since #14854 landed.

[saper]

This is really confusing, you are right.

I think that 39db98c is obsoleted by #14854

But de95b66 needs a test and possibly a nicer solution.

[BridgeAR]

@saper as far as I see it this is not needed at all anymore. If you can come up with a test case that fails I would ask you to post that. Otherwise there is little to do.