New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 649040 link

Starred by 4 users
Status: Archived
Owner:
rickyz@chromium.org
Closed: Sep 2016
Cc:
mnissler@chromium.org
vapier@chromium.org
█ daniel.h...@gmail.com
█ drysdale@google.com
dgreid@chromium.org
mmoroz@chromium.org
█ dan...@haxx.se
keescook@chromium.org
█ gzo...@gmail.com
saintlou@google.com
josa...@google.com
puneetster@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 0
Type: Bug-Security

Blocking:
issue 648971



Sign in to add a comment

Security: ChromeOS 1 byte write overflow in c-ares

Project Member Reported by elawrence@chromium.org, Sep 21 2016 
Breakout bug for a portion of the exploit chain described in  issue 648971 .
------

The vulnerability is a one byte overflow in c-ares, the DNS client library used by shill which runs at root. 

The bug can be seen in https://github.com/c-ares/c-ares/blob/cares-1_7_5/ares_mkquery.c#L101 :

  len = 1;
  for (p = name; *p; p++) {                     // (1)
    if (*p == '\\' && *(p + 1) != 0)
      p++;
    len++;
  }

  if (*name && *(p - 1) != '.')                 // (2)
    len++;

  *buflen = len + HFIXEDSZ + QFIXEDSZ;
  *buf = malloc(*buflen);

  q = *buf;                                     // (3)
  q += HFIXEDSZ;
  while (*name) {
    len = 0;
    for (p = name; *p && *p != '.'; p++) {
      if (*p == '\\' && *(p + 1) != 0)
        p++;
      len++;
    }
    *q++ = (unsigned char)len;                  // (4)
    for (p = name; *p && *p != '.'; p++) {
      if (*p == '\\' && *(p + 1) != 0)
        p++;
      *q++ = *p;
    }
    if (!*p)
      break;
    name = p + 1;
  }
  *q++ = 0;

  // writes QFIXEDSZ bytes at q
  DNS_QUESTION_SET_TYPE(q, type);
  DNS_QUESTION_SET_CLASS(q, dnsclass);

Briefly, at (3) it parses the dot separated parts of a DNS name and writes them into *buf. A one byte length is also written for each part at (4) and the terminating dot is omitted. The last part may or may not end with a dot. The buffer size is calculated in (1) as basically just the string length. For n length bytes there is either n or n - 1 dots depending on whether the last part ends with a dot. The check at (2) is meant to account for the n - 1 dots case, it adds +1 to buffer size for the length of the last part.

Now, dots can be escaped though and "\." is not considered to be a part terminator. If the last part ends with a "\." then it doesn't have a dot terminator so (1) doesn't account for its length byte. (2) thinks that the last part does end with a dot and doesn't add +1 for the length byte either. The buffer remains too short and overflows by one byte.

Comment 1 by mbarbe...@chromium.org, Sep 21 2016

Cc: gzo...@gmail.com

Comment 2 Deleted

Comment 3 by gzo...@gmail.com, Sep 21 2016 

I think that patch should fix it. I can only test it after about 10 hours though.
c-ares.patch
975 bytes Download

Comment 4 by gzo...@gmail.com, Sep 22 2016 

The patch in #3 applies to master. c-ares-1.7.5.patch applies to 1.7.5 that Chrome OS currently uses.

I tested with the c-ares-portage.patch that the fix stops the exploit. But I guess it should be fixed upstream instead.
c-ares-1.7.5.patch
969 bytes Download
c-ares-portage.patch
1.7 KB Download

Comment 5 by mnissler@chromium.org, Sep 22 2016 

Thanks for the patches. Ricky has already uploaded a changes for review yesterday here:

https://chromium-review.googlesource.com/#/c/388105 - moving the ebuild to the repo where we can make changes
https://chromium-review.googlesource.com/#/c/388126 - adding the patch with the fix to the ebuild

(feel free to comment on the code reviews if you like)

We should definitely report the issue and contribute the fix upstream. The vulnerability handling process for c-ares is documented here: https://c-ares.haxx.se/security.html

