InputScalesValid has a potential buffer overflow |
||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.62 Safari/537.36 Steps to reproduce the problem: I don't have a reproduction case, I just stumbled upon that while reading code. What is the expected behavior? What went wrong? InputScalesValid does: ``` size_t scales_size = static_cast<size_t>(input.size() / sizeof(float)); if (scales_size != expected.size()) return false; std::unique_ptr<float[]> scales(new float[scales_size]); memcpy(scales.get(), input.data(), input.size()); ``` This will write out of bounds if `input.size() % sizeof(float) != 4`. I don't think this is a real issue, but the fix is trivial: ``` if (input.size() != expected.size() * sizeof(float)) return false; size_t scales_size = static_cast<size_t>(input.size() / sizeof(float)); std::unique_ptr<float[]> scales(new float[scales_size]); memcpy(scales.get(), input.data(), scales_size * sizeof(float)); ``` Did this work before? N/A Chrome version: 62.0.3202.62 Channel: stable OS Version: Flash Version:
,
Oct 31 2017
I think you meant this will write OOB if `input.size() % sizeof(float) != 0`, is that right?
If so, I might try to fix it like this:
```
const size_t scales_size = input.size() / sizeof(float);
if (scales_size != expected.size() || input.size() % sizeof(float) != 0) {
return false;
}
```
(Note that the `static_cast<size_t>` should be unnecessary; the type of the expression is already size_t.)
I wonder if there is some way to refactor this to avoid needing to do the allocation and memcpy... a question for another day, I suppose.
So, this would seem to be potential memory corruption in the browser process, which would normally be Critical. But the mitigating factor here is that the target would have to choose to install a maliciously-crafted theme, right? I.e. the attack surface is only accessible after significant user action?
,
Oct 31 2017
I've provided a fix above that avoids the double conditional. > So, this would seem to be potential memory corruption in the browser process, which would normally be Critical. But the mitigating factor here is that the target would have to choose to install a maliciously-crafted theme, right? I.e. the attack surface is only accessible after significant user action? Yes, to be honest I don't think it would be easy to exploit, but it's so easy to fix...
,
Oct 31 2017
We started doing the memcpy() here https://chromiumcodereview.appspot.com/10854074
,
Oct 31 2017
,
Nov 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0aa1e66be68440b6966c8659c677d1a7ef2c200e commit 0aa1e66be68440b6966c8659c677d1a7ef2c200e Author: Peter Kotwicz <pkotwicz@chromium.org> Date: Wed Nov 01 19:57:26 2017 Fix potential buffer overflow in browser_theme_pack.cc This CL fixes a potential buffer overflow in InputScalesValid() BUG= 778251 Change-Id: Iec1dfa3a7758e94cbcdf50006540334fefba451f Reviewed-on: https://chromium-review.googlesource.com/747323 Reviewed-by: Chris Palmer <palmer@chromium.org> Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org> Cr-Commit-Position: refs/heads/master@{#513229} [modify] https://crrev.com/0aa1e66be68440b6966c8659c677d1a7ef2c200e/chrome/browser/themes/browser_theme_pack.cc
,
Nov 3 2017
,
Nov 4 2017
,
Nov 6 2017
,
Nov 6 2017
This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 6 2017
+awhalley@ for M63 merge review
,
Nov 7 2017
govind@ - good for M53
,
Nov 7 2017
Approving merge to M63 branch 3239 based on comment #12. Please merge ASAP so we can take it in for this week Beta release. Thank you.
,
Nov 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7396961b911c6840d6e0a7e4591b6f93626cc406 commit 7396961b911c6840d6e0a7e4591b6f93626cc406 Author: Peter Kotwicz <pkotwicz@chromium.org> Date: Tue Nov 07 18:53:57 2017 Fix potential buffer overflow in browser_theme_pack.cc This CL fixes a potential buffer overflow in InputScalesValid() BUG= 778251 Change-Id: Iec1dfa3a7758e94cbcdf50006540334fefba451f Reviewed-on: https://chromium-review.googlesource.com/747323 Reviewed-by: Chris Palmer <palmer@chromium.org> Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#513229}(cherry picked from commit 0aa1e66be68440b6966c8659c677d1a7ef2c200e) Reviewed-on: https://chromium-review.googlesource.com/757222 Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#409} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/7396961b911c6840d6e0a7e4591b6f93626cc406/chrome/browser/themes/browser_theme_pack.cc
,
Dec 4 2017
,
Feb 9 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
|
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by elawrence@chromium.org
, Oct 25 2017Components: UI>Browser>Themes