785176 - Do some testing to make sure that enablePrivilege is not exposed to the web

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Bobby Holley (:bholley)]

In bug 757046, I landed code to turn enablePrivilege off for the web, and hide it behind a pref used in our testing infrastructure. Within our tests (behind the pref), everyone can call enablePrivilege whenever they want. So we can't check in an automated test to quadruple-check that, no, unprivileged code cannot call enablePrivilege.

If the web were able to call it, it would give full system access to arbitrary webpages, so it seems like something we might want to add to our manual checking somewhere along the line.

Comment 1

[Bobby Holley (:bholley)]

(kind of stabbing in the dark with the CC list here).

Comment 2

[Al Billings [:abillings - ex-MoCo]]

?

I don't work in QA anymore. :-)

Comment 3

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #2)
> I don't work in QA anymore. :-)

Any idea who the best contact for a bug like this would be?

Comment 4

[u279076]

Bobby, I will look into this for you. I've just not time to give a serious look at this yet since I'm head-down on releasing Firefox 15. I'll try to get back to you next week.

Comment 5

[Bobby Holley (:bholley)]

Awesome, thanks :-)

Comment 6

[Jesse Ruderman]

I think enablePrivilege is turned on for Mochitest but not for Reftest.  So you could make a reftest for it.

Comment 7

[u279076]

Bobby, can you respond to comment 6? Is this something you can do? From the QA side of things I was thinking your request was for us to create a manual test that we run periodically during Beta, and possibly organize a testday in the near-term.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #7)
> Bobby, can you respond to comment 6? Is this something you can do? From the
> QA side of things I was thinking your request was for us to create a manual
> test that we run periodically during Beta, and possibly organize a testday
> in the near-term.

Hey Anthony,

Jesse's right in comment 6 that we could make a reftest that checks for enablePrivilege. But really, all I'm going for here is just a basic smoke/sanity test that no, web content can't just call enablePrivilege. Given how the code works, it's not something I imagine would break. But it just seemed like a good idea to add it to the final checklist for releases. Before the build goes out the door, it would be good if someone could just quickly check that calling:

netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

Doesn't work, particularly because the |netscape| object is undefined. After toggling the pref in bug 788653, calling enablePrivilege should succeed (but this is verified by our test suite).

This code doesn't vary by platform and doesn't need exhaustive coverage at all - certainly no testdays or anything. Just adding it as a 10-second line-item on litmus tests or something should be fine. :-)

Comment 9

[u279076]

This might be something we can automate in Mozmill. Can you elaborate on the expected results of executing netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");? What is the anticipated output and where can we view it? Where should we execute that code? Is it simply a matter of copying and pasting it into Error Console and checking from some error code?

Th

Comment 10