gzobqq: Have you already emailed c-ares-security@haxx.se? If not, I suggest you do so and we can definitely CC their people on this bug as needed. You'll likely want to include details on the shill exploit that the bug enables for their impact assessment, but I'd prefer if you could omit the details for the persistence exploit for now.

That said, we should hopefully have Chrome OS images with the fix within the next couple of days, and the code review fixing the overflow is public anyways so we can probably deal with whatever disclosure timeline works for them.

Ricky, let's also make sure we don't open this bug (and its parent) until the c-ares folks had a chance to react as needed.
Project Member

Comment 6 by sheriffbot@chromium.org, Sep 22 2016

Labels: M-53
Project Member

Comment 7 by sheriffbot@chromium.org, Sep 22 2016

Status: Assigned (was: Unconfirmed)

Comment 8 by gzo...@gmail.com, Sep 22 2016 

I notified c-ares-security@haxx.se and sent them the writeup, omitting persistence details.

mnissler, I CC-d you to reduce any communication delays.

Comment 9 by mnissler@chromium.org, Sep 22 2016

Cc: vapier@chromium.org
Thanks gzobqq!

Comment 10 by mnissler@chromium.org, Sep 22 2016

Cc: dan...@haxx.se

Comment 11 by mnissler@chromium.org, Sep 23 2016

Cc: daniel.h...@gmail.com
Status: Started (was: Assigned)
Flipping to Started, works is already underway here: https://chromium-review.googlesource.com/#/c/388126/

Comment 12 by mnissler@chromium.org, Sep 23 2016

Cc: drysdale@google.com
Project Member

Comment 14 by bugdroid1@chromium.org, Sep 25 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/ee058b6a072ff8846bf6f45b990a58b7dd456c97

commit ee058b6a072ff8846bf6f45b990a58b7dd456c97
Author: Ricky Zhou <rickyz@chromium.org>
Date: Thu Sep 22 02:12:46 2016

Fix heap overflow in ares_mkquery.

Thanks to the reporter for the patch.

BUG= chromium:649040 
TEST=Trybots

Change-Id: Ic038b69c227d8637c213ec655ace6bd567040db2
Reviewed-on: https://chromium-review.googlesource.com/388126
Commit-Ready: Ricky Zhou <rickyz@chromium.org>
Tested-by: Ricky Zhou <rickyz@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[rename] https://crrev.com/ee058b6a072ff8846bf6f45b990a58b7dd456c97/net-dns/c-ares/c-ares-1.7.5-r2.ebuild
[modify] https://crrev.com/ee058b6a072ff8846bf6f45b990a58b7dd456c97/net-dns/c-ares/c-ares-1.7.5.ebuild
[add] https://crrev.com/ee058b6a072ff8846bf6f45b990a58b7dd456c97/net-dns/c-ares/files/c-ares-1.7.5-mkquery-heap-overflow.patch

Comment 15 by keta...@chromium.org, Sep 26 2016 

Please add merge-request label to this bug once all CLs are ready for merge.

Comment 16 by rickyz@chromium.org, Sep 26 2016

Labels: Merge-Request-53 Merge-Request-54

Comment 17 by keta...@chromium.org, Sep 26 2016

Labels: -Merge-Request-53 Merge-Approved-53
Approving merge to M53 cros. Please merge as soon as possible.
Project Member

Comment 20 by bugdroid1@chromium.org, Sep 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/0df1d36d4b20c368d8fdee2b1e3a0164343b24ea

commit 0df1d36d4b20c368d8fdee2b1e3a0164343b24ea
Author: Ricky Zhou <rickyz@chromium.org>
Date: Thu Sep 22 02:12:46 2016

Fix heap overflow in ares_mkquery.

Thanks to the reporter for the patch.

BUG= chromium:649040 
TEST=Trybots

