New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

This package contains a serious security hole. #1

Closed
skx opened this Issue Nov 10, 2014 · 5 comments

Comments

Projects
None yet
3 participants
@skx

skx commented Nov 10, 2014

Consider the following code:

var dnsSync = require('dns-sync');
console.log(dnsSync.resolve('$(id > /tmp/foo)'));

The library is loaded. The function resolve is called, which contains this code:

  // ..
  cmd = util.format('"%s" "%s" %s', nodeBinary, scriptPath, hostname);
  // ...
  response = shell.exec(cmd, {silent: true});

So the end result is a call to a command like:

       "/opt/node/bin/node" "/path/to/dns-lookup-script" "$(id > /tmp/foo)'"

The shell expands that, by executing "/usr/bin/id > /tmp/foo" - et voila arbitrary command execution, triggered by a DNS lookup.

@skx skx changed the title from This package contains a security hole. to This package contains a serious security hole. Nov 10, 2014

@skoranga

This comment has been minimized.

Show comment
Hide comment
@skoranga

skoranga Nov 10, 2014

Owner

@skx
Added the validation check and tests with commit - d9abaae.

Also published dns-sync@0.1.1.

Please verify if you still see some issue.

Owner

skoranga commented Nov 10, 2014

@skx
Added the validation check and tests with commit - d9abaae.

Also published dns-sync@0.1.1.

Please verify if you still see some issue.

@skx

This comment has been minimized.

Show comment
Hide comment
@skx

skx Nov 11, 2014

I suspect the regular expression is more complex than it needs to be, but otherwise I don't see anything to complain about.

Thanks for your prompt attention.

skx commented Nov 11, 2014

I suspect the regular expression is more complex than it needs to be, but otherwise I don't see anything to complain about.

Thanks for your prompt attention.

@skx skx closed this Nov 11, 2014

@fgeek

This comment has been minimized.

Show comment
Hide comment
@fgeek

fgeek Nov 11, 2014

Good job @skx :)

fgeek commented Nov 11, 2014

Good job @skx :)

@skx

This comment has been minimized.

Show comment
Hide comment
@skx

skx Nov 12, 2014

@fgeek Thanks.

I'm still trying to solve a general problem with async DNS replies - blogspam.js #25 - and thought this library might be useful. Unfortunately the overhead of running a command for each lookup is too high so I had to rule it out.

skx commented Nov 12, 2014

@fgeek Thanks.

I'm still trying to solve a general problem with async DNS replies - blogspam.js #25 - and thought this library might be useful. Unfortunately the overhead of running a command for each lookup is too high so I had to rule it out.

@skoranga

This comment has been minimized.

Show comment
Hide comment
@skoranga

skoranga Nov 12, 2014

Owner

@skx, agreed this is bit slow since it's spawning a process to make DNS sync. Right now my use case is to resolve the DNS only on server startup, so it fits well. Let me know if you find a better solution. I am welcome to ideas.

Owner

skoranga commented Nov 12, 2014

@skx, agreed this is bit slow since it's spawning a process to make DNS sync. Right now my use case is to resolve the DNS only on server startup, so it fits well. Let me know if you find a better solution. I am welcome to ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment