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

deps: c-ares, avoid single-byte buffer overwrite #9108

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 15, 2016

This is a manual backport of #8849 because we're jumping back from 1.10 to 1.9 here. It's pretty awkward because there's no max_udp_size argument in this old form. Tests pass locally but I don't have the highest degree of confidence that I've got this 100% so I'd appreciate some more expert eyes please.

Maybe one of @indutny, @bnoordhuis, @addaleax might be able to grok this better than I?

Backport of nodejs#8849 for c-ares
1.9.0.

Incorrect string length calculation when passing escaped dot.

- CVE: CVE-2016-5180
- Upstream bug: https://c-ares.haxx.se/adv_20160929.html
@nodejs-github-bot nodejs-github-bot added cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. v0.10 labels Oct 15, 2016
@@ -191,5 +168,22 @@ int ares_mkquery(const char *name, int dnsclass, int type, unsigned short id,
DNS_QUESTION_SET_TYPE(q, type);
DNS_QUESTION_SET_CLASS(q, dnsclass);

q += QFIXEDSZ;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the latest version, this line was previously inside a if (max_udp_size) {} block but was lifted out for the patch. In this backport it's standing alone and I'm not super-confident that it belongs here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so previously q was just used to position for writes, but in the new version it's also used to check the final length and reject if it's too long, so we need this in here for that purpose whereas it was not needed before because there are no more writes after this.

@rvagg
Copy link
Member Author

rvagg commented Oct 15, 2016

CI @ https://ci.nodejs.org/job/node-test-pull-request/4527/, usual failures on Windows, others are good.

@jasnell
Copy link
Member

jasnell commented Oct 17, 2016

Are we going to be doing another v0.10 release before the 31st?
/cc @nodejs/lts

@rvagg
Copy link
Member Author

rvagg commented Oct 18, 2016

@jasnell yes, this fix has been announced already http://nodejs.org/en/blog/vulnerability/october-2016-security-releases/

@nodejs/collaborators I'd appreciate a review here asap, this needs to get out in less than 24 hours.

@rvagg
Copy link
Member Author

rvagg commented Oct 18, 2016

Pretty sure I grok most of what's going on in here now and am comfortable this is correct. I'm going to have to merge this unreviewed if nobody steps up in the next few hours, unfortunately! Gotta get a v0.10 out.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks correct to me. max_udp_size is related to EDNS but this version of c-ares doesn't support that.

rvagg added a commit to rvagg/io.js that referenced this pull request Oct 18, 2016
Backport of nodejs#8849 for c-ares
1.9.0.

Incorrect string length calculation when passing escaped dot.

- CVE: CVE-2016-5180
- Upstream bug: https://c-ares.haxx.se/adv_20160929.html

PR-URL: nodejs#9108
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@rvagg
Copy link
Member Author

rvagg commented Oct 18, 2016

a14a6a3

@rvagg rvagg closed this Oct 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cares Issues and PRs related to the c-ares dependency or the cares_wrap binding.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants