Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834893002:
Ensure ClipboardWin::ReadHTML does not crash on embedded nulls (Closed)
DescriptionEnsure ClipboardWin::ReadHTML does not crash on embedded nulls
If the CF_HTML content on the clipboard contains a null byte, a 64-bit
browser process would crash when attempting to perform a checked_cast of
0xFFFFFFFFFFFFFFFF (string::npos) to uint32_t.
This change avoids the crash by ensuring that the offset remains within
the length of the data.
BUG=607181
Review-Url: https://codereview.chromium.org/2834893002
Cr-Commit-Position: refs/heads/master@{#467350}
Committed: https://chromium.googlesource.com/chromium/src/+/d2338b606d6536d20975a601c197d3e2ee3cec0e
Patch Set 1 #
Total comments: 4
Patch Set 2 : Ensure start_fragment is appropriate #
Total comments: 2
Patch Set 3 : Fix off-by-one #
Total comments: 2
Patch Set 4 : Simplify #Patch Set 5 : Be smarter #Messages
Total messages: 18 (8 generated)
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
elawrence@chromium.org changed reviewers: + dcheng@chromium.org
PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2834893002/diff/1/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_win.cc (right): https://codereview.chromium.org/2834893002/diff/1/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_win.cc:575: base::checked_cast<uint32_t>(std::min(offsets[1], markup->length())); Should we do this for offsets[0] as well?
https://codereview.chromium.org/2834893002/diff/1/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_win.cc (right): https://codereview.chromium.org/2834893002/diff/1/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_win.cc:575: base::checked_cast<uint32_t>(std::min(offsets[1], markup->length())); On 2017/04/22 08:25:25, dcheng (OOO through May 2) wrote: > Should we do this for offsets[0] as well? Embedded nulls (the known repro case) aren't problematic for the fragment_start value, but in the case of a very malformed CF_HTML, it's certainly possible that the StartFragment index on the clipboard points outside of GlobalSize(data)/EndHTML. To address that, I'll ensure final fragment_start value is <= fragment_end.
https://codereview.chromium.org/2834893002/diff/1/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_win.cc (right): https://codereview.chromium.org/2834893002/diff/1/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_win.cc:575: base::checked_cast<uint32_t>(std::min(offsets[1], markup->length())); On 2017/04/25 16:20:19, elawrence wrote: > On 2017/04/22 08:25:25, dcheng (OOO through May 2) wrote: > > Should we do this for offsets[0] as well? > > Embedded nulls (the known repro case) aren't problematic for the fragment_start > value, but in the case of a very malformed CF_HTML, it's certainly possible that > the StartFragment index on the clipboard points outside of > GlobalSize(data)/EndHTML. > > To address that, I'll ensure final fragment_start value is <= fragment_end. Why can't embedded nulls appear before fragment start? https://codereview.chromium.org/2834893002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_win.cc (right): https://codereview.chromium.org/2834893002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_win.cc:574: markup_end--; Why do we need to adjust this? Microsoft's examples seem to be unclear and incomplete, but https://cs.chromium.org/chromium/src/ui/base/clipboard/clipboard_util_win.cc?... seems to imply that start/end indexes might work like we would expect them to.
https://codereview.chromium.org/2834893002/diff/1/ui/base/clipboard/clipboard... File ui/base/clipboard/clipboard_win.cc (right): https://codereview.chromium.org/2834893002/diff/1/ui/base/clipboard/clipboard... ui/base/clipboard/clipboard_win.cc:575: base::checked_cast<uint32_t>(std::min(offsets[1], markup->length())); On 2017/04/26 00:42:08, dcheng (OOO through May 2) wrote: > On 2017/04/25 16:20:19, elawrence wrote: > > On 2017/04/22 08:25:25, dcheng (OOO through May 2) wrote: > > > Should we do this for offsets[0] as well? > > > > Embedded nulls (the known repro case) aren't problematic for the > fragment_start > > value, but in the case of a very malformed CF_HTML, it's certainly possible > that > > the StartFragment index on the clipboard points outside of > > GlobalSize(data)/EndHTML. > > > > To address that, I'll ensure final fragment_start value is <= fragment_end. > > Why can't embedded nulls appear before fragment start? Ah, you're right in the general case. In the case of the client I've been using, the fragment always contains the entire markup. https://codereview.chromium.org/2834893002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_win.cc (right): https://codereview.chromium.org/2834893002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_win.cc:574: markup_end--; On 2017/04/26 00:42:08, dcheng (OOO through May 2) wrote: > Why do we need to adjust this? Microsoft's examples seem to be unclear and > incomplete, but > https://cs.chromium.org/chromium/src/ui/base/clipboard/clipboard_util_win.cc?... > seems to imply that start/end indexes might work like we would expect them to. Indeed, testing indicates that the indexes do work like you've described, although this isn't what I expected. :)
lgtm https://codereview.chromium.org/2834893002/diff/40001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_win.cc (right): https://codereview.chromium.org/2834893002/diff/40001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_win.cc:577: base::checked_cast<uint32_t>(std::min(offsets[0], markup_end))); Nit: I think that if one were to be clever, that this could be shortened to: *fragment_start = base::checked_cast<uint32_t>(std::min(offsets[0], *fragment_end)); As fragment_end is <= markup_end by definition.
https://codereview.chromium.org/2834893002/diff/40001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_win.cc (right): https://codereview.chromium.org/2834893002/diff/40001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_win.cc:577: base::checked_cast<uint32_t>(std::min(offsets[0], markup_end))); On 2017/04/26 15:19:10, dcheng (OOO through May 2) wrote: > Nit: I think that if one were to be clever, that this could be shortened to: > > *fragment_start = > base::checked_cast<uint32_t>(std::min(offsets[0], *fragment_end)); > > As fragment_end is <= markup_end by definition. Yeah, I'd had it that way originally, but clang won't compile it because "could not deduce template argument". It requires a cast so the second argument is the same type as the first, so I've made that change.
still lgtm
The CQ bit was checked by elawrence@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493221630731210,
"parent_rev": "dabbe072b786618bbc557381a0c535596c698822", "commit_rev":
"d2338b606d6536d20975a601c197d3e2ee3cec0e"}
Message was sent while issue was closed.
Description was changed from ========== Ensure ClipboardWin::ReadHTML does not crash on embedded nulls If the CF_HTML content on the clipboard contains a null byte, a 64-bit browser process would crash when attempting to perform a checked_cast of 0xFFFFFFFFFFFFFFFF (string::npos) to uint32_t. This change avoids the crash by ensuring that the offset remains within the length of the data. BUG=607181 ========== to ========== Ensure ClipboardWin::ReadHTML does not crash on embedded nulls If the CF_HTML content on the clipboard contains a null byte, a 64-bit browser process would crash when attempting to perform a checked_cast of 0xFFFFFFFFFFFFFFFF (string::npos) to uint32_t. This change avoids the crash by ensuring that the offset remains within the length of the data. BUG=607181 Review-Url: https://codereview.chromium.org/2834893002 Cr-Commit-Position: refs/heads/master@{#467350} Committed: https://chromium.googlesource.com/chromium/src/+/d2338b606d6536d20975a601c197... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/d2338b606d6536d20975a601c197... |
