Project: chromium Issues People Development process History Sign in
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 Security: ChromeOS 1 byte write overflow in c-ares
Starred by 4 users Project Member Reported by elawre...@chromium.org, Sep 21 2016 Back to list
Status: Fixed
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 0
Type: Bug-Security

Blocking:
issue 648971



Sign in to add a comment
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.
 
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
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
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.
Cc: vapier@chromium.org
Thanks gzobqq!
Cc: dan...@haxx.se
Cc: daniel.h...@gmail.com
Status: Started
Flipping to Started, works is already underway here: https://chromium-review.googlesource.com/#/c/388126/
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

Please add merge-request label to this bug once all CLs are ready for merge.
Labels: Merge-Request-53 Merge-Request-54
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
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
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
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.
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
Labels: -Merge-Approved-53 -Merge-Approved-54
All merges done already.
Project Member Comment 30 by sheriffbot@chromium.org, Jan 3
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
Labels: VerifyIn-57
Labels: VerifyIn-58
Labels: VerifyIn-59
Labels: VerifyIn-60
Sign in to add a comment