[Bobby Holley (:bholley)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #9)
> This might be something we can automate in Mozmill. Can you elaborate on the
> expected results of executing
> netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");?
> What is the anticipated output and where can we view it? Where should we
> execute that code? Is it simply a matter of copying and pasting it into
> Error Console and checking from some error code?

Yeah, pretty much, and/or running it in a test web page. Ideally you'd get an error that |nestscape| is undefined. But thanks to bug 791526, we'll probably have to add the |netscape| object back in for compat. So you'll probably end up with netscape.security being undefined.

Comment 11

[u279076]

So you'd run the code within a <script> tag on an HTML page and somehow output the error message to the same page? If so, I think we can do this with an automated test.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #11)
> So you'd run the code within a <script> tag on an HTML page and somehow
> output the error message to the same page? If so, I think we can do this
> with an automated test.

Sure. The issue is that much of our automated testing right now flips a pref that makes that work. Rather than relying on the exact configuration of our various test suites, I thought it might be a good idea to just have someone smoketest this once every 6 weeks. But if that's a burden or doesn't fit into an existing workflow, automating it in a test somewhere is fine.

For reference, the web page looks like this:

<!DOCTYPE html>
<html>
<head>
<script>
try {
  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
  alert('FAIL');
} catch(e) { alert('PASS: ' + e); }
</script>
</head>
<body>
</body>
</html>

Comment 13

[u279076]

Yeah, doing this once every 6 weeks manually is possible, I'm just concerned it might get lost in the churn. Either way, we'll need a test page for this so I'll go ahead and set that up.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So far we haven't had the need to enable this preference for Mozmill. So we could have it as part of our functional testrun. Anthony, please feel free to file a bug for it. Should be simple to implement. We could even highjack this bug.

I cannot guarantee that we wouldn't have to flip the pref in the future but until then I would not go with a manual test here.

Comment 15

[u279076]

Let's just hijack this bug.

Bobby, if I understand your patch from bug 757046 correctly, we want to run the testcase in comment 12 in Mozmill with user_pref("security.enablePrivilege.enable_for_tests", true); or do I have this backwards?

Comment 16

[Bobby Holley (:bholley)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #15)
> Let's just hijack this bug.
> 
> Bobby, if I understand your patch from bug 757046 correctly, we want to run
> the testcase in comment 12 in Mozmill with
> user_pref("security.enablePrivilege.enable_for_tests", true);

No. That's the pref that allows enablePrivilege to be used (to support our test suite). It's only for automatation (and we're changing the pref name anyway in bug 788653). The point here is to make sure that the configuration we ship to users doesn't allow enablePrivilege to be called.

Comment 17

[u279076]

Okay, thanks for the clarification Bobby. So our test could look something like:

1. Verify a pref exists and its default value is correct
2. Load the testcase in comment 12
3. Check for pass/fail in the alert box

Does this sound about right?

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #17)
> 1. Verify a pref exists and its default value is correct

I don't want to do that in Mozmill. If a check like that is necessary it should be done as a Mochitest.

> 2. Load the testcase in comment 12
> 3. Check for pass/fail in the alert box

No, we do not want to raise an alert box but do update some items on the web page, which we can access by id preferable.

Comment 19

[u279076]

Attachment: HTML Testcase

How does this testcase look to the both of you? If it's okay, I'll write up a patch to get it checked in to our litmus-data repository so it can get synced to mozqa.com

Comment 20

[Bobby Holley (:bholley)]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> (In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #17)
> > 1. Verify a pref exists and its default value is correct

The pref doesn't exist in all.js, so you won't see it at all in about:config. This is a good thing :-). No testing needs to be done there, I think.

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 666678 [details]
HTML Testcase

Yep, looks right. Just test it once to make sure it does the right thing with/without the pref. :-)

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 666678 [details]
HTML Testcase

>        netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>        document.write('<p>FAIL</p>');
>      } catch(e) { document.write('<p>PASS: ' + e + '</p>'); }

You don't want to use document.write() but insert this node directly in the body and give it an id as mentioned in a former comment on this bug. Then you can update it's value here with FAIL or PASS. That way we can make use of the testcase in Mozmill.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #19)
> How does this testcase look to the both of you? If it's okay, I'll write up
> a patch to get it checked in to our litmus-data repository so it can get
> synced to mozqa.com

I don't think we need this in the test data repository. This file is for an automated test only and will never be run manually.

Comment 24

[u279076]

Attachment: HTML Testcase

Comment 25

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #23)
> I don't think we need this in the test data repository. This file is for an
> automated test only and will never be run manually.

I uploaded a new version which uses a span ID. Will this work with Mozmill?

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 667659 [details]
HTML Testcase

>      var checkEnablePrivilege = function(result) {

Just use 'function init()' here. Do not pass in result but get the element as the first action inside the function.

>        } catch(e) { result.innerHTML = "PASS"; }

Please move the body of the catch clause to the next line.

>    <span id="result"></span>

Make it an '<p>' element and we are fine.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Lets get this request in the SV scrum sprint for this week.

Comment 28

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

patch adding the testcase(modified) and the test file

Comment 29

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 681504 [details] [diff] [review]
patch v1.0

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/3b34b084d902 (default)

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 681504 [details] [diff] [review]
patch v1.0

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

I will backout the patch because of the reasons you can find inline.

Backout:
http://hg.mozilla.org/qa/mozmill-tests/rev/cc2b1285dec8 (default)

::: data/enable_privilege/enable_privilege.html
@@ +7,5 @@
> +        var result = document.getElementById("result");
> +        try {
> +          netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> +          result.textContent = "FAIL";
> +        } catch(e) {

nit: not a hanging catch please.

::: tests/functional/manifest.ini
@@ +2,5 @@
>  [include:testAddons/manifest.ini]
>  [include:testAwesomeBar/manifest.ini]
>  [include:testBookmarks/manifest.ini]
>  [include:testDownloading/manifest.ini]
> +[include:testEnablePrivilege/manifest.ini]

This is not a component we have ever agreed on to use as a subfolder name. And it's not something we are going to do. It's way better suited in security.

::: tests/functional/testEnablePrivilege/testEnablePrivilege.js
@@ +6,5 @@
> +var { assert } = require("../../../lib/assertions");
> +var tabs = require("../../../lib/tabs");
> +
> +const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../data/");
> +const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + "enable_privilege/enable_privilege.html";

Same here for the testcase location.

Comment 31

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.0

addressed Henrik's review

Comment 32

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 684015 [details] [diff] [review]
patch v2.0

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

Looks good to me. Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/55b54bf06ddd

Comment 33

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.1[beta, release, esr10, esr17]

This is passing on default: http://mozmill-ci.blargon7.com/#/functional/report/674977957b923f4905160d1b9adbcb8f

Let's land it on the rest of the branches.
The existing patch applies cleanly on aurora, and here is the attached patch for beta, release, esr10 and esr17.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 685109 [details]
patch v2.1[beta, release, esr10, esr17]

We do not backport new tests.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Well, given that the feature on bug 788653 landed for Firefox 19 we should land it at least on the aurora branch:

http://hg.mozilla.org/qa/mozmill-tests/rev/6887385e974a (aurora)