gcm's SocketOutputStream::Flush can write arbitrary data to the network |
||||||||||||
Issue descriptionDuring the review of https://chromium-review.googlesource.com/c/chromium/src/+/1142344, we found that gcm::SocketOutputStream::Flush() can write data that is beyond the contents of |write_buffer_| to |socket_|. Code link: https://cs.chromium.org/chromium/src/google_apis/gcm/base/socket_stream.cc?rcl=b9d5a12b989872ef2b321a94538a129d76a613f8&l=259 The |next_pos_| is not decremented upon write completion. If |socket_->Write()| is a partial write, we will be in trouble. For instance, if we have 10 bytes split between two writes (i.e. next_pos_ = 10, and the first socket_->Write() returns 5). The second socket_->Write() will try to write 10 bytes starting at byte 5 of the write buffer. We will end up writing another 5 arbitrary bytes to |socket_|, because we are passing the start pointer and telling net::StreamSocket to write 10 bytes starting at the start pointer. I am uploading a fix in a bit.
,
Jul 23
Sheriff here adding some labels. Tentatively setting Severity-Medium as this seems like it could allow an attacker to read semi-random data and gain access to it. It looks like this has been this way for quite some time (at least 2014, looking at blame/history on the lines in question).
,
Jul 23
WIP CL: https://chromium-review.googlesource.com/c/chromium/src/+/1147475 I will send it out when trybots are passing.
,
Jul 24
,
Jul 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/74222e7796cecc859109d9252e96e396106423c3 commit 74222e7796cecc859109d9252e96e396106423c3 Author: Helen Li <xunjieli@chromium.org> Date: Tue Jul 24 14:39:47 2018 Fix gcm::SocketOutputStream to handle partial writes gcm::SocketOutputStream is not keeping track of partial writes correctly. It will always try to write |next_pos_| bytes. This CL adds a regression test. Bug: 866635 Change-Id: Iccfc4c88a9247ff151073d3691e20ba052f082b0 Reviewed-on: https://chromium-review.googlesource.com/1147475 Commit-Queue: Helen Li <xunjieli@chromium.org> Reviewed-by: Peter Beverloo <peter@chromium.org> Reviewed-by: Maks Orlovich <morlovich@chromium.org> Cr-Commit-Position: refs/heads/master@{#577531} [modify] https://crrev.com/74222e7796cecc859109d9252e96e396106423c3/google_apis/gcm/base/socket_stream.cc [modify] https://crrev.com/74222e7796cecc859109d9252e96e396106423c3/google_apis/gcm/base/socket_stream.h [modify] https://crrev.com/74222e7796cecc859109d9252e96e396106423c3/google_apis/gcm/base/socket_stream_unittest.cc
,
Jul 24
,
Jul 25
,
Aug 16
,
Aug 16
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 16
+ awhalley@ (Security TPM) for M69 merge review.
,
Aug 17
govind@ - Good for 69
,
Aug 17
Approving merge to M69 branch 3497 based on comment #11. Please merge ASAP so we can pick it up for next week beta. Thank you.
,
Aug 17
awhalley: Are you doing the merge? or do you want me to do it?
,
Aug 17
I'm happy to, but might be easier if you do in case there are any conflicts that need to be resolved :-)
,
Aug 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/718cd9d8c4430fb56ea2bef09bdb54fbfb104189 commit 718cd9d8c4430fb56ea2bef09bdb54fbfb104189 Author: Helen Li <xunjieli@chromium.org> Date: Fri Aug 17 17:56:00 2018 Fix gcm::SocketOutputStream to handle partial writes gcm::SocketOutputStream is not keeping track of partial writes correctly. It will always try to write |next_pos_| bytes. This CL adds a regression test. TBR=peter@chromium.org TBR=morlovich@chromium.org Bug: 866635 Change-Id: Iccfc4c88a9247ff151073d3691e20ba052f082b0 Reviewed-on: https://chromium-review.googlesource.com/1147475 Commit-Queue: Helen Li <xunjieli@chromium.org> Reviewed-by: Peter Beverloo <peter@chromium.org> Reviewed-by: Maks Orlovich <morlovich@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#577531}(cherry picked from commit 74222e7796cecc859109d9252e96e396106423c3) Reviewed-on: https://chromium-review.googlesource.com/1180121 Reviewed-by: Helen Li <xunjieli@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#688} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/718cd9d8c4430fb56ea2bef09bdb54fbfb104189/google_apis/gcm/base/socket_stream.cc [modify] https://crrev.com/718cd9d8c4430fb56ea2bef09bdb54fbfb104189/google_apis/gcm/base/socket_stream.h [modify] https://crrev.com/718cd9d8c4430fb56ea2bef09bdb54fbfb104189/google_apis/gcm/base/socket_stream_unittest.cc
,
Aug 28
,
Oct 31
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 |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by xunji...@chromium.org
, Jul 23The fix is to use BytesRemaining() instead of |next_pos_|. - DVLOG(1) << "Flushing " << next_pos_ << " bytes into socket."; + DVLOG(1) << "Flushing " << write_buffer_->BytesRemaining() + << " bytes into socket."; int result = - socket_->Write(write_buffer_.get(), next_pos_, + socket_->Write(write_buffer_.get(), write_buffer_->BytesRemaining(), base::Bind(&SocketOutputStream::FlushCompletionCallback, weak_ptr_factory_.GetWeakPtr(), callback), traffic_annotation_); @@ -323,7 +324,7 @@ void SocketOutputStream::FlushCompletionCallback( DCHECK_GT(result, net::OK); last_error_ = net::OK; - if (write_buffer_->BytesConsumed() + result < next_pos_) { + if (write_buffer_->BytesRemaining() > 0) { DVLOG(1) << "Partial flush complete. Retrying."; // Only a partial write was completed. Flush again to finish the write. write_buffer_->DidConsume(result);