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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from Jun 23, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
99b23e6
Fix arbitrary command injection, CWE-264
Logikgate May 14, 2018
d55bb29
remove unused macos implementation, unix covers it
Logikgate May 14, 2018
b2124ef
Fixes arbitrary command execution
Logikgate May 14, 2018
7b0a488
Fix for Node 0.8 thru 0.10
Logikgate May 17, 2018
214ad00
Run travis against osx and linux, travis doesn't do windows
Logikgate May 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .travis.yml
View file
Open in desktop
@@ -1,4 +1,7 @@
language: node_js
os:
- linux
- osx
node_js:
- stable
- "0.12"
Expand Down
6 changes: 3 additions & 3 deletions lib/linux.js
View file
Open in desktop
@@ -1,11 +1,11 @@
var exec = require('child_process').exec;
var execFile = require('child_process').execFile;

module.exports = function (iface, callback) {
exec("cat /sys/class/net/" + iface + "/address", function (err, out) {
execFile("cat", ["/sys/class/net/" + iface + "/address"], function (err, out) {
if (err) {
callback(err, null);
return;
}
callback(null, out.trim().toLowerCase());
});
};
};
16 changes: 0 additions & 16 deletions lib/macosx.js
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions lib/unix.js
View file
Open in desktop
@@ -1,7 +1,7 @@
var exec = require('child_process').exec;
var execFile = require('child_process').execFile;

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

@javabudd javabudd Sep 24, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ifconfig eth0 down

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

if (err) {
callback(err, null);
return;
Expand Down
4 changes: 2 additions & 2 deletions lib/windows.js
View file
Open in desktop
@@ -1,4 +1,4 @@
var exec = require('child_process').exec;
var execFile = require('child_process').execFile;

var regexRegex = /[-\/\\^$*+?.()|[\]{}]/g;

Expand All @@ -7,7 +7,7 @@ function escape(string) {
}

module.exports = function (iface, callback) {
exec("ipconfig /all", function (err, out) {
execFile("ipconfig", ["/all"], function (err, out) {
if (err) {
callback(err, null);
return;
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
@@ -1,6 +1,6 @@
{
"name": "macaddress",
"version": "0.2.9",
"version": "0.2.10",
"description": "Get the MAC addresses (hardware addresses) of the hosts network interfaces.",
"main": "index.js",
"scripts": {
Expand Down