1169653 - Enable SpeechRecognition and relevant APIs first only on PrivilegedApps or CertifiedApps apps with some certain permission

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

I think we should expose SpeechRecognition first only on PrivilegedApps or CertifiedApps on FirefoxOS. That way we'd get some testing for the API implementation before exposing it to the web, and would be also safer from security point of view, since no random 
JS would use the APIs.

So, the relevant webidl interfaces would probably need Func="SomeMethodName" and that
method would check that media.webspeech.recognition.enable is true and 
either we have the right value for exposed or we have some other pref, say
media.webspeech.recognition.forceEnable true. That latter pref is for the case
one wants to test the API on desktop without PrivilegedApps or CertifiedApps.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

anatal, kdavis, could either of you take this bug?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.h?rev=93320d4629be&mark=2953-2957,2960-2964#2952 would be useful for this

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Or use some permission at least, if we don't want to limit to PrivilegedApps or CertifiedApps.
Just to expose the API first to apps, and once we have some feedback about how the implementation works, expose to the web.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

and about WebIDL's Func, https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Func

Comment 5

[kdavis]

Olli why does this block Bug 1162507 and Bug 1167938?

Both Bug 1162507 and Bug 1167938 can be done with or without this bug being completed.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Because if we want to expose Web Speech API to the web (without much fuzzing or anything), we'll need to do much more throughout security review. But if we focus first on only certain apps and yet let fuzzers to also work on (locally or tryserver built) Nightlies when some pref is set, this all becomes
easier to handle from security point of view.


