Critical vulnerability of NPM package macaddress

#1

Post by tekkk » Mon, Jun 11, 2018, 11:29 AM UTC

Critical vulnerability of NPM package macaddress
nodesecurity.io

Re: Critical vulnerability of NPM package macaddress

#2

Post by koolba » Mon, Jun 11, 2018, 1:10 PM UTC

Here's the offending line of code[1]:

     exec("cat /sys/class/net/" + iface + "/address", function (err, out) {

That's some serious amateur hour and may even be a new contender for the Useless Use of Cat (and "exec(...)") Award.

Would be simpler and less error prone to just read the file via fs.readFile(...) after verifying that the iface parameter does not contain a directory separator.

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

Re: Critical vulnerability of NPM package macaddress

#3

Post by [deleted] » Mon, Jun 11, 2018, 1:13 PM UTC

[deleted]

Re: Critical vulnerability of NPM package macaddress

#4

Post by robin_reala » Mon, Jun 11, 2018, 1:14 PM UTC

This has been biting a lot of people as the mostly unmaintained cssnano uses it, which in turn is used by create-react-app. Previously this would have probably gone unnoticed, but npm audit in npm6 has brought it front and centre.

Re: Critical vulnerability of NPM package macaddress

#5

Post by speps » Mon, Jun 11, 2018, 1:14 PM UTC

The actual report, as that page contains nothing interesting: https://hackerone.com/reports/319467

Re: Critical vulnerability of NPM package macaddress

#6

Post by MrLeap » Mon, Jun 11, 2018, 1:15 PM UTC

koolba wrote: post #2

Here's the offending line of code[1]: exec("cat /sys/class/net/" + iface + "/address", function (err, out) { That's some serious amateur hour and may even be a new contender for the Useless Use of Cat (and "exec(...)") Award. Would be simpler and less error prone to just read the file via fs.readFile(...) after verifying that the iface parameter does not contain a directory separator. [1]: https://github.com/scravy/n…

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?

Re: Critical vulnerability of NPM package macaddress

#7

Post by arghwhat » Mon, Jun 11, 2018, 1:17 PM UTC

koolba wrote: post #2

Here's the offending line of code[1]: exec("cat /sys/class/net/" + iface + "/address", function (err, out) { That's some serious amateur hour and may even be a new contender for the Useless Use of Cat (and "exec(...)") Award. Would be simpler and less error prone to just read the file via fs.readFile(...) after verifying that the iface parameter does not contain a directory separator. [1]: https://github.com/scravy/n…

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

Re: Critical vulnerability of NPM package macaddress

#8

Post by andyana » Mon, Jun 11, 2018, 1:20 PM UTC

robin_reala wrote: post #4

This has been biting a lot of people as the mostly unmaintained cssnano uses it, which in turn is used by create-react-app. Previously this would have probably gone unnoticed, but npm audit in npm6 has brought it front and centre.

It's amazing that something like cssnano would have a dependency on getting MAC addresses. WTF?

Re: Critical vulnerability of NPM package macaddress

#9

Post by arghwhat » Mon, Jun 11, 2018, 1:22 PM UTC

MrLeap wrote: post #6

koolba wrote: post #2

Here's the offending line of code[1]: exec("cat /sys/class/net/" + iface + "/address", function (err, out) { That's some serious amateur hour and may even be a new contender for the Useless Use of Cat (and "exec(...)") Award. Would be simpler and less error prone to just read the file via fs.readFile(...) after verifying that the iface parameter does not contain a directory separator. [1]: https://github.com/scravy/n…

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 l…

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.

Re: Critical vulnerability of NPM package macaddress

#10

Post by drdaeman » Mon, Jun 11, 2018, 1:23 PM UTC

arghwhat wrote: post #7

koolba wrote: post #2

Here's the offending line of code[1]: exec("cat /sys/class/net/" + iface + "/address", function (err, out) { That's some serious amateur hour and may even be a new contender for the Useless Use of Cat (and "exec(...)") Award. Would be simpler and less error prone to just read the file via fs.readFile(...) after verifying that the iface parameter does not contain a directory separator. [1]: https://github.com/scravy/n…

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.