Closed Bug 920831 Opened 6 years ago Closed 6 years ago

MozInputMethod's init should only return undefined

Categories

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:koi+, firefox25 unaffected, firefox26 fixed, firefox27 fixed, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed)

RESOLVED FIXED
1.3 Sprint 2 - 10/11
blocking-b2g koi+
Tracking Status
firefox25 --- unaffected
firefox26 --- fixed
firefox27 --- fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed

People

(Reporter: mccr8, Assigned: xyuan)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main26-])

Attachments

(1 file, 4 obsolete files)

As seen in ConstructJSImplementation, the init method for JS-implemented WebIDL should only return undefined, because unlike the XPIDL-based JS-implementation, we ignore the return value of init.  The init for MozInputMethod returns null if it doesn't have permission.

I'm not sure exactly what the right approach is to detect the permission, maybe using a Func attribute like PushManager?
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/PushManager.webidl#7

(Also, we should consider crashing when an init() method doesn't return undefined, if people return other values in the case when permission fails...)
(gwagner hit this assertion)
I guess this should be a security bug, though we haven't shipped it.
Group: b2g-core-security
blocking-b2g: --- → koi?
(In reply to Andrew McCreight [:mccr8] from comment #0)
> As seen in ConstructJSImplementation, the init method for JS-implemented
> WebIDL should only return undefined, because unlike the XPIDL-based
> JS-implementation, we ignore the return value of init.  The init for
> MozInputMethod returns null if it doesn't have permission.
> 
> I'm not sure exactly what the right approach is to detect the permission,
> maybe using a Func attribute like PushManager?
> http://mxr.mozilla.org/mozilla-central/source/dom/webidl/PushManager.webidl#7
> 

Yes, see bug 884897.

Also filed bug 920840.
 Andrew, thank you. It seems easy to fix and I'm taking it;-)
Assignee: nobody → xyuan
Status: NEW → ASSIGNED
Attached patch 0001-navigator.patch (obsolete) — Splinter Review
The patch uses a static method Navigator::HasInputMethod to detect whether to expose MozInputMethod to navigator.

Boris, I guess you're the right person to review the patch, but if you don't have time, feel free to pass on.
Attachment #810412 - Flags: review?(bzbarsky)
Attached patch bug-920831-v1.patch (obsolete) — Splinter Review
fix nits.
Attachment #810412 - Attachment is obsolete: true
Attachment #810412 - Flags: review?(bzbarsky)
Attachment #810418 - Flags: review?(bzbarsky)
In one of my work for 906096 I just returned undefined if there was no permission instead of null. Couldn't that work?
Yes, but partially works. We also need to hide mozInputMethod attribute if there is no permission. Returning undefined won't hide mozInputMethod.
(In reply to Jan Jongboom [:janjongboom] from comment #7)
> In one of my work for 906096 I just returned undefined if there was no
> permission instead of null. Couldn't that work?

Undefined is not used to indicate success/error. Rather think of init() as returning 'void'. Which is why we need the C++ function to check for permission.
Attachment #810418 - Flags: review?(bzbarsky) → review+
Thanks :nsm and :yxl for explaining.
Boris, thank you.
Keywords: checkin-needed
Attached patch Fix test interfaces. (obsolete) — Splinter Review
The permission check prevents test_interfaces.html from accessing MozInputMethod without 'keyboard' permission.

The patch modifies the test_interfaces.html to bypass MozInputMethod.
Attachment #810996 - Flags: review?(bzbarsky)
blocking-b2g: koi? → koi+
Keywords: sec-high
Attachment #810996 - Flags: review?(bzbarsky) → review+
Folded the two patches together and rebased on top of bug 906096.
https://hg.mozilla.org/integration/b2g-inbound/rev/51a41b139305
Keywords: checkin-needed
Yeah, that'll teach me. My rebasing was wrong. Needs someone who knows webidl to do the right thing.
https://hg.mozilla.org/integration/b2g-inbound/rev/5e3d6f3b13a5
The Pref bit in the idl needs to go away.
Blocks: 920840
Needs more than that. I'll leave it to yxl to sort out. Backed out.
https://hg.mozilla.org/integration/b2g-inbound/rev/ed1bed4fbaab
This is the error:
In file included from UnifiedBindings11.cpp:12:
./InputMethodBinding.cpp:1909:21: error: no member named 'HasInputMethodSupport' in 'mozilla::dom::Navigator'
  return Navigator::HasInputMethodSupport(cx, obj);
         ~~~~~~~~~~~^
1 error generated.

yxl: The bustage is in non-B2G builds.  HasInputMethodSupport needs to be defined in non-B2G builds and return false.  Or something like that.
Ryan & Andrew, thanks for your help. Bug 906096enables MozInputMethod on non-B2G build and I should remove the macro '#ifdef MOZ_B2G'. 

I'll provide a rebased patch.
Attached patch rebased. (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=3303e20b3040
Attachment #810418 - Attachment is obsolete: true
Attachment #810996 - Attachment is obsolete: true
Attachment #811628 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6768887622d4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 2 - 10/11
Blocks: 922334
No longer blocks: 922334
Depends on: 922334
No longer depends on: 922334
Whiteboard: [adv-main26+]
Is this b2g only? What are the security implications of this bug?

This didn't go through sec-approval so I'm a bit in the dark on it. See https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: needinfo?(xyuan)
Looking over this some more, I think the flags are wrong.  The regressing bug, bug 899073, only landed in Firefox 26, and it was fixed everywhere, so we never shipped it.
Whiteboard: [adv-main26+] → [adv-main26-]
(In reply to Al Billings [:abillings] from comment #28)
> Is this b2g only? What are the security implications of this bug?
It is b2g only. Without fixing this bug, the b2g app can use MozInputMethod
Flags: needinfo?(xyuan)
(In reply to Al Billings [:abillings] from comment #28)
> Is this b2g only? What are the security implications of this bug?
It is b2g only. Without fixing this bug, the b2g app can use MozInputMethod API even if its permission check fails.
Group: b2g-core-security → core-security
Group: core-security
You need to log in before you can comment on or make changes to this bug.