Issue metadata
Sign in to add a comment
|
Incorrect packet size check leads to heap-buffer-overflow in pseudotcp |
||||||||||||||||||||||
Issue descriptionPseudoTCP::parse is provided by WebRTC and called by remoting with data read from a socket. It (https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/base/pseudotcp.cc?sq=package:chromium&rcl=1466046988&l=575) contains the following code. Note how size is checked < 12 instead of < HEADER_SIZE, so I think this will cause an OOB read. bool PseudoTcp::parse(const uint8_t* buffer, uint32_t size) { if (size < 12) return false; Segment seg; seg.conv = bytes_to_long(buffer); seg.seq = bytes_to_long(buffer + 4); seg.ack = bytes_to_long(buffer + 8); seg.flags = buffer[13]; seg.wnd = bytes_to_short(buffer + 14); seg.tsval = bytes_to_long(buffer + 16); seg.tsecr = bytes_to_long(buffer + 20); seg.data = reinterpret_cast<const char *>(buffer) + HEADER_SIZE; seg.len = size - HEADER_SIZE; pbos@, assuming I'm not being super dumb, do you know what the procedure for dealing with bugs like this is? (I guess just uploading the obvious patch would make it public...)
,
Jun 16 2016
sergeyu@ FYI, I'm not not sure who to notify. This isn't used by webrtc in general and I see a commit called "Put pseudotcp back because remoting uses it." in log history so I think you're the only users (and I believe we want to nuke this code). Is there a chance of us nuking pseudotcp anytime soon? This code looks very error prone (gaping issues such as the one above).
,
Jun 16 2016
Some other entertaining snippets from this code: > grep "//.*[?]" ~/build/webrtc-checkout/src/webrtc/p2p/base/pseudotcp.cc const uint32_t IP_HEADER_SIZE = 20; // (+ up to 40 bytes of options?) // TODO: Make JINGLE_HEADER_SIZE transparent to this code? // !?! Rethink these times // TODO(jbeda): !?! Not sure about this was closed business // If this is the wrong conversation, send a reset!?! (with the correct conversation?) // !?! send reset? // !?! Note, tcp says don't do this... but otherwise how does a closed window become open? // !?! A bit hacky // !?! We need to break up all outstanding and pending packets and then retransmit!?! // !?! Should we reset m_largest here?
,
Jun 16 2016
,
Jun 16 2016
PseudoTCP is used only in chromoting (hopefully not for very long time). I don't think security risk is high here. I'm not sure it's exploitable - as far as I can tell it can only result in segfault or the packet being rejected by the SSL layer (seg.len is uint32_t). And even if it is exploitable it may only be useful to exploit the client in remote-assistance scenario (because client already has full access to the host), so an attack would also require some social engineering. That client code runs in PNaCl sandbox (Android and iOS don't support remote assistance). Still it's best merge and push out the fix ASAP with our M52. Do you have a fix ready? Just land it if you do, otherwise I'll fix it myself.
,
Jun 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/external/webrtc.git/+/d1523ca38f98bd4e46faabcd2db57fad1e8ad154 commit d1523ca38f98bd4e46faabcd2db57fad1e8ad154 Author: sergeyu <sergeyu@chromium.org> Date: Fri Jun 17 01:08:12 2016 Fix header size check in PseudoTcp::parse(). BUG= chromium:620694 TBR=pbos@webrtc.org Review-Url: https://codereview.webrtc.org/2073963002 Cr-Commit-Position: refs/heads/master@{#13177} [modify] https://crrev.com/d1523ca38f98bd4e46faabcd2db57fad1e8ad154/webrtc/p2p/base/pseudotcp.cc
,
Jun 20 2016
Need to merge the fix to M52 branch. The change doesn't affect chrome.
,
Jun 20 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
Jun 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/external/webrtc.git/+/c04d6b6c9bb422e18c47c2c6daa545daf095ac71 commit c04d6b6c9bb422e18c47c2c6daa545daf095ac71 Author: Sergey Ulanov <sergeyu@chromium.org> Date: Mon Jun 20 19:37:00 2016 Fix header size check in PseudoTcp::parse(). BUG= chromium:620694 TBR=pbos@webrtc.org Review-Url: https://codereview.webrtc.org/2073963002 Cr-Commit-Position: refs/heads/master@{#13177} (cherry picked from commit d1523ca38f98bd4e46faabcd2db57fad1e8ad154) Review URL: https://codereview.webrtc.org/2083723002 . Cr-Commit-Position: refs/branch-heads/52@{#6} Cr-Branched-From: a376e70cf9d0df3c35d53533b454da542661775b-refs/heads/master@{#12798} [modify] https://crrev.com/c04d6b6c9bb422e18c47c2c6daa545daf095ac71/webrtc/p2p/base/pseudotcp.cc
,
Jun 21 2016
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. - Your friendly ClusterFuzz
,
Jun 21 2016
,
Jun 21 2016
,
Jun 24 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
,
Jun 24 2016
,
Jun 24 2016
,
Jul 19 2016
,
Sep 27 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 pbos@chromium.org
, Jun 16 2016