Arthur Edelstein [:arthur] Assignee
	Description • 9 years ago

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.

	Arthur Edelstein [:arthur] Assignee
	Comment 1 • 9 years ago

Here is the patch, rebased to mozilla-central. The pref is called 'network.jar.block-remote-files'.

	Michael Wu [:mwu]
	Comment 2 • 9 years ago

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.

	Michael Wu [:mwu]
	Comment 3 • 9 years ago

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..

	Jason Duell
	Comment 4 • 9 years ago

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?)

	Jason Duell
	Comment 5 • 9 years ago

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?)

	Valentin Gosu [:valentin] (he/him)
	Comment 6 • 9 years ago

No, the web packaged apps aren't affected by any changes to the JAR channels.

	Arthur Edelstein [:arthur] Assignee
	Comment 7 • 9 years ago

(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

	Jason Duell
	Comment 8 • 9 years ago

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.

	Arthur Edelstein [:arthur] Assignee
	Comment 9 • 9 years ago

(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

	Jason Duell
	Comment 10 • 9 years ago

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.

	Pulsebot
	Comment 11 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf18c1865e7d

	Carsten Book [:Tomcat]
	Comment 12 • 9 years ago

sorry had to back this out for test failures like https://hg.mozilla.org/integration/mozilla-inbound/rev/bf18c1865e7d

	Pulsebot
	Comment 13 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f75b5361f6e

	Arthur Edelstein [:arthur] Assignee
	Comment 14 • 9 years ago

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.

	Arthur Edelstein [:arthur] Assignee
	Comment 15 • 9 years ago

Sorry about the obsolete call. Try server for new version of patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47e5ebaf7939

	Pulsebot
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ee1f299da22

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/6ee1f299da22