4 ms·

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

• 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?
• koolba 8y ago
  Escaping and quoting is the wrong way to go. Even if it works you're technically not totally correct as you can't reference my network interface named "bad'name"[1]. If you can avoid fork/exec you're always better off. In this case reading the file directly is fine. I can't think of a valid reason to fork and invoke cat. If you have a legit reason to fork and run then use execFile(...) as you can pass the arguments as an actual array. No escaping needed as it gets passed in total without shell parsing of args. Your example would be something like: execFile('/bin/cat', ['--', `/sys/class/net/${iface}/address`,], (error, stdout, stderr) => { ... }) Though that has the issue of being able to pass in ../../../ and read a file named "address" anywhere on the filesystem. Should also add in a regex validation that no directory separators are in the text of iface. [1]: Yes this sounds stupid but it's a valid file name. A more realistic one may be something like "Alice's Wi-Fi". [2]: https://nodejs.org/api/child_process.html#child_process_child_process_execfile_file_args_options_callback https://nodejs.org/api/child_process.html#child_process_chil...
  • tomxor 8y ago
    > Escaping and quoting is the wrong way to go. Even if it works you're technically not totally correct as you can't reference my network interface named "bad'name"[1]. If you can avoid fork/exec you're always better off. In this case reading the file directly is fine. I can't think of a valid reason to fork and invoke cat. Yes I get that, I should have clarified: in the general use of exec (i.e a valid and necessary use). > If you have a legit reason to fork and run then use execFile(...) You are absolutely right, execFile is a much better way, I've used it before and I guess you are right - in _most_ cases this should be the #1 choice. [edited] - irrelevant details of obscure case where execFile isn't enough. Anyway, in a case where exec through a shell is necessary for some reason, i'm wondering if this method of single quoting is actually safe or not, as far as I can see it is, but i'm not a security expert.
• xorcist 8y ago
  Quoted exactly right, it will not be exploitable. However, this amounts to declaring which input is disallowed, generally not a good idea. It is preferrable to declare which input is allowed. In this case, matters are worse because without knowing which shell we are running under and which cat we are executing it is impossible to know the complete list of dangerous characters. (In fact, if some sloppy sysadmin have a dot in the search path, the cat binary might even be attacker uploaded.) (The fact that cat is even in there is of course doubly insane. They start an extra process just to make reading the data slightly more complicated.)
  • tomxor 8y ago
    Thanks > matters are worse because without knowing which shell we are running under The shell in exec can be configured, it's /bin/sh by default but of course this is linked to all kinds of different things on different OS, ubuntu links to dash, used to be bash, FreeBSD has a "proper" sh. I suppose if going through a shell was somehow necessary you could try to be more secure by specifying a absolute path to a shell but... > In fact, if some sloppy sysadmin have a dot in the search path, the cat binary might even be attacker uploaded. :P I feel like it's game over at that point, is there really any point in worrying about library security when the $PATH has been effectively hijacked?
    • xorcist 8y ago
      > is there really any point in worrying about library security when the $PATH has been effectively hijacked? Well, there might be legitimate (although somewhat misguided) reasons for someone to do that. People routinely do similar things in dev environments all the time. I can absolutely see how it could reach production. If you don't expect your web app to launch shell scripts you might not even think about it. It's the small things, which in themselves might not be serious or even noteworthy, which compound together to deliver the nasty surprises.
• yes_or_gnome 8y ago
  I'm not a JS developer, so this as much of a question as it is a statement. `exec` is a system call and doesn't call bash or any other shell. At least, it shouldn't. If it does, then it would be wrong to assume that bash is even available. Either way. The single quote sytax is specific to bash (and maybe sh, dash, etc. I'm not certain.) so, don't rely upon it.
  • tomxor 8y ago
    You are describing execFile in nodejs which directly spawns a binary into a child process and explicitly passes each argument from an array (Other's here have mentioned it's the prefered way as it obviously avoids any arbitrary execution potential of going through a shell)... exec is not a system call in nodejs, it's just a convenience function that sets up a child process and spawns a shell (of your choosing) and then passes the first argument as the single command.