Closed Bug 927901 Opened 6 years ago Closed 6 years ago

Test has failed: Permission denied to access property 'encrypt'

Categories

(Core :: Security, defect)

25 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox24 --- unaffected
firefox25 + verified
firefox26 + verified
firefox27 + verified
firefox-esr24 --- unaffected

People

(Reporter: alice0775, Assigned: smaug)

References

Details

(Keywords: regression)

Attachments

(2 files)

Steps To Reproduce:
1. Open http://html5test.com/
2. Click "Try the next version of HTML5test - New tests, new scores, coming soon!" link in orange bar at the top of page

Actual Results:
An alart box pops up.
Test has failed: Permission denied to access property 'encrypt'

Expected Results:
Test results should be shown on the page

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/54434a926c5f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 ID:20130805022018
Bad:
http://hg.mozilla.org/mozilla-central/rev/ad0ae007aa9e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 ID:20130805022418
Pushlog[:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=54434a926c5f&tochange=ad0ae007aa9e

In local build
Last Good: 81fada98a394
First Bad: 8aafb671f1e9
Regressed by:
	8aafb671f1e9	Yoshi Huang — Bug 883741 - Part 1: WebCrypto: Move Crypto to WebIDL. r=bz
s/An alart box/An alert box/
Yoshi, any idea of the criticality of this issue for users? Difficult to say from these STR. We're less than a week from release.
Flags: needinfo?(allstars.chh)
I will check this bug first.
But I am not sure the criticality here.
Assignee: nobody → allstars.chh
Flags: needinfo?(allstars.chh)
Last Friday I did see the error message "Test has failed: Permission denied to access property 'encrypt'",
but I just updated my m-c, which is 
commit a86900b72cfe2ce13c126c38c02f0e4a19a9b771
Author: Matt Brubeck <mbrubeck@mozilla.com>
Date:   Tue Oct 22 16:35:56 2013 -0700

    Back out 1ae5d58532ad (bug 928370) for breaking Tp5 measurement

and seems it's fixed now.

Hi  Alice0775 White:
Can you check this again?

Thanks
Flags: needinfo?(alice0775)
I can still reproduce the problem

http://hg.mozilla.org/mozilla-central/rev/21d97baadc05
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131022195201
Flags: needinfo?(alice0775)
This issue is also tracked by html5test.com here: https://github.com/NielsLeenheer/html5test/issues/293
Sorry, Alice0775, I tried again and it still exists.
Not sure what went wrong yesterday.
Sorry for the noise.

After some investigation, I found something,

- In beta.html5test.com
The erroe comes from testSecurity() in beta.html5test.com/engine.js
And the test is just 

var crypto = window.crypto || ...;
... 
!!crypto && 'encrypt' in crypto

At this time,
all property access to window.crypto are failed with the errorMsg
'Permission denied to access property' ...

It starts from 

http://mxr.mozilla.org/mozilla-central/source/js/src/jsproxy.h#407

The policy has been to true, so the handler->enter will go to 
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/FilteringWrapper.cpp#164

The check in line 179 XrayUtils::IsXrayResolving will return false
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/FilteringWrapper.cpp#179

Now will call the Policy::check in line 183,
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/FilteringWrapper.cpp#183

The check is in 
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AccessCheck.h#75
which is a CrossOriginAccessiblePropertyOnly policy

Then the check will return false in 
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AccessCheck.cpp#257

I am not sure what went wrong exactly.

CCing smaug and khuey here for help (bz is still in PTO)

Hi smaug and khuey
Can you kindly help to give me some information or some hints here?

Thank you :)
So, erm, nsGlobalWindow::GetCrypto should forward to inner window, as far as I see.
/me tests
Attached patch patchSplinter Review
Assignee: allstars.chh → bugs
Attachment #821663 - Flags: review?(khuey)
Yoshi, we need this at least for Aurora, but Beta too?
It is not clear to me on which branches we have bug 883741. It says mozilla26, but
the regression range in this bug talks about Firefox/25.0.

Could you please ask approval on the right branches.
Flags: needinfo?(allstars.chh)
I think it's on 25 and philor just set the wrong milestone there ...
Flags: needinfo?(philringnalda)
It's probably too late to get this into 25 but if we can take it as a ridealong in a respin we should.
You would need to ask either hg, or mcMerge since it was the one setting milestones.
Flags: needinfo?(philringnalda)
Is this test calling bareword "crypto" on a window that's been navigated away from (to a different origin, at that) or something?  If not, why is the forwarding direction relevant?

