Closed
Bug 920831
Opened 11 years ago
Closed 11 years ago
MozInputMethod's init should only return undefined
Categories
(Firefox OS Graveyard :: Gaia::Keyboard, defect)
Tracking
(blocking-b2g:koi+, firefox25 unaffected, firefox26 fixed, firefox27 fixed, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed)
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)
(deleted),
patch
|
xyuan
:
review+
|
Details | Diff | Splinter Review |
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...)
Reporter | ||
Comment 1•11 years ago
|
||
(gwagner hit this assertion)
Reporter | ||
Comment 2•11 years ago
|
||
I guess this should be a security bug, though we haven't shipped it.
Group: b2g-core-security
Reporter | ||
Updated•11 years ago
|
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.
Assignee | ||
Comment 4•11 years ago
|
||
Andrew, thank you. It seems easy to fix and I'm taking it;-)
Assignee: nobody → xyuan
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
fix nits.
Attachment #810412 -
Attachment is obsolete: true
Attachment #810412 -
Flags: review?(bzbarsky)
Attachment #810418 -
Flags: review?(bzbarsky)
Comment 7•11 years ago
|
||
In one of my work for 906096 I just returned undefined if there was no permission instead of null. Couldn't that work?
Assignee | ||
Comment 8•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #810418 -
Flags: review?(bzbarsky) → review+
Comment 10•11 years ago
|
||
Thanks :nsm and :yxl for explaining.
Comment 12•11 years ago
|
||
Keywords: checkin-needed
Comment 13•11 years ago
|
||
Backed out for B2G test_interfaces.html orange.
https://hg.mozilla.org/integration/b2g-inbound/rev/0c156ee959d0
https://tbpl.mozilla.org/php/getParsedLog.php?id=28424422&tree=B2g-Inbound
Assignee | ||
Comment 14•11 years ago
|
||
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)
Assignee | ||
Comment 15•11 years ago
|
||
Updated•11 years ago
|
blocking-b2g: koi? → koi+
Updated•11 years ago
|
Attachment #810996 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 16•11 years ago
|
||
Folded the two patches together and rebased on top of bug 906096.
https://hg.mozilla.org/integration/b2g-inbound/rev/51a41b139305
Keywords: checkin-needed
Comment 17•11 years ago
|
||
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
Comment 18•11 years ago
|
||
The Pref bit in the idl needs to go away.
Comment 19•11 years ago
|
||
Comment 20•11 years ago
|
||
Needs more than that. I'll leave it to yxl to sort out. Backed out.
https://hg.mozilla.org/integration/b2g-inbound/rev/ed1bed4fbaab
Reporter | ||
Comment 21•11 years ago
|
||
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.
Assignee | ||
Comment 22•11 years ago
|
||
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.
Assignee | ||
Comment 23•11 years ago
|
||
Attachment #810418 -
Attachment is obsolete: true
Attachment #810996 -
Attachment is obsolete: true
Attachment #811628 -
Flags: review+
Assignee | ||
Comment 24•11 years ago
|
||
Attachment #811628 -
Attachment is obsolete: true
Attachment #811649 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 25•11 years ago
|
||
Keywords: checkin-needed
Comment 26•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-firefox27:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 2 - 10/11
Comment 27•11 years ago
|
||
Reporter | ||
Updated•11 years ago
|
Updated•11 years ago
|
status-b2g18:
--- → wontfix
status-b2g-v1.1hd:
--- → wontfix
Updated•11 years ago
|
Whiteboard: [adv-main26+]
Comment 28•11 years ago
|
||
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)
Reporter | ||
Comment 29•11 years ago
|
||
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.
Reporter | ||
Updated•11 years ago
|
Updated•11 years ago
|
Whiteboard: [adv-main26+] → [adv-main26-]
Assignee | ||
Comment 30•11 years ago
|
||
(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)
Assignee | ||
Comment 31•11 years ago
|
||
(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.
Updated•11 years ago
|
status-firefox-esr24:
--- → unaffected
Updated•11 years ago
|
Group: b2g-core-security → core-security
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•