1305339 - macOS: Wrong quarantine types for https, ftp

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P3)

Description

[Nils Maier [:nmaier]]

https://dxr.mozilla.org/mozilla-central/rev/29beaebdfaccbdaeb4c1ee5a43a9795ab015ef49/xpcom/io/CocoaFileUtils.mm#227-228

The code checks "http" twice, while I think it is supposed to be checking "https" in the second part.

Also, marking ftp downloads as |kLSQuarantineTypeWebDownload| seems reasonable. And same for blob: and data:... actually every scheme except for file: maybe

Comment 1

[:Gijs (he/him)]

New feature in 51, should probably make sure it treats https and ftp correctly from the get-go.

The trivial patch here is obvious, but isn't there some kind of URI flag we can use to make this determination that would be better than trying to make a 'quarantine these schemes' kind of list (which would be liable to people bypassing it e.g. if add-ons support other schemes, or through crazy things like feed: and view-source: and the like) ?

Comment 2

[Markus Stange [:mstange]]

(In reply to :Gijs Kruitbosch from comment #1)
> but isn't there some kind of URI flag we
> can use to make this determination that would be better than trying to make
> a 'quarantine these schemes' kind of list (which would be liable to people
> bypassing it e.g. if add-ons support other schemes, or through crazy things
> like feed: and view-source: and the like) ?

I don't know. Who would be able to answer this question?

Comment 3

[:Gijs (he/him)]

(In reply to Markus Stange [:mstange] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > but isn't there some kind of URI flag we
> > can use to make this determination that would be better than trying to make
> > a 'quarantine these schemes' kind of list (which would be liable to people
> > bypassing it e.g. if add-ons support other schemes, or through crazy things
> > like feed: and view-source: and the like) ?
> 
> I don't know. Who would be able to answer this question?

I think the basic thing we want is asking for the URI flags from the protocol handler / io service. Not sure if URI_LOADABLE_BY_ANYONE is the right thing or if we want something like not URI_IS_LOCAL_FILE & not URI_IS_UI_RESOURCE... maybe Christoph knows what the right thing is?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #3)
> I think the basic thing we want is asking for the URI flags from the
> protocol handler / io service. Not sure if URI_LOADABLE_BY_ANYONE is the
> right thing or if we want something like not URI_IS_LOCAL_FILE & not
> URI_IS_UI_RESOURCE... maybe Christoph knows what the right thing is?

I am not the expert on our URI flags, but I would imagine that URI_LOADABLE_BY_ANYONE is what we could use here.
I did a brief check and here are the protocols which set URI_LOADABLE_BY_ANYONE:
* http/https
* ftp
* data
* jar
* javascript
* about (only if nsIAboutModule::MAKE_LINKABLE)

The only thing missing would be blob:, not sure how we would whitelist those. Other than that I suppose it's OK to rely on URI_LOADABLE_BY_ANYONE.

Comment 5

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> The only thing missing would be blob

blob is URI_LOADABLE_BY_SUBSUMERS . Technically, not having any flags is also treated by URI_LOADABLE_BY_ANYONE by the security manager, so maybe the negation of chrome / local flags would be better.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #5)
> blob is URI_LOADABLE_BY_SUBSUMERS . Technically, not having any flags is
> also treated by URI_LOADABLE_BY_ANYONE by the security manager, so maybe the
> negation of chrome / local flags would be better.

Why would you prefer the negation of chrome / local flags? Because addons could implement their own protocolhandler which would then use URI_LOADABLE_BY_ANYONE as default if not set explicitly? I would imagine that basically everything that uses URI_LOADABLE_BY_ANYONE is fine to load. Because really the only thing that should not load are local files, right?

Comment 7

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > blob is URI_LOADABLE_BY_SUBSUMERS . Technically, not having any flags is
> > also treated by URI_LOADABLE_BY_ANYONE by the security manager, so maybe the
> > negation of chrome / local flags would be better.
> 
> Why would you prefer the negation of chrome / local flags? Because addons
> could implement their own protocolhandler which would then use
> URI_LOADABLE_BY_ANYONE as default if not set explicitly? I would imagine
> that basically everything that uses URI_LOADABLE_BY_ANYONE is fine to load.
> Because really the only thing that should not load are local files, right?

The point of this flag, AIUI, is to indicate to the OS and apps that files are "downloaded from the web". So we want to be able to check the URI that we're downloading and determine "should we flag this file as coming from the web". It has nothing to do with what should/shouldn't load.

If we're using URI_LOADABLE_BY_ANYONE as a proxy for "came from the web", then there are a few problematic things:
- blobs can come from the web (and so we should err on the side of caution and mark as 'from the web') and don't have this flag (but have a different one)
- if an add-on implements a protocol handler and doesn't use any flag (possible, but not recommended) then no flag will be there but the URL will be "loadable by anyone" and we have no idea where the content came from.

So I think negating the chrome/local thing gives you moderately better assurance that the contents aren't local.

That said, it feels like there ought to be a better way of doing this.

Boris, am I missing something? Paolo, what do we do on Windows for setting "mark of the web"? How do we determine content came from there?

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #7)
> So I think negating the chrome/local thing gives you moderately better
> assurance that the contents aren't local.

Not quite right. What I meant was: better assurance against false negatives, ie files you don't mark as coming from the web but that really did come from somewhere non-local.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I was going to suggest "not URI_IS_LOCAL_RESOURCE", but that's not really the semantics people want here...

What's relevant here is the provenance, not where the actual bits live.  So you want to exclude blob:/data: even though they are URI_IS_LOCAL_RESOURCE.   What about blob:/data: in file:// pages?

In general, for things like blob: and data: just knowing the URI doesn't tell you the origin of the content.  You really have to know the provenance of the URI string in a way that I'm not sure anything captures right now.

So the safe thing is probably to mark-of-the-web anything that is not URI_IS_LOCAL_FILE/URI_IS_UI_RESOURCE/URI_DANGEEROUS_TO_LOAD....

Comment 10

[Nils Maier [:nmaier]]

(In reply to :Gijs Kruitbosch from comment #8)
> Not quite right. What I meant was: better assurance against false negatives,
> ie files you don't mark as coming from the web but that really did come from
> somewhere non-local.

Yes, my initial point exactly. That's why I suggested being "generous" when marking files. And actually that's what happening in the code currently; the distinction is only if it's a web download or "other" download.

As for finding out the true origin of things (in more cases):
- blob:-URIs implement nsIURIWithPrincipal:
https://dxr.mozilla.org/mozilla-central/rev/66a77b9bfe5dcacd50eccf85de7c0e7e15ce0ffd/netwerk/base/nsIURIWithPrincipal.idl#15
- Some other URIs (such as chrome-URIs IIRC) implement nsINestedURI:
https://dxr.mozilla.org/mozilla-central/rev/66a77b9bfe5dcacd50eccf85de7c0e7e15ce0ffd/netwerk/base/nsINestedURI.idl#25

As for what happens on Windows:
The code tries to map the URI to a zone, and if that fails defaults to ZONE_INTERNET
https://dxr.mozilla.org/mozilla-central/rev/66a77b9bfe5dcacd50eccf85de7c0e7e15ce0ffd/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#612
Zone mapping is done via https://msdn.microsoft.com/en-us/library/ms537133(v=vs.85).aspx , so this will fail for things such as data: URIs and blob: URIs and therefore use the ZONE_INTERNET default

Comment 11

[Boris Zbarsky [:bzbarsky]]

> - blob:-URIs implement nsIURIWithPrincipal:

That's not sufficient to determine true origin, because of sandboxed iframes.  Of course you could just treat those as "web" in all cases, even when it's from a file:// page, but...

Comment 12

[:Gijs (he/him)]

Attachment: Bug 1305339 - part 0: enable quarantine functionality when OSX doesn't do it for us,

It's widely documented on the web that Apple enforces quarantine attributes
'for us' using the list in
/System/Library/CoreServices/CoreTypes.bundle/Contents/Resources/Exceptions.plist

Unfortunately, this is based on the bundle ID. Which means it won't apply to custom
builds, to Nightly, and potentially to other cases. It would also be much nicer
if we could just make these determinations ourselves. Step 1: opt-in from Info.plist

Without this, for e.g. local Nightly builds, asking for quarantine attributes
simply gets you a nil dictionary and our code bails out.

Review commit: https://reviewboard.mozilla.org/r/82762/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82762/

Comment 13

[:Gijs (he/him)]

Attachment: Bug 1305339 - part 1: use correct key to get and set quarantine information on 10.10+,

kLSItemQuarantineProperties was deprecated in 10.10. AFAICT it was replaced by
kCFURLQuarantinePropertiesKey, which was inconveniently new in 10.10.
On my 10.11 machine, the Info.plist fix from the previous patch was not
sufficient to get the data to actually show up when using the old (deprecated)
key. I suspect the setter is a no-op with the old key. So here's code that
uses the new key ("documented" in LSQuarantine.h, where conveniently
the actual properties in the dictionary have kept their keys, but the
dictionary key is now referenced as the CF one).

Review commit: https://reviewboard.mozilla.org/r/82764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82764/

Comment 14

[:Gijs (he/him)]

Attachment: Bug 1305339 - part 2: use URI flags to determine if data is from the web,

This actually accomplishes what was discussed in the bug and marks any file with the
relevant URI flags as WebDownload, and everything else as OtherDownload.

Note that I'm using DoGetProtocolFlags in order to deal with
nsIProtocolHandlerWithDynamicFlags correctly; while just getting protocol flags
from the IO service directly would be less work, it's technically less correct.

Review commit: https://reviewboard.mozilla.org/r/82766/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82766/

Comment 15

[:Gijs (he/him)]

Boris, can you check the URI flag logic in part 2, please?

Comment 16

[Markus Stange [:mstange]]

(In reply to :Gijs Kruitbosch from comment #12)
> It's widely documented on the web that Apple enforces quarantine attributes
> 'for us' using the list in
> /System/Library/CoreServices/CoreTypes.bundle/Contents/Resources/Exceptions.
> plist
> 
> Unfortunately, this is based on the bundle ID. Which means it won't apply to
> custom
> builds, to Nightly, and potentially to other cases. It would also be much
> nicer
> if we could just make these determinations ourselves. Step 1: opt-in from
> Info.plist
> 
> Without this, for e.g. local Nightly builds, asking for quarantine attributes
> simply gets you a nil dictionary and our code bails out.

Thank you for getting to the bottom of this. I had no idea.

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8797146 [details]
Bug 1305339 - part 0: enable quarantine functionality when OSX doesn't do it for us,

https://reviewboard.mozilla.org/r/82762/#review81456

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8797147 [details]
Bug 1305339 - part 1: use correct key to get and set quarantine information on 10.10+,

https://reviewboard.mozilla.org/r/82764/#review81466

::: xpcom/io/CocoaFileUtils.mm:14
(Diff revision 1)
> +// Need to cope with us using old versions of the SDK and needing this on 10.10+
> +#ifndef kCFURLQuarantinePropertiesKey
> +#define kCFURLQuarantinePropertiesKey CFSTR("NSURLQuarantinePropertiesKey")
> +#endif

In the 10.10 SDK (or in later SDKs), kCFURLQuarantinePropertiesKey is not a preprocessor define, so the "#ifndef kCFURLQuarantinePropertiesKey" will always match regardless of SDK. A better solution would be:

#if !defined(MAC_OS_X_VERSION_10_10) || (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_10)
const CFStringRef kCFURLQuarantinePropertiesKey = CFSTR("NSURLQuarantinePropertiesKey");
#endif

This actually checks for the SDK and defines the variable in the same way as the headers do.

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 8797148 [details]
Bug 1305339 - part 2: use URI flags to determine if data is from the web,

https://reviewboard.mozilla.org/r/82766/#review81470

This seems ok code-wise. I'll leave it to bz to check that this is actually doing the right thing.

By the way, I think if your commit message had contained "r?mstange, r?bz" instead of "r?mstange,bz", mozreview would have requested a proper review from bz.

Comment 20

[:Gijs (he/him)]

(In reply to Markus Stange [:mstange] from comment #19)
> By the way, I think if your commit message had contained "r?mstange, r?bz"
> instead of "r?mstange,bz", mozreview would have requested a proper review
> from bz.

No, mozreview tried requesting review alright, but when I tried to publish the commits bz still had review requests blocked (in which case you can't publish, you have to manually remove the reviewer and then publish, which is what I did, and why I used needinfo instead).

Comment 21

[Markus Stange [:mstange]]

Ah, I see.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8797148 [details]
Bug 1305339 - part 2: use URI flags to determine if data is from the web,

https://reviewboard.mozilla.org/r/82766/#review81798

::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp:243
(Diff revision 1)
> +  if (!aURI) {
> +    return true;
> +  }
> +
> +  nsAutoCString scheme;
> +  nsresult rv = aURI->GetScheme(scheme);

I think it would be safer in terms of nested URI handling to do something like:

    bool hasFlag;
    if (NS_SUCCEEDED(NS_URIChainHasFlags(aURI, nsIProtocolHandler::URI_DANGEROUS_TO_LOAD, &hasFlag)) &&
        hasFlag) {
      return false;
    }
    
    // Repeat for the other two flags
    
If we are worried about the performance aspect of getting the protocol handler multiple times (which I don't think we should be), we can go ahead and add a NS_URIChainHasAnyFlags and corresponding backend bits in the ioservice...  Certainly we have other consumers who have wanted such a thing.

r=me on the protocol flag bits with that.

Comment 23

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #22)
> Comment on attachment 8797148 [details]
> Bug 1305339 - part 2: use URI flags to determine if data is from the web,
> 
> https://reviewboard.mozilla.org/r/82766/#review81798
> 
> ::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp:243
> (Diff revision 1)
> > +  if (!aURI) {
> > +    return true;
> > +  }
> > +
> > +  nsAutoCString scheme;
> > +  nsresult rv = aURI->GetScheme(scheme);
> 
> I think it would be safer in terms of nested URI handling to do something
> like:
> 
>     bool hasFlag;
>     if (NS_SUCCEEDED(NS_URIChainHasFlags(aURI,
> nsIProtocolHandler::URI_DANGEROUS_TO_LOAD, &hasFlag)) &&
>         hasFlag) {
>       return false;
>     }
>     
>     // Repeat for the other two flags
>     
> If we are worried about the performance aspect of getting the protocol
> handler multiple times (which I don't think we should be), we can go ahead
> and add a NS_URIChainHasAnyFlags and corresponding backend bits in the
> ioservice...  Certainly we have other consumers who have wanted such a thing.
> 
> r=me on the protocol flag bits with that.

This will mark e.g. view-source:http:// as not-web, because view-source has DANGEROUS_TO_LOAD. Ditto for feed:... I don't think that's what we want?

It feels like we actually want NS_URIChainLacksFlags(), which doesn't exist right now...

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, good catch.  

What we really want here for maximum safety is "all things in the URI chain have one of these flags", which as you note doesn't exist right now...

Comment 25

[:Gijs (he/him)]

Per discussion with bz on IRC, going to rework the last patch to actually work the nested URI chain inside that code. Then I'll update the patchset and re-request review.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8797146 [details]
Bug 1305339 - part 0: enable quarantine functionality when OSX doesn't do it for us,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82762/diff/1-2/

Comment 27

[:Gijs (he/him)]

Comment on attachment 8797147 [details]
Bug 1305339 - part 1: use correct key to get and set quarantine information on 10.10+,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82764/diff/1-2/

Comment 28

[:Gijs (he/him)]

Comment on attachment 8797148 [details]
Bug 1305339 - part 2: use URI flags to determine if data is from the web,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82766/diff/1-2/

Comment 29

[:Gijs (he/him)]

Comment on attachment 8797148 [details]
Bug 1305339 - part 2: use URI flags to determine if data is from the web,

Boris, can you doublecheck my nested URI code? I've messed it up once before... :-\

(I tried testing, but I couldn't easily come up with something that was downloadable and manage to feed that through feed: or view-source: or whatever)

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8797148 [details]
Bug 1305339 - part 2: use URI flags to determine if data is from the web,

https://reviewboard.mozilla.org/r/82766/#review83250

::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp:238
(Diff revision 2)
>    return NS_ERROR_NOT_IMPLEMENTED;
>  #endif
>  }
> +
> +// Check if a URI is likely to be web-based, by checking its URI flags.
> +// Is conservative about claiming things aren't from the web.

I think this would be clearer as:

    // If in doubt (e.g. if anything fails during the check) claims things are from the web.
    
instead of leaving the reader guessing what "conservative" would mean in this case.

::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp:248
(Diff revision 2)
> +  if (!ios) {
> +    return true;
> +  }
> +
> +  while (uri) {
> +    nsAutoCString scheme;

This may be worth a comment about how you're not using nsIIOService::ProtocolHasFlags, because that checks for all the given flags being present, while you want to check for any one of the given flags being present.

::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp:275
(Diff revision 2)
> +    // Otherwise, check if the URI is nested, and if so go through
> +    // the loop again:
> +    nsCOMPtr<nsINestedURI> nestedURI = do_QueryInterface(uri);
> +    uri = nullptr;
> +    if (nestedURI) {
> +      nsresult rv2;

Might as well just reuse rv here, I'd think.

r=me

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40764ff2dfd1
part 0: enable quarantine functionality when OSX doesn't do it for us, r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd5f04019bc
part 1: use correct key to get and set quarantine information on 10.10+, r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/176c3e7915d1
part 2: use URI flags to determine if data is from the web, r=mstange,bz

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/40764ff2dfd1
https://hg.mozilla.org/mozilla-central/rev/ebd5f04019bc
https://hg.mozilla.org/mozilla-central/rev/176c3e7915d1

Comment 33

[Gerry Chang [:gchang]]

Hi :Gijs,
From comment #1, what is the feature in 51? Is this worth uplifting to 51? Is there any possibilities that you can add more tests?

Comment 34

[:Gijs (he/him)]

(In reply to Gerry Chang [:gchang] from comment #33)
> Hi :Gijs,
> From comment #1, what is the feature in 51? Is this worth uplifting to 51?
> Is there any possibilities that you can add more tests?

There are tests that exercise this code, and the patches add tests.

It probably makes sense to uplift, yes.

Comment 35

[:Gijs (he/him)]

Oh, but we should uplift with the fix for bug 1310518. Once I write it. :-\

Comment 36

[:Gijs (he/him)]

Comment on attachment 8797146 [details]
Bug 1305339 - part 0: enable quarantine functionality when OSX doesn't do it for us,

(In reply to :Gijs Kruitbosch from comment #34)
> (In reply to Gerry Chang [:gchang] from comment #33)
> > Hi :Gijs,
> > From comment #1, what is the feature in 51? Is this worth uplifting to 51?
> > Is there any possibilities that you can add more tests?
> 
> There are tests that exercise this code, and the patches add tests.

Err, I was confused with a different bug when I wrote this - this patchset does not add tests. Sorry for the confusion.

There are general download tests, but AFAIK the patches in bug 337051 did not land with tests and this bug did not add such tests.


NB: approval request for all 3 patches as well as bug 1310518. I'll see about adding a unified patch for uplift.

Approval Request Comment
[Feature/regressing bug #]: bug 337051
[User impact if declined]: OS X download information / sandboxing doesn't work correctly on newer versions of OS X and/or with non-official (ie release/beta) versions of Firefox, and/or with non-http (e.g. ftp/https) URLs.
[Describe test coverage new/current, TreeHerder]: Downloading files generally is tested using automated tests, so I'm confident nothing is catastrophically broken. However, because it's a bit tricky to simulate a download from compiled code tests, and equally there aren't currently straightforward APIs to verify the attributes we're using in these patches from browser frontend code, there are currently no automated tests that do this. I can file a followup bug to write some, but it might be difficult and therefore take additional time. I would recommend not delaying landing these patches while waiting for those tests.
[Risks and why]: reasonably low. The changes in these patches are localized and relatively straightforward, and this is "only" aurora uplift.
[String/UUID change made/needed]: no.

Comment 37

[:Gijs (he/him)]

Attachment: Patch for aurora

Comment 38

[Gerry Chang [:gchang]]

Track 51+ as download information doesn't work correctly in OS X.

Comment 39

[Gerry Chang [:gchang]]

Comment on attachment 8797146 [details]
Bug 1305339 - part 0: enable quarantine functionality when OSX doesn't do it for us,

Fix a download issue in OS X. Take it in 51 aurora.

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f6f97d57ad5