Closed Bug 1173171 Opened 5 years ago Closed 4 years ago

Provide pref to disable download of remote jar files (Tor 12430)

Categories

(Core :: Networking: JAR, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: arthur, Assigned: arthur)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(1 file, 3 obsolete files)

Remote jar files are potentially dangerous and seldom used on the web. In Tor Browser, we added a pref that prevents the download of remote jar files via the jar: protocol. We'd like to propose upstreaming this patch to Firefox.
Here is the patch, rebased to mozilla-central. The pref is called 'network.jar.block-remote-files'.
Attachment #8617640 - Flags: review?(mwu)
Whiteboard: [tor]
Comment on attachment 8617640 [details] [diff] [review]
0001-Bug-12430-Disable-external-jar-via-preference.patch

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

I think jduell knows the remote jar code a bit better than I do.
Attachment #8617640 - Flags: review?(mwu) → review?(jduell.mcbugs)
Fabrice, would this affect apps? I'm guessing no since apps should always be installed locally, but I don't know about any future plans..
Flags: needinfo?(fabrice)
Comment on attachment 8617640 [details] [diff] [review]
0001-Bug-12430-Disable-external-jar-via-preference.patch

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

I suspect this patch will do the right thing, but I can't tell because it doesn't apply cleanly for me.  Is this patch on top of other TOR mods, by any chance? Line 855 is not within AsyncOpen in the mozilla-inbound tree, for instance, and the existing comment that the patch assumes is there ("// These variables must only be set if we're going to trigger") is not in the tree.

Cut me a new patch and we'll talk :)