Or is the issue that there's a stale window.crypto on the outer from some other (different origin) page and we're getting it?
(In reply to On vacation Oct 12 - Oct 27 from comment #16)
> Or is the issue that there's a stale window.crypto on the outer from some
> other (different origin) page and we're getting it?

The latter, AIUI.
Comment on attachment 821663 [details] [diff] [review]
patch

I'm going to nominate this for both Aurora and Beta approval.  If this request is processed after the 10/28 cutover this will already be on Aurora, so only the Beta approval request will be relevant.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 883741
User impact if declined: The window.crypto property may be unavailable to a web page if a previous web page sharing the same outer window (e.g. in the same tab) also accessed it and the two pages in question are cross-origin.  This could break web sites that use window.crypto.
Testing completed (on m-c, etc.): On m-c, very simple patch.
Risk to taking this patch (and alternatives if risky): This is a simple low risk patch.  Backing out the regressing bug would be far more risky IMO.
String or IDL/UUID changes made by this patch: None.
Attachment #821663 - Flags: approval-mozilla-beta?
Attachment #821663 - Flags: approval-mozilla-aurora?
(In reply to Olli Pettay [:smaug] from comment #12)
> Yoshi, we need this at least for Aurora, but Beta too?
> It is not clear to me on which branches we have bug 883741. It says
> mozilla26, but
> the regression range in this bug talks about Firefox/25.0.
> 
> Could you please ask approval on the right branches.

Thanks for the patch, smaug.

From mcMerge, https://tbpl.mozilla.org/mcmerge/?cset=8aafb671f1e9, the MileStone is set as Firefox26.

But from the push log, it says FIREFOX_AURORA_25_BASE
https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=ad0ae007aa9e

I think you said in Comment 9 means we need another test for this, I'll file a bug for this.

Also thanks for Khuey for asking for approval here.
Flags: needinfo?(allstars.chh)
https://hg.mozilla.org/mozilla-central/rev/692a1de53310
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Verified on Nightly 27.0a1 buildid 20131025030239.
QA Contact: ananuti
Also, see bug 930827 before making the approval decision please.
Attachment #821663 - Flags: approval-mozilla-beta?
Attachment #821663 - Flags: approval-mozilla-beta+
Attachment #821663 - Flags: approval-mozilla-aurora?
Attachment #821663 - Flags: approval-mozilla-aurora+
Attachment #821663 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Given that this is a regression in window.crypto, which is older than dirt (almost), and it's a regression in 25 I think we have take this. To add to the reasoning, per bug 930827 this is used by some google APIs, and those APIs could be used pretty much anywhere on the web (if not today, then tomorrow).

Kyle, could you look into writing a test for this one so we don't regress this again etc?
Comment on attachment 821663 [details] [diff] [review]
patch

[Triage Comment]
Attachment #821663 - Flags: approval-mozilla-release+
Attachment #821663 - Flags: approval-mozilla-beta?
Attachment #821663 - Flags: approval-mozilla-beta+
I'll write a testcase.
Attached patch testSplinter Review
rs=jst
I tested the following try builds (as indicated by Alex in r-d email) and they do not reproduce this issue, nor bug 930828:
* Beta: https://tbpl.mozilla.org/?tree=Mozilla-Beta&rev=aa459ad5a1ad
* Release: https://tbpl.mozilla.org/?tree=Mozilla-Release&rev=9a3e312d260c

I won't explicitly marked this verified fixed until I have a chance to verify the real Beta and RC builds but I thought I'd check the try builds before we went too far down the build process. Looks promising anyways.
Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 - 20131025150754 (beta 12)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 - 20131025151332 (RC b#3)
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:27.0) Gecko/20100101 Firefox/27.0 - 20131025100746

Still reproducible on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 - 20131025004002

Did this fix make into aurora?
(In reply to Ioana Budnar, QA [:ioana] from comment #32)
> Verified as fixed on:
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 -
> 20131025150754 (beta 12)
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 -
> 20131025151332 (RC b#3)
> Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:27.0) Gecko/20100101
> Firefox/27.0 - 20131025100746
> 
> Still reproducible on:
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 -
> 20131025004002
> 
> Did this fix make into aurora?

Same results on Ubuntu 13.04 and Mac OS X 10.8.4.
Verified on Aurora 26.0a2 buildid 20131026004005.
Status: RESOLVED → VERIFIED
Also verified fixed against Fennec 25 Beta build 3.
Flags: in-testsuite+
this ended up on mozilla-release as well (see FF25 status flag)

https://hg.mozilla.org/releases/mozilla-release/rev/9a3e312d260c
Depends on: 937950
You need to log in before you can comment on or make changes to this bug.