Issue metadata
Sign in to add a comment
|
Security: Multiple vulnerabilities in mojo channel implementation |
||||||||||||||||||||||
Issue description
In mojo::edk::Channel::OnReadComplete() (which appears to be given data from PlatformChannelRecvmsg which isn’t validated):
const Message::Header* header = reinterpret_cast<const Message::Header*>(
read_buffer_->occupied_bytes());
if (header->num_bytes < sizeof(Message::Header) ||
header->num_bytes > kMaxChannelMessageSize) {
LOG(ERROR) << "Invalid message size: " << header->num_bytes;
return false;
}
if (read_buffer_->num_occupied_bytes() < header->num_bytes) {
// Not enough data available to read the full message. Hint to the
// implementation that it should try reading the full size of the message.
*next_read_size_hint =
header->num_bytes - read_buffer_->num_occupied_bytes();
return true;
}
...
size_t extra_header_size =
header->num_header_bytes - sizeof(Message::Header);
const void* extra_header = header + 1;
size_t payload_size = header->num_bytes - header->num_header_bytes;
void* payload =
payload_size ? reinterpret_cast<Message::Header*>(
const_cast<char*>(read_buffer_->occupied_bytes()) +
header->num_header_bytes)
* |header->num_bytes| is checked against |read_buffer->num_occupied_bytes()|. However, there is no validation at all for |header->num_header_bytes| (uint16_t). For instance, |num_header_bytes| could be > |header->num_bytes|, leading to an integer underflow and a very large |payload_size|.
* The |payload| pointer is also assigned |read_buffer->occupied_bytes()| + |header->num_header_bytes|, which could point well past the read buffer, leading to wild reads later on.
* |extra_header| could also point outside the read buffer before being passed to GetReadPlatformHandles(). |extra_header_size| can likewise be a bad value, but it doesn’t appear to be used at all in ChannelPosix::GetReadPlatformHandles() anyway.
In mojo::edk::NodeChannel::OnChannelMessage(), which receives the |payload| and |payload_size| from OnReadComplete(), |payload_size| is not used or checked for the vast majority of message types. E.g.
case MessageType::ACCEPT_CHILD: {
const AcceptChildData* data;
GetMessagePayload(payload, &data);
delegate_->OnAcceptChild(remote_node_name_, data->parent_name,
data->token);
break;
}
GetMessagePayload simply advances |payload| past the payload header (without any checks), and assigns that pointer to |data| and passes this to OnAcceptChild().
Unfortunately, I don’t have a good PoC for these bugs -- I did make small modifications to OnFileCanReadWithoutBlocking to modify the header values in the read buffer before passing them to Channel::OnReadComplete, and could reproduce crashes.
It appears that the code in question wasn’t designed with untrusted input in mind, so there are likely more issues that I’ve missed.
,
May 13 2016
,
May 13 2016
,
May 13 2016
,
May 13 2016
(+non Chromium Mojo FYI)
,
May 13 2016
> |extra_header_size| can likewise be a bad value, but it doesn’t appear to be used at all in ChannelPosix::GetReadPlatformHandles() anyway. Ooops, I'm wrong about this one. extra_header_size is used in ChannelPosix::GetReadPlatformHandles() for IOS/OSX where it matters. Please disregard that point.
,
May 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/10eebb639c2ca8d062ce9c3623f2b7bd67dd9f4c commit 10eebb639c2ca8d062ce9c3623f2b7bd67dd9f4c Author: rockot <rockot@chromium.org> Date: Sat May 14 00:52:47 2016 [mojo-edk] Add sanity checks to NodeChannel inputs Prevents reading memory beyond the bounds of a received NodeChannel message. BUG= 611887 Review-Url: https://codereview.chromium.org/1977493004 Cr-Commit-Position: refs/heads/master@{#393713} [modify] https://crrev.com/10eebb639c2ca8d062ce9c3623f2b7bd67dd9f4c/mojo/edk/system/node_channel.cc
,
May 14 2016
,
May 14 2016
,
May 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8eb4c80b7558a1bdd547e24b6b129b0d84223b21 commit 8eb4c80b7558a1bdd547e24b6b129b0d84223b21 Author: rockot <rockot@chromium.org> Date: Mon May 16 18:25:03 2016 [mojo-edk] Better validation of untrusted message data Adds some more sanity checks to close a Channel if it receives bad data. BUG= 611887 R=amistry@chromium.org,ochang@chromium.org Review-Url: https://codereview.chromium.org/1985523002 Cr-Commit-Position: refs/heads/master@{#393874} [modify] https://crrev.com/8eb4c80b7558a1bdd547e24b6b129b0d84223b21/mojo/edk/system/channel.cc [modify] https://crrev.com/8eb4c80b7558a1bdd547e24b6b129b0d84223b21/mojo/edk/system/channel.h [modify] https://crrev.com/8eb4c80b7558a1bdd547e24b6b129b0d84223b21/mojo/edk/system/channel_posix.cc [modify] https://crrev.com/8eb4c80b7558a1bdd547e24b6b129b0d84223b21/mojo/edk/system/channel_win.cc
,
May 16 2016
,
May 17 2016
FYI: another similar issue here: bug 612364
,
May 17 2016
Adding Merge-Triage label for tracking purposes. Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Request-XX label, where XX is the Chrome milestone. When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com. Your fix is very close to the branch point. After the branch happens, please make sure to check if your fix is in. - Your friendly ClusterFuzz
,
May 17 2016
,
May 17 2016
[Automated comment] Less than 2 weeks to go before stable on M51, manual review required.
,
May 17 2016
Before we approve merge to M51, Could you please confirm whether this bug is baked/verified in Canary and safe to merge?
,
May 18 2016
,
May 18 2016
This is safe to merge
,
May 18 2016
Ok, approving merge to M51 branch 2704 based on update #19. Please merge ASAP. Thank you.
,
May 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/018079748682ecd00c467f22ab498e4eccadd995 commit 018079748682ecd00c467f22ab498e4eccadd995 Author: Ken Rockot <rockot@chromium.org> Date: Wed May 18 21:22:08 2016 [mojo-edk] Add sanity checks to NodeChannel inputs Prevents reading memory beyond the bounds of a received NodeChannel message. BUG= 611887 Review-Url: https://codereview.chromium.org/1977493004 Cr-Commit-Position: refs/heads/master@{#393713} (cherry picked from commit 10eebb639c2ca8d062ce9c3623f2b7bd67dd9f4c) Review URL: https://codereview.chromium.org/1987423002 . Cr-Commit-Position: refs/branch-heads/2704@{#587} Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251} [modify] https://crrev.com/018079748682ecd00c467f22ab498e4eccadd995/mojo/edk/system/node_channel.cc
,
May 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8f78c72aac6e7ca010015aff715b3ab1f6ebe712 commit 8f78c72aac6e7ca010015aff715b3ab1f6ebe712 Author: Ken Rockot <rockot@chromium.org> Date: Wed May 18 21:34:54 2016 [mojo-edk] Better validation of untrusted message data Adds some more sanity checks to close a Channel if it receives bad data. BUG= 611887 R=amistry@chromium.org,ochang@chromium.org Review-Url: https://codereview.chromium.org/1985523002 Cr-Commit-Position: refs/heads/master@{#393874} (cherry picked from commit 8eb4c80b7558a1bdd547e24b6b129b0d84223b21) Review URL: https://codereview.chromium.org/1997453002 . Cr-Commit-Position: refs/branch-heads/2704@{#588} Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251} [modify] https://crrev.com/8f78c72aac6e7ca010015aff715b3ab1f6ebe712/mojo/edk/system/channel.cc [modify] https://crrev.com/8f78c72aac6e7ca010015aff715b3ab1f6ebe712/mojo/edk/system/channel.h [modify] https://crrev.com/8f78c72aac6e7ca010015aff715b3ab1f6ebe712/mojo/edk/system/channel_posix.cc [modify] https://crrev.com/8f78c72aac6e7ca010015aff715b3ab1f6ebe712/mojo/edk/system/channel_win.cc
,
May 18 2016
Both CLs merged to M51. Should we also merge these security fixes back to M50? I'm concerned that it wouldn't be a clean merge because the code has changed quite a bit since M50 branch. WDYT ochang@?
,
May 18 2016
Generally we target the current stable milestone (M50) for high severity bugs, so if a merge is possible that would be great. That said, if the patches can't be applied without significant changes, it should be fine to wait for M51 stable since that's quite soon.
,
May 18 2016
OK - I will attempt an M50 merge upon approval
,
May 18 2016
I don't think there will be any more m50 releases... +govind
,
May 18 2016
That is correct. There won't be any M50 releases as we're very close to M51 stable promotion.
,
May 18 2016
,
May 18 2016
I think the status quo has been to merge (if possible) even if there aren't any planned releases in case there are emergency/unplanned releases?
,
May 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4a0b247b4a5034a97366da5deb31d6ac9a7bcf4b commit 4a0b247b4a5034a97366da5deb31d6ac9a7bcf4b Author: Ken Rockot <rockot@chromium.org> Date: Wed May 18 23:20:02 2016 [mojo-edk] Better validation of untrusted message data Adds some more sanity checks to close a Channel if it receives bad data. NOTE: This is a reland of the merge to M51 to resolve Mach port handling conflicts on Mac. BUG= 611887 R=amistry@chromium.org,ochang@chromium.org Review-Url: https://codereview.chromium.org/1985523002 Cr-Commit-Position: refs/heads/master@{#393874} (cherry picked from commit 8eb4c80b7558a1bdd547e24b6b129b0d84223b21) TBR=amistry@chromium.org Review URL: https://codereview.chromium.org/1993963002 . Cr-Commit-Position: refs/branch-heads/2704@{#594} Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251} [modify] https://crrev.com/4a0b247b4a5034a97366da5deb31d6ac9a7bcf4b/mojo/edk/system/channel.cc [modify] https://crrev.com/4a0b247b4a5034a97366da5deb31d6ac9a7bcf4b/mojo/edk/system/channel.h [modify] https://crrev.com/4a0b247b4a5034a97366da5deb31d6ac9a7bcf4b/mojo/edk/system/channel_posix.cc [modify] https://crrev.com/4a0b247b4a5034a97366da5deb31d6ac9a7bcf4b/mojo/edk/system/channel_win.cc
,
May 24 2016
,
May 24 2016
,
May 24 2016
,
Aug 23 2016
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
,
Oct 1 2016
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
,
Oct 2 2016
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
,
Oct 2 2016
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by och...@chromium.org
, May 13 2016Status: Assigned (was: Unconfirmed)