Oh, and any time we add a new pref, add it (with a default value: in this case, for Mozilla trunk, we'll want "false", and TOR can set to "true") to ./modules/libpref/init/all.js. (Unless you need different values for different products, i.e. Desktop Firefox vs Android firefox, etc. That's not the case here).  I asked on #developers to make sure, and I hear that pref checks (like in this patch) will also throw with an error if they're not set.  (Did you test this patch?)
Attachment #8617640 - Flags: review?(jduell.mcbugs) → feedback+
Valentin, is the stuff you're doing for packages-over-HTTP going to be broken by this patch?  (I.e. do remote package URI loads map to remote jar channels?)
Flags: needinfo?(valentin.gosu)
No, the web packaged apps aren't affected by any changes to the JAR channels.
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(fabrice)
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #4)
> Comment on attachment 8617640 [details] [diff] [review]
> 0001-Bug-12430-Disable-external-jar-via-preference.patch
> 
> Review of attachment 8617640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I suspect this patch will do the right thing, but I can't tell because it
> doesn't apply cleanly for me.  Is this patch on top of other TOR mods, by
> any chance? Line 855 is not within AsyncOpen in the mozilla-inbound tree,
> for instance, and the existing comment that the patch assumes is there ("//
> These variables must only be set if we're going to trigger") is not in the
> tree.

Oops, sorry -- I accidentally sent an old version of the patch. Thanks for the feedback.

> Cut me a new patch and we'll talk :)
> 
> Oh, and any time we add a new pref, add it (with a default value: in this
> case, for Mozilla trunk, we'll want "false", and TOR can set to "true") to
> ./modules/libpref/init/all.js. (Unless you need different values for
> different products, i.e. Desktop Firefox vs Android firefox, etc. That's not
> the case here).  I asked on #developers to make sure, and I hear that pref
> checks (like in this patch) will also throw with an error if they're not
> set.  (Did you test this patch?)

Here's the patch on top of the latest mozilla-central, including a default setting for the pref, added to ./modules/libpref/init/all.js.

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ac7b9b9f846
Attachment #8617640 - Attachment is obsolete: true
Attachment #8624038 - Flags: review?(jduell.mcbugs)
Comment on attachment 8624038 [details] [diff] [review]
0001-Disable-external-jar-via-preference.patch

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

The general approach here is fine, but I think you need to move where you do your check.

::: modules/libjar/nsJARChannel.cpp
@@ +947,5 @@
>  
> +    // Check preferences to see if all remote jar support should be disabled
> +    if (!mJarFile && Preferences::GetBool("network.jar.block-remote-files", true)) {
> +        mIsUnsafe = true;
> +        return NS_ERROR_UNSAFE_CONTENT_TYPE;

I don't think this is the right place for your check.  mJarFile is always null here (we set it to null a few lines above this), so IIUC you'll fail all jar channels here if your pref is set, including local jar files.  Instead I think you need to wait until we've called LookupFile, and if mJarFile is null after that, we actually knew we're remote and you can do your check then.  I'm thinking it would go here:

  https://dxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARChannel.cpp?from=nsJARChannel.cpp#1019

@@ +1180,5 @@
>          mContentDisposition = NS_GetContentDispositionFromHeader(mContentDispositionHeader, this);
>      }
>  
> +    // This is a defense-in-depth check for the preferences to see if all remote jar
> +    // support should be disabled. This check may not be needed.

Yes, I don't think you need this check here, but it can't hurt.  But given how big on an error this would be, it's simpler to use a MOZ_RELEASE_ASSERT(!Preferences::GetBool("network.jar.block-remote-files", true))?  Better to crash the browser with a report if we're this off in our logic.
Attachment #8624038 - Flags: review?(jduell.mcbugs) → review-
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #8)
> Comment on attachment 8624038 [details] [diff] [review]
> 0001-Disable-external-jar-via-preference.patch
> 
> Review of attachment 8624038 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The general approach here is fine, but I think you need to move where you do
> your check.
> 
> ::: modules/libjar/nsJARChannel.cpp
> @@ +947,5 @@
> >  
> > +    // Check preferences to see if all remote jar support should be disabled
> > +    if (!mJarFile && Preferences::GetBool("network.jar.block-remote-files", true)) {
> > +        mIsUnsafe = true;
> > +        return NS_ERROR_UNSAFE_CONTENT_TYPE;
> 
> I don't think this is the right place for your check.  mJarFile is always
> null here (we set it to null a few lines above this), so IIUC you'll fail
> all jar channels here if your pref is set, including local jar files. 
> Instead I think you need to wait until we've called LookupFile, and if
> mJarFile is null after that, we actually knew we're remote and you can do
> your check then.  I'm thinking it would go here:
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARChannel.
> cpp?from=nsJARChannel.cpp#1019

Oops, thanks for catching this -- fixed.

> @@ +1180,5 @@
> >          mContentDisposition = NS_GetContentDispositionFromHeader(mContentDispositionHeader, this);
> >      }
> >  
> > +    // This is a defense-in-depth check for the preferences to see if all remote jar
> > +    // support should be disabled. This check may not be needed.
> 
> Yes, I don't think you need this check here, but it can't hurt.  But given
> how big on an error this would be, it's simpler to use a
> MOZ_RELEASE_ASSERT(!Preferences::GetBool("network.jar.block-remote-files",
> true))?  Better to crash the browser with a report if we're this off in our
> logic.

Good idea.

In this new version of the patch, I have included a simple mochitest to ensure the pref is working as expected.

try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc575350fec3
Attachment #8624038 - Attachment is obsolete: true
Attachment #8641265 - Flags: review?(jduell.mcbugs)
Comment on attachment 8641265 [details] [diff] [review]
0001-Bug-1173171-Disable-external-jar-via-preference.patch

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

Looks good!  Thanks Arthur. I'll mark this checkin-needed and it should be in mozilla-central soon.
Attachment #8641265 - Flags: review?(jduell.mcbugs) → review+
Keywords: checkin-needed
sorry had to back this out for test failures like https://hg.mozilla.org/integration/mozilla-inbound/rev/bf18c1865e7d
Flags: needinfo?(arthuredelstein)
I'm removing the recently obsoleted call to spawnTask(...) and using the new add_task(...) in mochitest-plain instead. Otherwise this patch is the same as the last reviewed patch.
Attachment #8641265 - Attachment is obsolete: true
Sorry about the obsolete call. Try server for new version of patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47e5ebaf7939
Flags: needinfo?(arthuredelstein)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6ee1f299da22
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee: nobody → arthuredelstein
Summary: Provide pref to disable download of remote jar files → Provide pref to disable download of remote jar files (Tor 12430)
Blocks: meta_tor
You need to log in before you can comment on or make changes to this bug.