Use-after-free in DnsTransaction, again |
|||||||||||||||||
Issue descriptionI discovered while following up on issue 788131 that we still have a similar use-after-free. Unlike the one in that bug, the "new" one is in stable and there's nothing to revert. The fuzzer probably hasn't found it because it requires a well-formed DNS response. If this is really critical, we have the option of turning off the async resolver using Finch. I'd like to avoid that if possible since it's a big performance regression. This bug happens when the resolver makes a DNS query and then gets a synchronous invalid response followed immediately by a synchronous valid response. We randomize ports and transaction IDs, and the attacker would have to get both right. So I'd be surprised to see a working exploit. I looked at crashes and didn't see any with a stack trace like this. I don't have a fix yet, I only have a regression test. I expect I'll be able to find a fix that's a fairly safe merge.
,
Dec 8 2017
Unfortunately, memory corruption in the browser is Critical. Are we sure it affects only the platforms you list?
,
Dec 8 2017
Those are the only platforms with the async resolver enabled. On Chrome OS and Mac it's the default (and has been for years), on Android it's a 50% dev/beta/canary and 1% stable experiment. The fix is in the CQ right now.
,
Dec 8 2017
Sounds great. Thanks!
,
Dec 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9069772b10e2796e4a09d6248a81b3c4ea4506d5 commit 9069772b10e2796e4a09d6248a81b3c4ea4506d5 Author: Miriam Gershenson <mgersh@chromium.org> Date: Fri Dec 08 21:43:05 2017 Don't clear DnsAttempts that have received a response When a DnsTransaction finishes synchronously, it posts a task to run its callback. In the meantime, DnsAttempts can keep running, and if a TCP attempt starts, it will delete all the previous attempts. Then the callback will run and use an attempt which was just deleted. This fix is designed to be easy to merge to branches. Bug: 788131 , 793099 Change-Id: I6b695fb0f7ff7a7bf1257a3c6baea881478eaa16 Reviewed-on: https://chromium-review.googlesource.com/817681 Reviewed-by: Julia Tuttle <juliatuttle@chromium.org> Commit-Queue: Miriam Gershenson <mgersh@chromium.org> Cr-Commit-Position: refs/heads/master@{#522883} [modify] https://crrev.com/9069772b10e2796e4a09d6248a81b3c4ea4506d5/net/dns/dns_transaction.cc [modify] https://crrev.com/9069772b10e2796e4a09d6248a81b3c4ea4506d5/net/dns/dns_transaction_unittest.cc
,
Dec 9 2017
This is a critical security issue. 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
,
Dec 9 2017
+awhalley@, could you ptal we already cut M63 Beta RC for desktop for release on Monday and this is marked as Beta blocker by sheriffbot?
,
Dec 9 2017
No need to block Monday's 63 Beta. We should merge this on to 63 on Monday afternoon/Tuesday morning once it's had a little time on Canary. We'll pick it up for the 63 spin later next week.
,
Dec 10 2017
,
Dec 10 2017
Your change meets the bar and is auto-approved for M64. Please go ahead and merge the CL to branch 3282 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8b056cf36e38ee7070509596153a717e87a33353 commit 8b056cf36e38ee7070509596153a717e87a33353 Author: Miriam Gershenson <mgersh@chromium.org> Date: Mon Dec 11 18:09:03 2017 Don't clear DnsAttempts that have received a response When a DnsTransaction finishes synchronously, it posts a task to run its callback. In the meantime, DnsAttempts can keep running, and if a TCP attempt starts, it will delete all the previous attempts. Then the callback will run and use an attempt which was just deleted. This fix is designed to be easy to merge to branches. Bug: 788131 , 793099 Change-Id: I6b695fb0f7ff7a7bf1257a3c6baea881478eaa16 Reviewed-on: https://chromium-review.googlesource.com/817681 Reviewed-by: Julia Tuttle <juliatuttle@chromium.org> Commit-Queue: Miriam Gershenson <mgersh@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#522883}(cherry picked from commit 9069772b10e2796e4a09d6248a81b3c4ea4506d5) Reviewed-on: https://chromium-review.googlesource.com/820190 Reviewed-by: Miriam Gershenson <mgersh@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#132} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/8b056cf36e38ee7070509596153a717e87a33353/net/dns/dns_transaction.cc [modify] https://crrev.com/8b056cf36e38ee7070509596153a717e87a33353/net/dns/dns_transaction_unittest.cc
,
Dec 11 2017
,
Dec 11 2017
+ cmasso@ (Chrome on Android TPM)
,
Dec 11 2017
awhalley@ Do we have to respin for this?
,
Dec 11 2017
I mean now? or it needs more bake time.
,
Dec 12 2017
govind@ - good for 63
,
Dec 12 2017
Approving merge to M63 branch 3239 based on comment #16. Please merge ASAP. Thank you.
,
Dec 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/28127157132dc40808c4ec4037884ddbc4600b2e commit 28127157132dc40808c4ec4037884ddbc4600b2e Author: Miriam Gershenson <mgersh@chromium.org> Date: Tue Dec 12 19:38:09 2017 Don't clear DnsAttempts that have received a response When a DnsTransaction finishes synchronously, it posts a task to run its callback. In the meantime, DnsAttempts can keep running, and if a TCP attempt starts, it will delete all the previous attempts. Then the callback will run and use an attempt which was just deleted. This fix is designed to be easy to merge to branches. Bug: 788131 , 793099 Change-Id: I6b695fb0f7ff7a7bf1257a3c6baea881478eaa16 Reviewed-on: https://chromium-review.googlesource.com/817681 Reviewed-by: Julia Tuttle <juliatuttle@chromium.org> Commit-Queue: Miriam Gershenson <mgersh@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#522883}(cherry picked from commit 9069772b10e2796e4a09d6248a81b3c4ea4506d5) Reviewed-on: https://chromium-review.googlesource.com/822891 Reviewed-by: Miriam Gershenson <mgersh@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#664} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/28127157132dc40808c4ec4037884ddbc4600b2e/net/dns/dns_transaction.cc [modify] https://crrev.com/28127157132dc40808c4ec4037884ddbc4600b2e/net/dns/dns_transaction_unittest.cc
,
Dec 14 2017
,
Feb 28 2018
,
Mar 5 2018
,
Mar 18 2018
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
,
Mar 27 2018
,
Mar 27 2018
|
|||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||
Comment 1 by elawrence@chromium.org
, Dec 7 2017