Closed Bug 1484910 Opened 2 years ago Closed 1 year ago

Dangerous usage of CheckedInt in mozilla::WidgetQueryContentEvent::Input::MakeOffsetAbsolute

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P2)

63 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: pauljt, Assigned: Alex_Gaynor)

Details

(Keywords: sec-low, Whiteboard: [post-critsmash-triage][adv-main67-])

Attachments

(1 file)

I'm doing some static analysis to look for dangerous usage of the CheckedInt. The following function, MakeOffsetAbsolute[1],  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[2], 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)


[1] https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/widget/TextEvents.h#1048
[2] https://searchfox.org/mozilla-central/source/mfbt/CheckedInt.h#485
Component: Source Code Analysis → Widget
Product: Firefox Build System → Core
Group: core-security → layout-core-security
Component: Widget → Event Handling
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.
Keywords: sec-low
Group: layout-core-security → dom-core-security
Priority: -- → P2
Assignee: nobody → agaynor
Keywords: checkin-needed

Not sure which static analysis comment 0 refers to, but the in-tree one specifically intended to prevent this sort of lapse, apparently does not. I filed bug 1533623 to do so -- currently hidden because who knows if any other uses, more problematic than this one, exist in the tree.

Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Component: Event Handling → User events and focus handling
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main67-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.