1204703 - Make nsContentSecurityManager scriptable and add security checks to nsIChannel implementation in JS

Status: RESOLVED FIXED

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_make_contentsecuritymanager_scriptable.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_toolit_and_b2g.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_test_updates.patch

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8661532 [details] [diff] [review]
bug_1143922_make_contentsecuritymanager_scriptable.patch

Review of attachment 8661532 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed.

::: dom/interfaces/security/nsIContentSecurityManager.idl
@@ +32,5 @@
> +   *     Whether or not the channel should be openend or not.
> +   */
> +   bool performSecurityCheck(in nsIChannel aChannel,
> +                             in nsIStreamListener aStreamListener,
> +                             out nsIStreamListener outStreamListener);

I would say just return the new streamlistener rather than use an out-argument.

You can still throw an exception if the security checks fail.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8661533 [details] [diff] [review]
bug_1143922_toolit_and_b2g.patch

Review of attachment 8661533 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/components/ActivityChannel.jsm
@@ +57,5 @@
>    asyncOpen2: function(aListener) {
> +    var outListener = {};
> +    var allowed = contentSecManager.performSecurityCheck(this, aListener, outListener);
> +    if (!allowed) {
> +      return Cr.NS_ERROR_CONTENT_BLOCKED;

You don't want to return an error code here. You want to throw one.

Returning an error in C++ maps to throwing an exception in JS.

::: toolkit/components/addoncompat/RemoteAddonsChild.jsm
@@ +281,5 @@
> +  asyncOpen2: function(listener) {
> +    var outListener = {};
> +    var allowed = contentSecManager.performSecurityCheck(this, aListener, outListener);
> +    if (!allowed) {
> +      return Cr.NS_ERROR_CONTENT_BLOCKED;

Same here.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8661535 [details] [diff] [review]
bug_1143922_test_updates.patch

Review of attachment 8661535 [details] [diff] [review]:
-----------------------------------------------------------------

Here too

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_make_contentsecuritymanager_scriptable.patch

Agreed, lets return a streamListener instead of using an out-argument. Carrying over r+ from Jonas.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_toolit_and_b2g.patch

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_test_updates.patch

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

These patches landed with the wrong bug number (1143922):
https://hg.mozilla.org/integration/mozilla-inbound/rev/977d5b7ecba3
https://hg.mozilla.org/integration/mozilla-inbound/rev/deda472458fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/309b4d1ab81c




Backed those three out in:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c2a2e73b931f

...and relanded with the right numbers:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5adeb6e823
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2b6f2b470f95
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9f7b7ab7dc1f

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4a5adeb6e823
https://hg.mozilla.org/mozilla-central/rev/2b6f2b470f95
https://hg.mozilla.org/mozilla-central/rev/9f7b7ab7dc1f

Comment 12

[Boris Zbarsky [:bzbarsky]]

>    2.25 +   * whether the channel should be openend or should be blocked consulting

"opened"?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #12)
> >    2.25 +   * whether the channel should be openend or should be blocked consulting
> 
> "opened"?

Indeed, will get that updated. Thanks for pointing it out.