Closed Bug 1484910 Opened 2 years ago Closed 1 year ago
Dangerous usage of Checked
Int in mozilla::Widget Query Content Event::Input::Make Offset Absolute
47 bytes, text/x-phabricator-request
|Details | Review|
I'm doing some static analysis to look for dangerous usage of the CheckedInt. The following function, MakeOffsetAbsolute, adds two integers and then assigns them to a CheckedInt. If I understand CheckedInt correctly, there is a chance that an overflow will happen PRIOR to assigning to checked int: CheckedInt<uint32_t> absOffset = mOffset + aInsertionPointOffset; Is there a chance this might overflow PRIOR to being assigned to CheckedInt? As far as I can see, the normal idiom should be something like: CheckedInt<uint32_t> absOffset = CheckedInt<uint32_t>(mOffset) + aInsertionPointOffset; Am I correct here, or is there some C++ type coercion that makes this safe? Now is this _actually_ an exploitable security bug, not as far as I can see. It looks the inputs to this function come from the IME on the system, and I can't see any paths that allow these to be controlled by untrusted content. BUt Im not confident of that as TextEvent is pretty complicated. So Im flagging as security sensitive just in case. (and so as not to disclose this pattern, as there are more of these, see the bug that this blocks)  https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/widget/TextEvents.h#1048  https://searchfox.org/mozilla-central/source/mfbt/CheckedInt.h#485
2 years ago
Component: Source Code Analysis → Widget
Product: Firefox Build System → Core
Yeah, as you said, it comes only from IME or something via native IME event or interface, they must be enough safe. So, it just checks whether coming values make sense or not since we're struggling with odd behavior of a lot of IMEs even in these days. And your suggestion to fix it looks good to me.
We should fix this, but the security risk seems low if the input is only from the IME.
Component: Event Handling → User events and focus handling
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main67-]
You need to log in before you can comment on or make changes to this bug.