Change-Id: Ic038b69c227d8637c213ec655ace6bd567040db2
Reviewed-on: https://chromium-review.googlesource.com/388126
Commit-Ready: Ricky Zhou <rickyz@chromium.org>
Tested-by: Ricky Zhou <rickyz@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
(cherry picked from commit ee058b6a072ff8846bf6f45b990a58b7dd456c97)
Reviewed-on: https://chromium-review.googlesource.com/389533
Reviewed-by: Ricky Zhou <rickyz@chromium.org>
Commit-Queue: Ricky Zhou <rickyz@chromium.org>

[rename] https://crrev.com/0df1d36d4b20c368d8fdee2b1e3a0164343b24ea/net-dns/c-ares/c-ares-1.7.5-r2.ebuild
[modify] https://crrev.com/0df1d36d4b20c368d8fdee2b1e3a0164343b24ea/net-dns/c-ares/c-ares-1.7.5.ebuild
[add] https://crrev.com/0df1d36d4b20c368d8fdee2b1e3a0164343b24ea/net-dns/c-ares/files/c-ares-1.7.5-mkquery-heap-overflow.patch
Project Member

Comment 21 by bugdroid1@chromium.org, Sep 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/19f1ea9a017a80c59f13e9f54bd4ab6ac2d63a07

commit 19f1ea9a017a80c59f13e9f54bd4ab6ac2d63a07
Author: Ricky Zhou <rickyz@chromium.org>
Date: Thu Sep 22 02:12:46 2016

Fix heap overflow in ares_mkquery.

Thanks to the reporter for the patch.

BUG= chromium:649040 
TEST=Trybots

Change-Id: Ic038b69c227d8637c213ec655ace6bd567040db2
Reviewed-on: https://chromium-review.googlesource.com/388126
Commit-Ready: Ricky Zhou <rickyz@chromium.org>
Tested-by: Ricky Zhou <rickyz@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
(cherry picked from commit ee058b6a072ff8846bf6f45b990a58b7dd456c97)
Reviewed-on: https://chromium-review.googlesource.com/389534
Reviewed-by: Ricky Zhou <rickyz@chromium.org>
Commit-Queue: Ricky Zhou <rickyz@chromium.org>

[rename] https://crrev.com/19f1ea9a017a80c59f13e9f54bd4ab6ac2d63a07/net-dns/c-ares/c-ares-1.7.5-r2.ebuild
[modify] https://crrev.com/19f1ea9a017a80c59f13e9f54bd4ab6ac2d63a07/net-dns/c-ares/c-ares-1.7.5.ebuild
[add] https://crrev.com/19f1ea9a017a80c59f13e9f54bd4ab6ac2d63a07/net-dns/c-ares/files/c-ares-1.7.5-mkquery-heap-overflow.patch
Project Member

Comment 22 by sheriffbot@chromium.org, Sep 27 2016

Status: Fixed (was: Started)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 23 by dimu@chromium.org, Sep 27 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 24 by sheriffbot@chromium.org, Sep 28 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 25 by mmoroz@chromium.org, Sep 30 2016

Cc: mmoroz@chromium.org
FTR, this seems to be fixed upstream and got assigned CVE-2016-5180: https://c-ares.haxx.se/adv_20160929.html

Comment 26 by mnissler@chromium.org, Sep 30 2016 

Yes, we've been in close contact with upstream regarding this issue. The plan is to upgrade to the fixed c-ares version, but given that the Chrome OS version is quite far behind we've patched our version first to get a fix out quickly.

Comment 27 by mnissler@chromium.org, Sep 30 2016 

I've created a bug to track the c-ares update:  issue 651760
Project Member

Comment 28 by sheriffbot@chromium.org, Sep 30 2016 

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 29 by mnissler@chromium.org, Sep 30 2016

Labels: -Merge-Approved-53 -Merge-Approved-54
All merges done already.
Project Member

Comment 30 by sheriffbot@chromium.org, Jan 3 2017

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 31 by dchan@google.com, Jan 21 2017

Labels: VerifyIn-57

Comment 32 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58

Comment 33 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 34 by dchan@google.com, May 30 2017

Labels: VerifyIn-60

Comment 35 by dchan@chromium.org, Aug 1 2017

Labels: VerifyIn-61

Comment 36 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)
►

Sign in to add a comment