Closed Bug 1427726 Opened 7 years ago Closed 7 years ago

Remove remote jar support (currently behind a disabled about:config pref)

Categories

(Core :: Security, enhancement)

Product:
Core
Component:
Security
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, site-compat)

Attachments

(5 files)

Bug 1427726 - move test for bug 1063538 away from jar URIs,
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1427726 - remove postMessage tests for jars,
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1427726 - remove support for remote JAR files,
59 bytes, text/x-review-board-request
michal
: review+
Details
Bug 1427726 - remove nsIJARChannel::isUnsafe and nsIDocShell::channelIsUnsafe,
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1427726 - remove pref from preference_usage performance test,
59 bytes, text/x-review-board-request
johannh
: review+
Details 
My understanding is that remote JAR stuff is behind a hidden pref that's off by default, and has been since 55. I'm not aware of any fallout from that change. I would like to propose removing the hidden pref and our support for remote jars entirely.

The best argument I currently know of in favour of keeping it is that some developers use it to read archive attachments on bugzilla. The second-best argument is that we haven't actually shipped the removal to ESR yet, AIUI, so there could be compat fallout hiding there. :mkaply, any idea how frequent enterprise use is? (I'm assuming that the now-expired telemetry wouldn't really have painted a great picture of it because enterprise.)

