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

Fixes arbitrary command injection by using execFile instead of exec #20

Merged
merged 5 commits into from Jun 23, 2018

Conversation

@Logikgate
Copy link
Contributor

Logikgate commented May 30, 2018

Note: this is the same as PR #18, replaced to allow further development on our fork's master branch

execFile takes the call arguments as variables and they are not executed under the shell environment

Reference
https://blog.liftsecurity.io/2014/08/19/Avoid-Command-Injection-Node.js/

@Logikgate

This comment has been minimized.

Copy link
Contributor Author

Logikgate commented May 30, 2018

Since it appears this repo is abandoned, I have published a new npm module based on our fork. This fork will be maintained for security fixes and will be open to feature pull requests from the community.

You can use it with:
npm install macaddress-secure
or
yarn add macaddress-secure

Also, if you're using yarn you can force your sub-modules to use this package by adding the below to your package.json
"resolutions": { "*/**/macaddress": "macaddress-secure^0.2.11" }

@mikroskeem

This comment has been minimized.

Copy link

mikroskeem commented on lib/linux.js in 99b23e6 Jun 11, 2018

There's a fs#readFile for reading files, you don't need to use cat and exec* functions for that.

Besides that, I'd use path#basename on iface as well to avoid path traversal (even though I doubt that anyone would really use file named address in production, but you'll never know 😉)

@q3k

This comment has been minimized.

Copy link

q3k commented Jun 11, 2018

This is still vulnerable to arbitrary file read:

> let execFile = require('child_process').execFile;
undefined
> let garbage = function (iface, callback) {
...     execFile("cat", ["/sys/class/net/" + iface + "/address"], function (err, out) {
.....         if (err) {
.......             callback(err, null);
.......             return;
.......         }
.....         callback(null, out.trim().toLowerCase());
.....     });
... };
undefined
> garbage("../../../../etc/passwd\x00", (e, o) => { console.log(o); })
undefined
> root:x:0:0:root:/root:/bin/bash
daemon:x:1:1:daemon:/usr/sbin:/usr/sbin/nologin
bin:x:2:2:bin:/bin:/usr/sbin/nologin
sys:x:3:3:sys:/dev:/usr/sbin/nologin
sync:x:4:65534:sync:/bin:/bin/sync
games:x:5:60:games:/usr/games:/usr/sbin/nologin
man:x:6:12:man:/var/cache/man:/usr/sbin/nologin
lp:x:7:7:lp:/var/spool/lpd:/usr/sbin/nologin
mail:x:8:8:mail:/var/mail:/usr/sbin/nologin
news:x:9:9:news:/var/spool/news:/usr/sbin/nologin
uucp:x:10:10:uucp:/var/spool/uucp:/usr/sbin/nologin
proxy:x:13:13:proxy:/bin:/usr/sbin/nologin
www-data:x:33:33:www-data:/var/www:/usr/sbin/nologin
backup:x:34:34:backup:/var/backups:/usr/sbin/nologin
list:x:38:38:mailing list manager:/var/list:/usr/sbin/nologin
irc:x:39:39:ircd:/var/run/ircd:/usr/sbin/nologin
gnats:x:41:41:gnats bug-reporting system (admin):/var/lib/gnats:/usr/sbin/nologin
nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin
systemd-timesync:x:100:103:systemd time synchronization,,,:/run/systemd:/bin/false
systemd-network:x:101:104:systemd network management,,,:/run/systemd/netif:/bin/false
systemd-resolve:x:102:105:systemd resolver,,,:/run/systemd/resolve:/bin/false
systemd-bus-proxy:x:103:106:systemd bus proxy,,,:/run/systemd:/bin/false
node:x:1000:1000::/home/node:/bin/bash

> 

I recommend at least using a proper file read function which will panic on null bytes and to manually filter out any pathseps from the given iface:

> require('fs').readFile('/sys/class/net/../../../../../etc/passwd\x00/address', (e, o) => { console.log(o); })
TypeError [ERR_INVALID_ARG_VALUE]: The argument 'path' must be a string or Uint8Array without null bytes. Received '/sys/class/net/../../../../../etc/passwd\u0000/address'
    at Object.fs.readFile (fs.js:291:3)

or even better, using real APIs to access network device information (ie. Netlink on Linux).

@q3k

This comment has been minimized.

Copy link

q3k commented Jun 14, 2018

@Logikgate ping, check out the above cf. your fork ^

(I have to mention you here, because your '-secure' repo has GH Issues disabled ?!)

@epet

This comment has been minimized.

Copy link

epet commented Jun 22, 2018

@Logikgate, would you enable issues/discussions on your repo, please.

@Logikgate

This comment has been minimized.

Copy link
Contributor Author

Logikgate commented Jun 22, 2018

@epet, @q3k I've enabled issues, sorry about that. @q3k I'll take a look at removing cat completely soon here

@epet

This comment has been minimized.

Copy link

epet commented Jun 22, 2018

@q3k, @Logikgate, copied issue @q3k reported to flypapertech#1

@scravy scravy merged commit 358fd59 into scravy:master Jun 23, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@scravy

This comment has been minimized.

Copy link
Owner

scravy commented Jun 23, 2018

Hi everyone,

thank you for the discussion and the PR which I merged way to late. Friends brought it to my attention that this issue has even been brought up at https://news.ycombinator.com/item?id=17283394

Sorry for the seemingly abandonness of this project, I took care of a few pull requests and am releasing a new version right away.

@epet

This comment has been minimized.

Copy link

epet commented Jun 25, 2018

thanks @Logikgate for fixing and @scravy for resurfacing!

@kwbskf

This comment has been minimized.

Copy link

kwbskf commented Jul 3, 2018

Sorry to interject since I don't really belong here (just a package user) but... I have a usage of uuid-1345 that in turn uses macaddress, and it gives an error where macaddress code has a "require('child_process')". I believe this was the fix, right? I mean, to use execFile, which needs child_process. Shouldn't this package somehow indicate that it has a dependency on this 'child_process' package? Ideally you shouldn't have to manually figure this out and install it, but for me at least, the story doesn't end there: manually doing "npm install --save child_process" is not sufficient to get the reference to resolve.

@scravy

This comment has been minimized.

Copy link
Owner

scravy commented Jul 3, 2018

@kwbskf Could you open an issue for that on the uuid package? I will have a look in the weekend.

@kwbskf

This comment has been minimized.

Copy link

kwbskf commented Jul 5, 2018

done; but it seems more like an issue with macaddress and how it references child_process, though it might have more to do with something amiss in my configuration


module.exports = function (iface, callback) {
exec("ifconfig " + iface, function (err, out) {
execFile("ifconfig", [iface], function (err, out) {

This comment has been minimized.

Copy link
@javabudd

javabudd Sep 24, 2018

If iface isn't properly sanitized, wouldn't this still be vulnerable?

ifconfig eth0 down

This comment has been minimized.

Copy link
@scravy

scravy Oct 1, 2018

Owner

@javabudd No. execFile does not invoke a command interpreter / shell (unless an options dict with shell is passed, but the shell option defaults to false). The command is not simply concatenated with spaces and then interpreted again. Your example would yield ifconfig 'eth0 down' which is not two arguments eth0 and down but one argument etc0 down which ifconfig will not find an interface for.

The previous vulnerability was that exec indeed does interpret the command and you could have done things like ifconfig eth0; rm -rf /. This is properly solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.