Skip to content
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

dns.setServers is memory unsafe and can be used to dump core #8538

Closed
deian opened this issue Sep 14, 2016 · 3 comments
Closed

dns.setServers is memory unsafe and can be used to dump core #8538

deian opened this issue Sep 14, 2016 · 3 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dns Issues and PRs related to the dns subsystem.

Comments

@deian
Copy link
Member

deian commented Sep 14, 2016

  • Version: 6.5.0
  • Platform:
  • Subsystem:

dns.setServers is memory unsafe and can be used to dump core.

This doesn't seem like a serious security vulnerability (hence my reporting here), but can certainly be used to cause DOS and it might be nice to have a stdlib that is memory safe.

const dns = require('dns');
const servers = [];
servers[0] = '127.0.0.1';
servers[2] = '0.0.0.0';
dns.setServers(servers);
// causes the CHECK src/cares_wrap.cc:1241 to dump core

another variant:

const dns = require('dns');
const servers = ['127.0.0.1','192.168.1.1'];
servers[3] = '127.1.0.1';
servers[4] = '127.1.0.1';
servers[5] = '127.1.1.1';
Object.defineProperty(servers, 2, {
  enumerable: true,
  get: () => {
    servers.length = 3;
    return '0.0.0.0';
  }
});
dns.setServers(servers);
// causes the CHECK src/cares_wrap.cc:1241 to dump core

Related to: #8539, #8537, #7902

@deian deian changed the title dns.setServers is memory unsafe and can be used to dump core dns.setServers is memory unsafe and can be used to dump core Sep 14, 2016
@mscdex mscdex added the dns Issues and PRs related to the dns subsystem. label Sep 14, 2016
@imyller
Copy link
Member

imyller commented Sep 15, 2016

This is a known issue and affects most C++ API functions accepting non-primitive values.

For more information:

#7902 (comment)

@imyller imyller added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 15, 2016
@deian
Copy link
Member Author

deian commented Sep 17, 2016

Like #8537, I also don' think this will be resolved by using the Maybe API. This is deliberately abusing the CHECK to dump core.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 17, 2016

See #8567 for a fix for this one.

cjihrig added a commit to cjihrig/node that referenced this issue Sep 20, 2016
This commit adds better handling of exceptional array formats
passed to dns.setServers(). Prior to this commit, the input
array was validated using map(), which preserves holes, allowing
them to be passed to c-ares, crashing Node. This commit replaces
map() with forEach(), which skips holes.

Fixes: nodejs#8538
PR-URL: nodejs#8567
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this issue Oct 11, 2016
This commit adds better handling of exceptional array formats
passed to dns.setServers(). Prior to this commit, the input
array was validated using map(), which preserves holes, allowing
them to be passed to c-ares, crashing Node. This commit replaces
map() with forEach(), which skips holes.

Fixes: #8538
PR-URL: #8567
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants