Closed Bug 1215745 Opened 5 years ago Closed 5 years ago

nsJARChannel::AsyncOpen2 often doesn't do security checks

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla44
Tracking Status
firefox41 --- unaffected
firefox42 + verified
firefox43 + verified
firefox44 + verified

People

(Reporter: sicking, Assigned: sicking)

References

Details

(Keywords: sec-high)

Attachments

(1 file)

Attached patch jarchannelSplinter Review
We made a bad mistake when implementing nsJARChannel::AsyncOpen2. The thinking was that instead of doing security checks, we were going to let the inner channel take care of them.

The problem is that the nsJARChannel implementation doesn't always open an inner channel. If given a file:// URL, it opens that file directly, without using an inner channel.

This means that it's currently possible for any page to load from any zip-file on the users system, without any security checks happening. Which is obviously pretty bad.

We could fix this by simply adding a call to nsContentSecurityManager::doContentSecurityCheck in the codepath which opens files directly. However a safer solution seems to be to make nsJARChannel follow the same approach that we do elsewhere and call nsContentSecurityManager::doContentSecurityCheck directly from within AsyncOpen2.

The reason we didn't before is that it would mean that we'll run through security checks twice in the case when there's an inner channel. However the first time we will set the 'initialSecurityCheckDone' flag, which means that when we call AsyncOpen2 on the inner channel, we won't do any more additional checks than we do on redirect.

So lets do the safer solution here.

[Tracking Requested - why for this release]: This bug enables loading content in arbitrary zip files from the users filesystem. I *think* that so far it doesn't allow extracting arbitrary data from the files, but I'm not sure. Most likely you can extract some data from some file types, like extracting video size from video files.

This regression was caused by bug 1143922
Attachment #8675193 - Flags: review?(mozilla)
Comment on attachment 8675193 [details] [diff] [review]
jarchannel

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

Thanks for catching that Jonas. Obviously that is not great, but luckily you found the problem before the code in question hits release. You should land and request uplift all the way to beta.
Attachment #8675193 - Flags: review?(mozilla) → review+
Comment on attachment 8675193 [details] [diff] [review]
jarchannel

Approval Request Comment
[Feature/regressing bug #]:1143922

[User impact if declined]: This bug enables loading content in arbitrary zip files from the users filesystem. I *think* that so far it doesn't allow extracting arbitrary data from the files, but I'm not sure. Most likely you can extract some data from some file types, like extracting video size from video files.

[Describe test coverage new/current, TreeHerder]: Unfortunately there's no tests of this currently in the tree, which is why this regression could happen. The patches in bug 1195167 will change some existing code to exercise this code more, such that it will be covered by tests.

[Risks and why]: The risk is very low. The exact same code pattern is used by lots of other channel implementations, including the http channel which is quite thoroughly tested. The tests in bug 1195167 doesn't pass try without this patch, but does pass tests with it.

[String/UUID change made/needed]: None
Attachment #8675193 - Flags: approval-mozilla-beta?
Attachment #8675193 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b5fdf7bbfeb8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: core-security → core-security-release
Sounds like we should uplift this.  I will leave that to Sylvestre since we are late in beta. 
Shouldn't this have a rating and sec approval though?
In the meantime, tracking this for 43+. Looks like the regression is from 42.
Flags: needinfo?(sledru)
Flags: needinfo?(abillings)
Yes, this needs sec-approval? set and questions answered, as well as a security rating. It should not have been checked in with no rating and no sec-approval since it affects more than trunk.
Flags: needinfo?(abillings)
I'm happy to uplift as soon as I get approval for the patches.

Sorry if I didn't set the right flags before landing. The process has changed a lot since I last wrote patches. I'll try to find the proper documentation.

(I've seen a warning pop up in the attachments section with a description of what the proper procedure is. But that doesn't show up in this bug for some reason)
Comment on attachment 8675193 [details] [diff] [review]
jarchannel

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

Trying this flag for now
Attachment #8675193 - Flags: sec-approval?
Comment on attachment 8675193 [details] [diff] [review]
jarchannel

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

For some reason the questions weren't automatically added, so grabbed them from the wiki.

[Security approval request comment] 
How easily can the security issue be deduced from the patch?

It's definitely possible. Though requires understanding what nsIChannel.AsyncOpen2 is.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? 

No.

Which older supported branches are affected by this flaw?

See comment 3

If not all supported branches, which bug introduced the flaw?

See comment 3

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? 

The patch should apply on all needed branches.

How likely is this patch to cause regressions; how much testing does it need? 

See comment 3
Flags: needinfo?(sledru)
Comment on attachment 8675193 [details] [diff] [review]
jarchannel

Thanks Liz.

Approved to aurora & beta as it, unfortunately, already landed in m-c.
Attachment #8675193 - Flags: approval-mozilla-beta?
Attachment #8675193 - Flags: approval-mozilla-beta+
Attachment #8675193 - Flags: approval-mozilla-aurora?
Attachment #8675193 - Flags: approval-mozilla-aurora+
Attachment #8675193 - Flags: sec-approval? → sec-approval+
QA Contact: mwobensmith
I made an attempt at verifying this bug fix. However, I have not been able to see the actual bug in action. 

Comment 0 says it was possible to load a page via the combined jar/file scheme, pointing to the local zip file. However, I've tried loading this resource via iframe and href, and both cases show the expected security warning in the console and do not load.

Examples:
<iframe src="jar:file:///Users/username/Desktop/test.zip!/test.htm">
<a href="jar:file:///Users/username/Desktop/test.zip!/test.htm">

I've tried nightly builds from 2015-07-27, 2015-08-31, and 2015-10-16, which should contain the bug if I am not mistaken. 

I'm happy to take another look at this if I'm given more information.
Given that bug 1195167 has landed, the code here is now exercised in CI. So that probably makes manual testing less important.

That's how I verified the fix. Landing patches in bug 1195167 caused test failures, landing patch in this bug fixed them.

That said, if you do want to try to verify the fix, I *believe* that you could do something like:

<video src="jar:file:///path/to/file.zip!/video.mp4">

Where "path/to/file.zip" is the path and filename of a zip-file on your filesystem. This zip should contain a video called "video.mp4" (you might want to double-check that the video can be played in firefox when not in a zip file).

Put the above markup in a HTML file that lives on a webserver. The video should *not* play.
Thanks Jonas. I can see this behavior in the video tag for affected builds, and can verify that it's been fixed in latest builds of Fx42/43/44. Much appreciated.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.