[deleted]
Critical vulnerability of NPM package macaddress
31–40 of 99 posts
Re: Critical vulnerability of NPM package macaddress
#32Another month, another ridiculous issue involving NPM that has no place being in software with millions of weekly downloads.
It really is never ending amateur hour in the NPM community.
Re: Critical vulnerability of NPM package macaddress
#33Here'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…
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...
Re: Critical vulnerability of NPM package macaddress
#34Another month, another ridiculous issue involving NPM that has no place being in software with millions of weekly downloads. It really is never ending amateur hour in the NPM community.
This has nothing to do with NPM. It is a specific package published on NPM. Also, stereotyping and insulting the NPM community is nonconstructive and out of place. Vulnerabilities in packages happen all the time in all kinds of package managers. The lesson here is to not blind-trust any package you come across that might make your job easier.
Re: Critical vulnerability of NPM package macaddress
#35Many people here are saying that the problem had been in the code base for years, was super obvious, and that just no one looked at the code. I think that it is much more relevant that the node community does do many micro packages, that it makes so much nose to look at the quality of pulled in packages. I recently pulled down a web app (Zulip) the packages.json expands to almost 1400 npm packages. How is any human g…
I am not a fan of the whole javascript craze but for comparison on my desktop machine with I assume average setup of ElementaryOS:
$ apt list --installed | wc -l
2394
Re: Critical vulnerability of NPM package macaddress
#36Here'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…
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'`, () => {})
iface = `'; touch /uhoh;`
cat '/sys/class/net/; touch /uhoh;/address'
=> cat: '/sys/class/net/; touch /uhoh/address': No such file or directory
cat '/sys/class/net/` touch /uhoh `/address'
=> cat: '/sys/class/net/`touch /uhoh`/address': No such file or directoryRe: Critical vulnerability of NPM package macaddress
#37Earlier quoted context omitted.
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.
Re: Critical vulnerability of NPM package macaddress
#38Earlier quoted context omitted.
I maintain a package that has an indirect dep to macaddress because of UUID generation. UUID v2 is generated from Mac address + timestamp. My package only generates uuid v4 (fully random) and even if it was, the uuid dep doesn't expose the vulnerable argument. As such it isn't vulnerable. But the dep is still there. This isn't surprising at all tbh.
A dependency for uuid4 is hardly needed: crypto = require('crypto'); function uuid4() { const bytes = crypto.randomBytes(16); bytes[6] = (bytes[6] & 0x0f) | 0x40; bytes[8] = (bytes[8] & 0x3f) | 0x80; return bytes.toString('hex').match(/(.{8})(.{4})(.{4})(.{4})(.{12})/).slice(1).join('-'); }
v1 and v2 UUIDs are derived from a MAC address by specification, the uniqueness of the v1 scheme in particular depends on it. Whether that's a good idea or not is another topic entirely, but if you want to conform to spec, an implementation needs to be able to read MAC addresses
Re: Critical vulnerability of NPM package macaddress
#39Here'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…
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...
Re: Critical vulnerability of NPM package macaddress
#40Another month, another ridiculous issue involving NPM that has no place being in software with millions of weekly downloads. It really is never ending amateur hour in the NPM community.
This has nothing to do with NPM. It is a specific package published on NPM. Also, stereotyping and insulting the NPM community is nonconstructive and out of place. Vulnerabilities in packages happen all the time in all kinds of package managers. The lesson here is to not blind-trust any package you come across that might make your job easier.
[deleted]