New issue
Advanced search Search tips

Issue 884242 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 18
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

P2P TCP sockets may crash the network service after receiving invalid packet

Project Member Reported by sergeyu@chromium.org, Sep 14

Issue description

See services/network/p2p/socket_tcp.cc . The problem there is that network::P2PSocketTcpBase::OnPacket() may destroy the object by calling OnError(), but not all callers handle this case - they may try to use the object after it has been deleted. This means that network process may crash when handling invalid packet received from WebRTC peer.

 
https://chromium-review.googlesource.com/c/chromium/src/+/1220676 will address this issue (currently in CQ).
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/83329a47787cd4974400a49b766676f32c27d7cb

commit 83329a47787cd4974400a49b766676f32c27d7cb
Author: Sergey Ulanov <sergeyu@chromium.org>
Date: Fri Sep 14 18:23:51 2018

Fix issues in P2P socket error handling logic.

P2P sockets destroy themselves on errors, but there were some cases when
they would still access themselves after an error has occured. Some, but
not all of these issues were addressed in crrev.com/590539. This CL
addresses remaining problems. Particularly errors from
P2PSocketTcpBase::OnPacket() were not handled properly.

Also updated unittests to destroy sockets, to make it possible to catch
these issues in tests in the future.

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I9cae97c5715f199c3f6dbccd3fbcd687e276a325
Bug:  884242 
Reviewed-on: https://chromium-review.googlesource.com/1220676
Commit-Queue: Sergey Ulanov <sergeyu@chromium.org>
Reviewed-by: Oskar Sundbom <ossu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591404}
[modify] https://crrev.com/83329a47787cd4974400a49b766676f32c27d7cb/services/network/p2p/socket_tcp.cc
[modify] https://crrev.com/83329a47787cd4974400a49b766676f32c27d7cb/services/network/p2p/socket_tcp.h
[modify] https://crrev.com/83329a47787cd4974400a49b766676f32c27d7cb/services/network/p2p/socket_tcp_server_unittest.cc
[modify] https://crrev.com/83329a47787cd4974400a49b766676f32c27d7cb/services/network/p2p/socket_tcp_unittest.cc
[modify] https://crrev.com/83329a47787cd4974400a49b766676f32c27d7cb/services/network/p2p/socket_test_utils.cc
[modify] https://crrev.com/83329a47787cd4974400a49b766676f32c27d7cb/services/network/p2p/socket_test_utils.h
[modify] https://crrev.com/83329a47787cd4974400a49b766676f32c27d7cb/services/network/p2p/socket_udp.cc
[modify] https://crrev.com/83329a47787cd4974400a49b766676f32c27d7cb/services/network/p2p/socket_udp.h
[modify] https://crrev.com/83329a47787cd4974400a49b766676f32c27d7cb/services/network/p2p/socket_udp_unittest.cc

Labels: Security_Severity-High Security_Impact-Beta
Project Member

Comment 5 by sheriffbot@chromium.org, Sep 15

Labels: ReleaseBlock-Stable
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: benmason@chromium.org
Labels: Merge-Request-70
Project Member

Comment 8 by sheriffbot@chromium.org, Sep 17

Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
How safe is this merge and how confident are you this won't regress anything in M70?
Most of the changes in the CL above are in the test code. Production code changes are rather simple, so they are not likely to break anything. 
Labels: -Merge-Review-70 Merge-Approved-70
Project Member

Comment 12 by sheriffbot@chromium.org, Sep 18

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
Project Member

Comment 13 by sheriffbot@chromium.org, Sep 19

Labels: Restrict-View-SecurityNotify
Project Member

Comment 14 by bugdroid1@chromium.org, Sep 19

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/995edadc42667c1f7e9422e1c5a3c4e30fecac46

commit 995edadc42667c1f7e9422e1c5a3c4e30fecac46
Author: Sergey Ulanov <sergeyu@chromium.org>
Date: Wed Sep 19 17:22:40 2018

Fix issues in P2P socket error handling logic.

P2P sockets destroy themselves on errors, but there were some cases when
they would still access themselves after an error has occured. Some, but
not all of these issues were addressed in crrev.com/590539. This CL
addresses remaining problems. Particularly errors from
P2PSocketTcpBase::OnPacket() were not handled properly.

Also updated unittests to destroy sockets, to make it possible to catch
these issues in tests in the future.

TBR=sergeyu@chromium.org

(cherry picked from commit 83329a47787cd4974400a49b766676f32c27d7cb)

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I9cae97c5715f199c3f6dbccd3fbcd687e276a325
Bug:  884242 
Reviewed-on: https://chromium-review.googlesource.com/1220676
Commit-Queue: Sergey Ulanov <sergeyu@chromium.org>
Reviewed-by: Oskar Sundbom <ossu@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#591404}
Reviewed-on: https://chromium-review.googlesource.com/1234322
Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#530}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/995edadc42667c1f7e9422e1c5a3c4e30fecac46/services/network/p2p/socket_tcp.cc
[modify] https://crrev.com/995edadc42667c1f7e9422e1c5a3c4e30fecac46/services/network/p2p/socket_tcp.h
[modify] https://crrev.com/995edadc42667c1f7e9422e1c5a3c4e30fecac46/services/network/p2p/socket_tcp_server_unittest.cc
[modify] https://crrev.com/995edadc42667c1f7e9422e1c5a3c4e30fecac46/services/network/p2p/socket_tcp_unittest.cc
[modify] https://crrev.com/995edadc42667c1f7e9422e1c5a3c4e30fecac46/services/network/p2p/socket_test_utils.cc
[modify] https://crrev.com/995edadc42667c1f7e9422e1c5a3c4e30fecac46/services/network/p2p/socket_test_utils.h
[modify] https://crrev.com/995edadc42667c1f7e9422e1c5a3c4e30fecac46/services/network/p2p/socket_udp.cc
[modify] https://crrev.com/995edadc42667c1f7e9422e1c5a3c4e30fecac46/services/network/p2p/socket_udp.h
[modify] https://crrev.com/995edadc42667c1f7e9422e1c5a3c4e30fecac46/services/network/p2p/socket_udp_unittest.cc

Labels: -ReleaseBlock-Stable
Project Member

Comment 16 by sheriffbot@chromium.org, Dec 25

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

Sign in to add a comment