(And don't take bugzilla 'blocks' or 'depends on' to literally.)

Comment 7

[kdavis]

(In reply to Olli Pettay [:smaug] from comment #6)
> 
> (And don't take bugzilla 'blocks' or 'depends on' to literally.)

As long as someone is continuing on the security reviews and not waiting on this bug,
I don't care if 'blocks' or 'depends on' is being used in a non-standard manner.

I just want to make sure that the security reviews are not waiting on this bug, as
they really do not need to wait.

Comment 8

[kdavis]

Oh, by the way, is there some means of determining if a method is being
called while a try, or more generally a test suite, is running? 

This is needed as the test suite is not privileged or certified.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

You can always check the logs to see that a certain test is run.


And for the security review part, sounds like we (well, I) will just do a rather high level auditing, and then rely on this bug to limit API to be exposed only on certain apps (unless some pref is set) and do fuzzing.
So I guess more correct would be to mark this bug to block the bug which adds support for
the pocketsphinx based Web Speech API backend.

Comment 10

[kdavis]

(In reply to Olli Pettay [:smaug] from comment #9)
> You can always check the logs to see that a certain test is run.

This is not sufficient for what I am thinking.

Basically, what I have in mind is to have Func="SomeMethodName" on the proper place(s)
of the webidl, then the implementation of SomeMethodName to be something like...

bool
SomeMethodName(JSContext* aCx, JSObject* aGlobal)
{
  return IsInPrivilegedApp(aCx, aGlobal) || IsInCertifiedApp(aCx, aGlobal) || IsInTest(aCx, aGlobal);
}

As far as I know, there is no function IsInTest.

Having the code as above allows the tests to run even though they are neither privileged
nor certified.
 
> And for the security review part, sounds like we (well, I) will just do a
> rather high level auditing, and then rely on this bug to limit API to be
> exposed only on certain apps (unless some pref is set) and do fuzzing.
> So I guess more correct would be to mark this bug to block the bug which
> adds support for the pocketsphinx based Web Speech API backend.

Ok

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Oh, you mean that. There shouldn't be IsInTest, but a pref check, and then the pref is enabled while running the tests.
(the same pref would be set also when one does some fuzz testing)

Comment 12

[kdavis]

I ended up doing

bool
SpeechRecognition::IsAuthorized(JSContext* aCx, JSObject* aGlobal)
{
  bool enableTests = Preferences::GetBool(TEST_PREFERENCE_ENABLE);
  return IsInPrivilegedApp(aCx, aGlobal) || IsInCertifiedApp(aCx, aGlobal) || enableTests;
}

I'm running the try now.

Comment 13

[kdavis]

Attachment: Part 1 of 1: Patch limits use of SpeechRecognition::start in JS to privileged or certified apps

Part 1 of 1:  Patch limits use of SpeechRecognition::start in JS to privileged or certified apps

This is the part 1 of 1 for this bug. 

This patch limits use of SpeechRecognition::start in JS to privileged or certified apps.

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=59e88d5908a9

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8613870 [details] [diff] [review]
Part 1 of 1:  Patch limits use of SpeechRecognition::start in JS to privileged or certified apps

The webidl part looks wrong.  We don't want to expose the rest of the speech API to web pages, but not start().
Move the check to interface itself, and also relevant speech recognition related events and such need the same check.

And looks like IsInPrivilegedApp checks also for certified app
http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.cpp#2384
But do we also want to have a permission for the app, as per the email from dveditz. Many b2g APIs have permission + availableIn checks.

Comment 15

[kdavis]

(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 8613870 [details] [diff] [review]
> Part 1 of 1:  Patch limits use of SpeechRecognition::start in JS to
> privileged or certified apps
> 
> The webidl part looks wrong.  We don't want to expose the rest of the speech
> API to web pages, but not start().

Why? They can't do speech recognition without start(). So this seems like the
minimal change.

> Move the check to interface itself and also relevant speech recognition
> related events and such need the same check.

This will cause failures in dom/tests/mochitest/general/test_interfaces.html
What is the standard way to deal with these in this case?

> And looks like IsInPrivilegedApp checks also for certified app
> http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.
> cpp#2384

Ok. But that's an implementation detail that may or may not continue to
be true. We should not rely upon that.

> But do we also want to have a permission for the app, as per the email from
> dveditz.

Could you capture the relevant part of that conversation of that email in a comment
to this bug? (If for no other reason than posterity.) As I wasn't privy all of the
conversation, I am not sure what you mean here.

Also, what is "the app"?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to kdavis from comment #15)
> (In reply to Olli Pettay [:smaug] from comment #14)
> > Comment on attachment 8613870 [details] [diff] [review]
> > Part 1 of 1:  Patch limits use of SpeechRecognition::start in JS to
> > privileged or certified apps
> > 
> > The webidl part looks wrong.  We don't want to expose the rest of the speech
> > API to web pages, but not start().
> 
> Why? They can't do speech recognition without start(). So this seems like the
> minimal change.
Because web pages do feature testing based on interfaces, like
if ("SpeechRecognition" in window) { ....} 
So, SpeechRecognition shouldn't be in the global scope if the API is disabled.


> This will cause failures in dom/tests/mochitest/general/test_interfaces.html
> What is the standard way to deal with these in this case?
Ah, good,  test_interfaces.html captures then a bug in your patch. We should not be exposing the interface in global scope unless
"media.webspeech.recognition.enable" is true.
Why don't you just make the check like I suggested in comment 0?


> Could you capture the relevant part of that conversation of that email in a
> comment
> to this bug?
"Unless there's an actual permission restricting the use of web speech (such that marketplace reviews would restrict its use to appropriate apps) then allowing all privileged apps to use an API isn't a whole lot of protection. Tons safer than exposing it directly to the web, maybe not much safer than exposing it to all apps."



> Also, what is "the app"?
Some FFOS application which tries to use Web Speech API.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

If we're going to use speech recognition first only in very builtin apps only, then to check certified apps only should be enough (I mean, no need for permission + privileged check, pref check should be still there).

Comment 18

[kdavis]

(In reply to Olli Pettay [:smaug] from comment #16)
> (In reply to kdavis from comment #15)
> > (In reply to Olli Pettay [:smaug] from comment #14)
> > > Comment on attachment 8613870 [details] [diff] [review]
> > > Part 1 of 1:  Patch limits use of SpeechRecognition::start in JS to
> > > privileged or certified apps
> > > 
> > > The webidl part looks wrong.  We don't want to expose the rest of the speech
> > > API to web pages, but not start().
> > 
> > Why? They can't do speech recognition without start(). So this seems like the
> > minimal change.
> Because web pages do feature testing based on interfaces, like
> if ("SpeechRecognition" in window) { ....} 
> So, SpeechRecognition shouldn't be in the global scope if the API is
> disabled.

Good point.

Comment 19

[kdavis]

(In reply to Olli Pettay [:smaug] from comment #16)
> (In reply to kdavis from comment #15)
> > (In reply to Olli Pettay [:smaug] from comment #14)
> > > Comment on attachment 8613870 [details] [diff] [review]
> >
> > This will cause failures in dom/tests/mochitest/general/test_interfaces.html
> > What is the standard way to deal with these in this case?
> Ah, good,  test_interfaces.html captures then a bug in your patch. We should
> not be exposing the interface in global scope unless
> "media.webspeech.recognition.enable" is true.

media.webspeech.recognition.enable, IsInPrivilegedApp(), and IsInCertifiedApp()
are three independent booleans.

So, for example media.webspeech.recognition.enable can be true while IsInPrivilegedApp()
and IsInCertifiedApp() are false, as is the case for example when the tests of
test_interfaces.html are run.

This being the case, I don't see how your suggestion helps. The tests will still fail.

Comment 20

[kdavis]

(In reply to Olli Pettay [:smaug] from comment #16)
> (In reply to kdavis from comment #15)
> > (In reply to Olli Pettay [:smaug] from comment #14)
> Why don't you just make the check like I suggested in comment 0?

In Comment 0 you state

>I think we should expose SpeechRecognition first only on PrivilegedApps or CertifiedApps
>on FirefoxOS. That way we'd get some testing for the API implementation before exposing
>it to the web, and would be also safer from security point of view, since no random 
>JS would use the APIs.
> 
>So, the relevant webidl interfaces would probably need Func="SomeMethodName" and that
>method would check that media.webspeech.recognition.enable is true and 
>either we have the right value for exposed or we have some other pref, say
>media.webspeech.recognition.forceEnable true. That latter pref is for the case
>one wants to test the API on desktop without PrivilegedApps or CertifiedApps.

From my understanding of this comment you want to check in "SomeMethodName", schematically,
something like:

IsInPrivilegedApp(...) || IsInCertifiedApp(...) || media.webspeech.recognition.enable;

Assuming this is what you want to check, my first question is: Why check 
media.webspeech.recognition.enable?

The various webidl interfaces are already protected with:

[Constructor,
 Pref="media.webspeech.recognition.enable"]
interface SpeechRecognition : EventTarget {
...
}

So, adding in a media.webspeech.recognition.enable check in the
function "SomeMethodName" serves no purpose I can see.

The other alternative you suggest is to introduce a new flag
media.webspeech.recognition.forceEnable for testing the API
on the desktop.

As we are currently only focusing on B2G, I don't think we should
worry about desktop yet.

So, in summary, that's why I didn't use media.webspeech.recognition.enable
or media.webspeech.recognition.forceEnable in the method "SomeMethodName"
and only used IsInPrivilegedApp and IsInCertifiedApp

Comment 21

[kdavis]

(In reply to Olli Pettay [:smaug] from comment #16)
> (In reply to kdavis from comment #15)
> > (In reply to Olli Pettay [:smaug] from comment #14)
> > 
> > Could you capture the relevant part of that conversation of that email in a
> > comment
> > to this bug?
> "Unless there's an actual permission restricting the use of web speech (such
> that marketplace reviews would restrict its use to appropriate apps) then
> allowing all privileged apps to use an API isn't a whole lot of protection.
> Tons safer than exposing it directly to the web, maybe not much safer than
> exposing it to all apps."

I think this sounds like a reasonable suggestion, but it seems out of scope
for this bug which has a much more tight focus.

This suggestion implies some speech app recognition review structure in the
marketplace, some entity that indicates if an app is properly using the Web
Speech API, some document indicating how the Web Speech API is properly used,
modifications to nsIPrincipal and associated code, and, I am sure, other things
I can't think of right now.

This task seems like a large task and would require approval from more senior
people before we can proceed. In addition, it would require its own meta bug
and associated child bugs with subtasks.

Comment 22

[kdavis]

(In reply to Olli Pettay [:smaug] from comment #16)
> (In reply to kdavis from comment #15)
> > (In reply to Olli Pettay [:smaug] from comment #14)
> > 
> > Also, what is "the app"?
> Some FFOS application which tries to use Web Speech API.

The "the" just threw me. I thought you were talking about a specific
app like "Vaani" say not a generic app.

Comment 23

[kdavis]

(In reply to Olli Pettay [:smaug] from comment #17)
> If we're going to use speech recognition first only in very builtin apps
> only, then to check certified apps only should be enough 

Sounds fine by me.

Comment 24

[kdavis]

(In reply to Olli Pettay [:smaug] from comment #17)
> (I mean, no need for permission + privileged check, pref check should be still there).

For the reasons I give in Comment 20 I don't the the pref check for media.webspeech.recognition.enable
or media.webspeech.recognition.forceEnable should be there.

Comment 25

[kdavis]

(In reply to Olli Pettay [:smaug] from comment #16)
> (In reply to kdavis from comment #15)
> > (In reply to Olli Pettay [:smaug] from comment #14)
> > > Comment on attachment 8613870 [details] [diff] [review]
> 
> > We should not be exposing the interface in global scope unless
> > "media.webspeech.recognition.enable" is true.

The interfaces are only being exposed in global scope when "media.webspeech.recognition.enable"
is true.

All the various speech recognition webidl interfaces (SpeechGrammar, SpeechGrammarList...) have

[Constructor,
 Pref="media.webspeech.recognition.enable"]

on their webidl interface and are thus only exposed in global scope when 
"media.webspeech.recognition.enable" is true.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

So, I was thinking effectively

(media.webspeech.recognition.enable == true) &&
 (media.webspeech.recognition.forceEnable == true ||
  certifiedApp == true)


That way for fuzzing on desktop one could just enable media.webspeech.recognition.enable and
media.webspeech.recognition.forceEnable

and on b2g media.webspeech.recognition.enable would be enough to expose this stuff to certified apps.


(I'd prefer to keep media.webspeech.recognition.enable as a global way to enable/disable this stuff, so that we can disable the API easily if something really bad is found during early testing.)

Comment 27

[kdavis]

(In reply to Olli Pettay [:smaug] from comment #26)
> So, I was thinking effectively
> 
> (media.webspeech.recognition.enable == true) &&
>  (media.webspeech.recognition.forceEnable == true ||
>   certifiedApp == true)
> 
> 
> That way for fuzzing on desktop one could just enable
> media.webspeech.recognition.enable and
> media.webspeech.recognition.forceEnable

I think, as the team agreed on not supporting desktop in this initial release,
that this should be curtailed to 

(media.webspeech.recognition.enable == true) && (certifiedApp == true)

after Whistler we can add in the second flag.

However, I don't think this addresses my initial concern.

This will cause failures in dom/tests/mochitest/general/test_interfaces.html
as in this HTML, the value of media.webspeech.recognition.enable is true but
certifiedApp is false.

Hence, for example, SpeechRecognition will not exist globally but the tests
in test_interfaces.html have no way to know about this as they can not turn
off if certifiedApp is false.

> (I'd prefer to keep media.webspeech.recognition.enable as a global way to
> enable/disable this stuff, so that we can disable the API easily if
> something really bad is found during early testing.)

Ok. I think there is agreement on this.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

(In reply to kdavis from comment #27)
> I think, as the team agreed on not supporting desktop in this initial
> release,
by default. But for fuzzing we really need to be able to enable the speech API on locally or try-server built nightlies.
"not supporting" doesn't mean, "not possible to use even if built locally".
And dveditz requested
"And again if we could build it as part of Firefox rather than b2g it would fit our fuzzing harnesses better. "



> after Whistler we can add in the second flag.
we need to be able to start fuzzing the implementation asap. It doesn't block
security review (since that won't be really security review but very high level auditing),
but we need to start testing the API and its implementation for real world use, and that
means also fuzz testing.


> This will cause failures in dom/tests/mochitest/general/test_interfaces.html
> as in this HTML, the value of media.webspeech.recognition.enable is true but
> certifiedApp is false.
> 
> Hence, for example, SpeechRecognition will not exist globally but the tests
> in test_interfaces.html have no way to know about this as they can not turn
> off if certifiedApp is false.
Don't put SpeechRecognition to the test_interfaces.html then.

Comment 29

[kdavis]

I removed Bug 1162507 and Bug 1167938 from being blockers, sorry Olli, as:

1. They are not really blockers
2. If they are listed as blockers, they cause a circular dependency when a real dependency Bug 1051148 is added

Comment 30

[kdavis]

Attachment: Part 1 of 1: Patch limits use of the speech recognition API in JS to certified apps or apps with the proper flags set

Part 1 of 1:  Patch limits use of the speech recognition API in JS to certified apps or apps with the proper flags set

This is the part 1 of 1 for this bug. 

This patch limits use of WebSpeech API speech recognition interfaces
in JS to code in which the following returns true

(IsInCertifiedApp(...) || media.webspeech.recognition.force_enable || media.webspeech.test.enable) && media.webspeech.recognition.enable

where the default value for any unspecified preference is false.

In addition this patch removes all WebSpeech API speech recognition
interfaces, described here

https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html#speechreco-section

from the interface tests in the file

dom/tests/mochitest/general/test_interfaces.html

as it is currently not possible for them to be included there
and be subject to the above limitation.

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=24296b0c680a

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdaede62fdcf

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Backed out for introducing new rooting hazards.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf40676095aa

Function '_ZN7mozilla3dom17SpeechRecognition12IsAuthorizedEP9JSContextP8JSObject|uint8 mozilla::dom::SpeechRecognition::IsAuthorized(JSContext*, JSObject*)' has unrooted 'aGlobal' of type 'JSObject*' live across GC call '_ZN7mozilla11Preferences7GetBoolEPKcb|uint8 mozilla::Preferences::GetBool(int8*, uint8)' at dom/media/webspeech/recognition/SpeechRecognition.cpp:152
    dom/media/webspeech/recognition/SpeechRecognition.cpp:150: Call(1,2, enableTests := GetBool("media.webspeech.test.enable",0))
    dom/media/webspeech/recognition/SpeechRecognition.cpp:151: Call(2,3, enableRecognitionEnable := GetBool("media.webspeech.recognition.enable",0))
    dom/media/webspeech/recognition/SpeechRecognition.cpp:152: Call(3,4, enableRecognitionForceEnable := GetBool("media.webspeech.recognition.force_enable",0)) [[GC call]]
    dom/media/webspeech/recognition/SpeechRecognition.cpp:153: Call(4,5, __temp_1 := IsInCertifiedApp(aCx*,aGlobal*))
GC Function: _ZN7mozilla11Preferences7GetBoolEPKcb|uint8 mozilla::Preferences::GetBool(int8*, uint8)
    uint32 mozilla::Preferences::GetBool(int8*, uint8*)
    PREF_GetBoolPref
    PrefHashEntry* pref_HashTableLookup(void*)
    PLDHashEntryHdr* PL_DHashTableSearch(PLDHashTable*, void*)
    PLDHashEntryHdr* PLDHashTable::Search(void*)
    PLDHashEntryHdr* PLDHashTable::SearchTable(void*, uint32) [with PLDHashTable::SearchReason Reason = (PLDHashTable::SearchReason)0u; PLDHashNumber = unsigned int]
    IndirectCall: matchEntry

Comment 33

[kdavis]

Attachment: Patch limits use of the speech recognition API in JS to certified apps or apps with the proper flags set

Part 1 of 1:  Patch limits use of the speech recognition API in JS to certified apps or apps with the proper flags set

This is the part 1 of 1 for this bug. 

This patch has the exact same functionality as the patch it replaces. However,
it fixes a problem with an unrooted JSObject representing the global object.

This fix is seen in the interdiff between the two patches

diff -u b/dom/media/webspeech/recognition/SpeechRecognition.cpp b/dom/media/webspeech/recognition/SpeechRecognition.cpp
--- b/dom/media/webspeech/recognition/SpeechRecognition.cpp
+++ b/dom/media/webspeech/recognition/SpeechRecognition.cpp
@@ -147,10 +147,11 @@
 bool
 SpeechRecognition::IsAuthorized(JSContext* aCx, JSObject* aGlobal)
 {
+  bool inCertifiedApp = IsInCertifiedApp(aCx, aGlobal);
   bool enableTests = Preferences::GetBool(TEST_PREFERENCE_ENABLE);
   bool enableRecognitionEnable = Preferences::GetBool(TEST_PREFERENCE_RECOGNITION_ENABLE);
   bool enableRecognitionForceEnable = Preferences::GetBool(TEST_PREFERENCE_RECOGNITION_FORCE_ENABLE);
-  return (IsInCertifiedApp(aCx, aGlobal) || enableRecognitionForceEnable || enableTests) && enableRecognitionEnable;
+  return (inCertifiedApp || enableRecognitionForceEnable || enableTests) && enableRecognitionEnable;
 }
 
 already_AddRefed<SpeechRecognition>

The problem was that the global JSObject* aGlobal could have been garbage 
collected as a result of call(s) to Preferences::GetBool(...) and if that
were to happen, then as a result of the manner in which Spidermonkey GC's
the pointer aGlobal would then become invalid.

The solution to this problem is to use aGlobal immediately before any call(s)
to Preferences::GetBool(...) thus not allowing GC to invalidate aGlobal.

The corresponding unit test for this GC problem is "H" which succeeds in the 
new try.

As a result of the problem with this commit, Andrew McCreight opened a new
bug Bug 1173965 to "Handlify PropertyEnabled" in order to prevent this GC 
problem from being triggered over calls to Preferences::GetBool(...)

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=c18fb1eccef7

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8621538 [details] [diff] [review]
Patch limits use of the speech recognition API in JS to certified apps or apps with the proper flags set

Sounds like rooting analyzer is a bit too eager.
But this is anyhow good.

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/417b50770873

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/417b50770873