Closed Bug 785176 Opened 10 years ago Closed 10 years ago

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

Categories

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

defect

Tracking

(firefox19 fixed, firefox20 fixed)

RESOLVED FIXED
Tracking Status
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: bholley, Assigned: AlexLakatos)

References

Details

(Whiteboard: s=121105 u=new c=security p=1)

Attachments

(3 files, 2 obsolete files)

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.
(kind of stabbing in the dark with the CC list here).
?

I don't work in QA anymore. :-)
(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?
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.
QA Contact: anthony.s.hughes
Awesome, thanks :-)
I think enablePrivilege is turned on for Mochitest but not for Reftest.  So you could make a reftest for it.
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.
(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. :-)
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
(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.
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.
(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>
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.
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.
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?
Component: General → Mozmill Tests
QA Contact: anthony.s.hughes
Summary: Do some manual testing to make sure that enablePrivilege is not exposed to the web → Do some testing to make sure that enablePrivilege is not exposed to the web
(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.
Priority: -- → P2
Whiteboard: [mentor=whimboo][lang=js]
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?
Depends on: 788653
(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.
Attached file HTML Testcase (obsolete) —
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
Assignee: nobody → anthony.s.hughes
Status: NEW → ASSIGNED
Attachment #666678 - Flags: feedback?(hskupin)
Attachment #666678 - Flags: feedback?(bobbyholley+bmo)
(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 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. :-)
Attachment #666678 - Flags: feedback?(bobbyholley+bmo) → feedback+
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.
Attachment #666678 - Flags: feedback?(hskupin) → feedback-
(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.
Attached file HTML Testcase
Attachment #667659 - Flags: feedback?(hskupin)
Attachment #666678 - Attachment is obsolete: true
(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 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.
Attachment #667659 - Flags: feedback?(hskupin) → feedback+
Lets get this request in the SV scrum sprint for this week.
Assignee: anthony.s.hughes → nobody
Status: ASSIGNED → NEW
Whiteboard: [mentor=whimboo][lang=js] → s=121105 u=new c=security p=1
Assignee: nobody → alex.lakatos
Status: NEW → ASSIGNED
Attached patch patch v1.0 (obsolete) — Splinter Review
patch adding the testcase(modified) and the test file
Attachment #681504 - Flags: review?(hskupin)
Attachment #681504 - Flags: review?(dave.hunt)
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)
Attachment #681504 - Flags: review?(hskupin)
Attachment #681504 - Flags: review?(dave.hunt)
Attachment #681504 - Flags: review+
Attachment #681504 - Flags: checkin+
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.
Attachment #681504 - Flags: checkin+ → review-
Attached patch patch v2.0Splinter Review
addressed Henrik's review
Attachment #681504 - Attachment is obsolete: true
Attachment #684015 - Flags: review?(hskupin)
Attachment #684015 - Flags: review?(dave.hunt)
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
Attachment #684015 - Flags: review?(hskupin)
Attachment #684015 - Flags: review?(dave.hunt)
Attachment #684015 - Flags: review+
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.
Attachment #685109 - Flags: review?(hskupin)
Attachment #685109 - Flags: review?(dave.hunt)
Comment on attachment 685109 [details]
patch v2.1[beta, release, esr10, esr17]

We do not backport new tests.
Attachment #685109 - Attachment is patch: false
Attachment #685109 - Flags: review?(hskupin)
Attachment #685109 - Flags: review?(dave.hunt)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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)
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.