Given only Firefox ever supported the jar: protocol at all, we've now done 3 main releases with support turned off, and we talked about removing it for ages before then (including one backed out attempt for 45, 2 years ago), and supporting it adds complexity in various places where complexity is bad (like security checks!), I am still inclined to suggest removing the pref and our support, though if we have hard data that a significant number of enterprises rely on this, I guess that could change the balance of the argument...
Flags: needinfo?(mozilla)
What was the reason it got backed out on 45? I would probably just post something to the enterprise list as a "notice of removal" and see if folks complain.
Flags: needinfo?(mozilla)
(In reply to Mike Kaply [:mkaply] from comment #1)
> What was the reason it got backed out on 45? I would probably just post
> something to the enterprise list as a "notice of removal" and see if folks
> complain.

IBM iNotes. That got fixed, though. See bug 1255139 for context.
Does removing remote jar: let us simplify security-check code?  I'm not sure why that would be the case (e.g. jar:file:// would still be supported, right?).

That said, it definitely would allow removing a bunch of complexity from jarchannel.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> Does removing remote jar: let us simplify security-check code?  I'm not sure
> why that would be the case (e.g. jar:file:// would still be supported,
> right?).

Sadly, I now believe you're probably right. On desktop, while we load data from that type of location, we tend to address with chrome: or resource: URIs, so having explicit jar:file principals and security checks seemed like something we didn't actually need to support to me - but it seems I'm wrong and at least fennec explicitly passes around even nested jar:, so jar:jar:file:.... :-\
Blocks: 1171853
Blocks: 1430257
No longer blocks: 1171853
I posted to the enterprise list and m.d.platform about this proposed removal. If there's no objections in the next week or two, I'll try to get a patch together.
I have WIP patches for this, but am wondering if I should hold off until bug 1373708 lands. Michal, is that likely to happen soon?
Flags: needinfo?(michal.novotny)
See Also: → 1373708
(In reply to :Gijs from comment #6)
> I have WIP patches for this, but am wondering if I should hold off until bug
> 1373708 lands. Michal, is that likely to happen soon?

I just started to work on it, so I guess it's likely to be fixed soon.
Flags: needinfo?(michal.novotny)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8966156 [details]
Bug 1427726 - move test for bug 1063538 away from jar URIs,

https://reviewboard.mozilla.org/r/234886/#review240540
Attachment #8966156 - Flags: review?(amarchesini) → review+
Comment on attachment 8966305 [details]
Bug 1427726 - remove pref from preference_usage performance test,

https://reviewboard.mozilla.org/r/235028/#review240712

Yay!
Attachment #8966305 - Flags: review?(jhofmann) → review+
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/remote-jar-support-has-been-completely-removed/
(In reply to Kohei Yoshino [:kohei] from comment #16)
> Posted the site compatibility note:
> https://www.fxsitecompat.com/en-CA/docs/2018/remote-jar-support-has-been-
> completely-removed/

Thanks, though this hasn't shipped yet... I still need reviews! :-)
Comment on attachment 8966158 [details]
Bug 1427726 - remove support for remote JAR files,

https://reviewboard.mozilla.org/r/234890/#review240792

::: modules/libjar/nsJARChannel.h:98
(Diff revision 1)
>      struct {
>          bool isCanceled;
>          uint32_t suspendCount;
>      }                               mPendingEvent;
>  
>      bool                            mIsUnsafe;

mIsUnsafe should be IMO removed.

::: modules/libjar/nsJARChannel.cpp:290
(Diff revision 1)
>              rv = jarCache->GetZip(clonedFile, getter_AddRefs(reader));
>          else
>              rv = jarCache->GetInnerZip(clonedFile, mInnerJarEntry,
>                                         getter_AddRefs(reader));
>      } else {
> +        NS_ENSURE_TRUE(mJarFile, NS_ERROR_UNEXPECTED);

This is not needed. mJarFile cannot be null and there is already MOZ_ASSERTION(mJarFile) in this method.

::: modules/libjar/nsJARChannel.cpp:1019
(Diff revision 1)
> -        }
>  
> -    }
> -    else {
> -        rv = OpenLocalFile();
> +    rv = OpenLocalFile();
> -        if (NS_SUCCEEDED(rv)) {
> +    LOG(("nsJARChannel::AsyncOpen [this=%p] 8\n", this));

There are several identical LOG in this method which were added in bug 1373708. Can you please remove all except the very first one in this method?
Attachment #8966158 - Flags: review?(michal.novotny) → review-
(In reply to Michal Novotny (:michal) from comment #18)
> ::: modules/libjar/nsJARChannel.h:98
> (Diff revision 1)
> >      struct {
> >          bool isCanceled;
> >          uint32_t suspendCount;
> >      }                               mPendingEvent;
> >  
> >      bool                            mIsUnsafe;
> 
> mIsUnsafe should be IMO removed.

Yep, this is in the next cset. I separated it out because it entails some docshell code removal as well, so it seemed best to do it separately.

I'll update the patch based on your other comments, thanks for the quick review!
Comment on attachment 8966158 [details]
Bug 1427726 - remove support for remote JAR files,

https://reviewboard.mozilla.org/r/234890/#review240986
Attachment #8966158 - Flags: review?(michal.novotny) → review+
Comment on attachment 8966157 [details]
Bug 1427726 - remove postMessage tests for jars,

https://reviewboard.mozilla.org/r/234888/#review241214

r=me
Attachment #8966157 - Flags: review?(bzbarsky) → review+
Comment on attachment 8966159 [details]
Bug 1427726 - remove nsIJARChannel::isUnsafe and nsIDocShell::channelIsUnsafe,

https://reviewboard.mozilla.org/r/234892/#review241218

::: modules/libjar/nsJARChannel.cpp:944
(Diff revision 2)
>  
>      nsresult rv = LookupFile();
>      if (NS_FAILED(rv))
>          return rv;
>  
>      // If mJarInput was not set by LookupFile, the JAR is a remote jar.

This comment still mentions remote JARs... should it have gotten updated somewhere here?
Attachment #8966159 - Flags: review?(bzbarsky) → review+
Comment on attachment 8966159 [details]
Bug 1427726 - remove nsIJARChannel::isUnsafe and nsIDocShell::channelIsUnsafe,

https://reviewboard.mozilla.org/r/234892/#review241218

> This comment still mentions remote JARs... should it have gotten updated somewhere here?

D'oh, yes, fixed in previous commit.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/753ece6c9f1b
move test for bug 1063538 away from jar URIs, r=baku
https://hg.mozilla.org/integration/autoland/rev/cb35e7b10235
remove postMessage tests for jars, r=bz
https://hg.mozilla.org/integration/autoland/rev/f41cf7811770
remove support for remote JAR files, r=michal
https://hg.mozilla.org/integration/autoland/rev/b1b76f9dff73
remove nsIJARChannel::isUnsafe and nsIDocShell::channelIsUnsafe, r=bz
https://hg.mozilla.org/integration/autoland/rev/ee9abd6f1ba5
remove pref from preference_usage performance test, r=johannh
Backed out for failing linux asan at modules/libjar/test/unit/test_bug407303.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ee9abd6f1ba529d7b5b61c290c87b926ab6b4b2a

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=173050292&repo=autoland&lineNumber=2541

Backout: https://hg.mozilla.org/integration/autoland/rev/f0ae3ef35bcbfd63de832d660e8e5236606575aa
Flags: needinfo?(gijskruitbosch+bugs)
Huh. So it seems verifying "did I get everything" via trypush is just fundamentally a bad idea. More tests to delete, it seems. :-\
(In reply to :Gijs (he/him) from comment #32)
> Huh. So it seems verifying "did I get everything" via trypush is just
> fundamentally a bad idea. More tests to delete, it seems. :-\

Specifically, I neglected to run xpcshell. But also, it seems I forgot to actually 'hg rm' some of the files that I removed from their respective test manifests... fixed now, and verified via grep that there really aren't any pref accesses left.
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/25b2e11c93b4
move test for bug 1063538 away from jar URIs, r=baku
https://hg.mozilla.org/integration/autoland/rev/bfff5b3739f9
remove postMessage tests for jars, r=bz
https://hg.mozilla.org/integration/autoland/rev/a9185d7a30d8
remove support for remote JAR files, r=michal
https://hg.mozilla.org/integration/autoland/rev/064ca3f3d42b
remove nsIJARChannel::isUnsafe and nsIDocShell::channelIsUnsafe, r=bz
https://hg.mozilla.org/integration/autoland/rev/964dbe5e2afe
remove pref from preference_usage performance test, r=johannh
Boris, given that this landed after bug 1373708 (which will stick with 61, afaict) I expect the risk/reward divide is not in favour of uplifting this to 60. Do you agree, or do you think we should try anyway because ESR?
Flags: needinfo?(bzbarsky)
It doesn't seem like this should be that hard to backport if we want to do it...

That said, what is the benefit of doing so?  There's no obvious security win, since it's disabled by default anyway.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #42)
> It doesn't seem like this should be that hard to backport if we want to do
> it...
> 
> That said, what is the benefit of doing so?  There's no obvious security
> win, since it's disabled by default anyway.

That's fair. I'm a bit sad we won't be able to get bug 1430257 done for esr, but so be it.
status-firefox60: affected → fixed
Sigh.
status-firefox60: fixed → wontfix
Ah, hmm.  If we were planning to backport the other bug 1430257 work it would make sense to backport this for sure.  I didn't realize that was that close to being all set to go.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #45)
> Ah, hmm.  If we were planning to backport the other bug 1430257 work it
> would make sense to backport this for sure.

Right

> I didn't realize that was that close to being all set to go.

I think my comment was too brief and therefore misleading. That work *isn't* all set to go. I just wish we'd done this + that sooner, and I'm having trouble accepting the state of the world where that isn't the case, and it likely isn't realistic to fix those bugs, and backport them + this in the space of 1.5 weeks.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: