SECURITY: undocumented options.name parameter allows arbitrary paths to be injected into tmp

[adamcohenrose]

Operating System

• all

NodeJS Version

• all

Tmp Version

all existing and current code base

Expected Behavior

Prevent creating arbitrary files on the filesystem. Documented options do not include the name option.

Experienced Behavior

Specifying the undocumented name option as well as the documented dir option allow creation (and detection) of a file anywhere on the filesystem.

Security Concern

This can be a major security concern, depending on how applications make use of tmp.

[silkentrance]

Nice find, the name option is there but has not been documented.

As for the security concern. Well, basically tmp is meant for use with servers in a controlled environment, especially since it allows unsecure deletion of whole directory trees.

With the advent of containerized applications, this issue does not necessarily pose that much of a problem, except for the maleficient package that you are using in your controlled environment fiddling around with your system and potentially destroying your container instance by having tmp delete the files or even root, recursively.

Of course, for applications that still run as root on a standard linux or windows machine with no change-root in place, or even with a less permissive account, this still poses a potential threat. But then again, one should know about the dependencies that one integrates into a given application. And also one should know to how to and be able to harden the filesystem and process environment in which a given application is being run.

As for maleficient packages, in the past years I never came across such a package. So one should consider the overall process including npm rather safe and that the community that is providing these packages on npm to be benevolent rather than maleficient. Unless, of course, you trust arbitrary user input in your web service or application.

And having maleficient packages, one can always assume that they will implement this kind of behaviour by themselves and not rely upon some other package to do the trick for them. Especially when it comes to spying out the filesystem and reporting information back to some maleficient site.

In order to overcome this, we would have to redesign tmp to always use the system provided tmp directory and not let the user redefine the absolute or relative path that may point to arbitrary locations.

So my proposal would be to make the passed in dir option to always be relative to the system provided tmp directory, and, given the possibility to define TEMP or TMP or whatever environment variable there is in the process environment, one can redefine the tmp dir location on a per application basis, but will have to do this prior to the start of the application.

Once this is in place, passing in a name that already exists should cause tmp to fail, so that the existing file will not be overwritten.

And I am afraid that this is all that we can do unless you have additional information that you can provide which will help us making this more "secure".

[adamcohenrose]

Thanks for getting back. Yes, I agree – there's only a limited scope to make the library more secure given that it is designed to be able to delete whole directory trees!

I think your proposal sounds great – the library would be much less surprising this way, and I see you've already had someone being confused in #204!

My only concern would be the one you've already covered in #207 about informing people who are already using the library for directories that aren't under the system temp. Given that the TEMP or TMP environment variables often have a wider effect than just node, might it be an idea to have a separate library-specific environment variable for the base directory?

[silkentrance]

@adamcohenrose

Proposal 1

function _getTmpDir() {
  return os.tmpdir();
}

would also check for process env ("TMP_DIR")

e.g.

function _getTmpDir() {
  return process.env.TMP_DIR || os.tmpdir();
}

or something along that line (error handling etc.)

[silkentrance]

as for #207 this will break existing solutions, so we need to make a major release here

[silkentrance]

@adamcohenrose We have decided against yet another environment variable in favour of the standard system TMP/TEMP environment variable in combination with the then to be relative dir option. Introducing yet another environment variable for pointing the application to its temporary file system storage seems to be utterly redundant. One should use the dir option for that in case one needs multiple temporary directories or pass in different root locations via the TMP/TEMP environment variable.

[adamcohenrose]

Absolutely fine by me. Thanks for keeping me posted

[silkentrance]

Reconsidering

const os = require('os');

console.log(os.tmpdir());

calling this without any environment variables gives /tmp.

calling this with TMP=/tmp/tmp gives /tmp/tmp.

So we actually don't have to change anything on this behalf.

[silkentrance]

Closing as already resolved. The name option has been documented, see merged commit for #206 (#221).
Also see ongoing efforts in #233 (#234).