P2P TCP sockets may crash the network service after receiving invalid packet |
|||||||||||||
Issue descriptionSee 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.
,
Sep 14
,
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
,
Sep 14
,
Sep 15
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
,
Sep 17
,
Sep 17
,
Sep 17
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
,
Sep 17
How safe is this merge and how confident are you this won't regress anything in M70?
,
Sep 18
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.
,
Sep 18
,
Sep 18
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
,
Sep 19
,
Sep 19
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
,
Sep 25
,
Dec 25
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 sergeyu@chromium.org
, Sep 14