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

ares_create_query: avoid single-byte buffer overwrite #9037

Closed
wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

deps

Description of change

Incorrect string length calculation when passing escaped dot.

This port involved changing ares_malloc to malloc as the symbol doesn't exist on the version we have in v4.x

@MylesBorins MylesBorins added v4.x cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. labels Oct 11, 2016
@MylesBorins
Copy link
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/4471/

I am opting to keep the original sub-system name, even if it is not correct, for the sake of simplicity.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Rubber-stamp LGTM as the diffs are identical except for malloc 👍

@jbergstroem
Copy link
Member

LGTM, patch looks identical to upstream. I'm -1 to keeping subsystem but don't hold any landings up as a result of it.

Incorrect string length calculation when passing escaped dot.

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

Ref: nodejs#9037
PR-URL: nodejs#8849
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM

@MylesBorins
Copy link
Contributor Author

updated sub system

@jasnell
Copy link
Member

jasnell commented Oct 11, 2016

LGTM

@rvagg
Copy link
Member

rvagg commented Oct 14, 2016

thoughts on timeline for releasing this @thealphanerd?

MylesBorins pushed a commit that referenced this pull request Oct 14, 2016
Incorrect string length calculation when passing escaped dot.

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

Ref: #9037
PR-URL: #8849
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@MylesBorins
Copy link
Contributor Author

landed in v4.x-staging in 1f900b6

rvagg pushed a commit that referenced this pull request Oct 15, 2016
Incorrect string length calculation when passing escaped dot.

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

Ref: #9037
PR-URL: #8849
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 18, 2016
Incorrect string length calculation when passing escaped dot.

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

Ref: nodejs#9037
PR-URL: nodejs#8849
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@MylesBorins MylesBorins deleted the cares-v4 branch November 14, 2017 17:43
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.

8 participants