1008778 - Download manager should respect Security Zone settings

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Masatoshi Kimura [:emk]]

Steps to reproduce:
1. Add a site to Local Intranet Zone.
2. Download a file from the site.
3. Add a site to Restricted Site Zone.
4. Download a file from the site.

Actual result:
All files will be marked with ZoneId=3

Expected result:
Files from Intranet Zone and Trusted Site zone should not be marked.
Files from Restricted Site Zone should be marked with ZoneId=4 (it will prevent downloaded files from execute).

Chrome will treat downloaded files as expected. IE will not mark files from Trusted/Intranet Zone. IE will not allow download files from Restricted Sites by default, but if the security settings changed to allow download files, it will also mark files from Restricted Sites with ZoneId=4.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Respect Security Zone settings when the download manager marks a downloaded file

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Respect Security Zone settings when the download manager marks a downloaded file

Forgot to bump the iid.

Comment 3

[Masatoshi Kimura [:emk]]

Although this patch changes the interface, I would like to land this patch to aurora (Firefox 31) because aurora will be the next ESR and this bug will affect Enterprise users downloading files from their Intranet sites (see bug 952961 comment #33, for example).

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Tracking because we want ESR to be in good shape.

Comment 5

[:Paolo Amadini]

Comment on attachment 8420723 [details] [diff] [review]
Respect Security Zone settings when the download manager marks a downloaded file

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

Thanks for finding this API! This is another step towards matching Windows behavior without affecting performance at the time of downloading.

I assume that the API works without causing too much main-thread I/O (except maybe for library loading or registry access).

::: toolkit/components/jsdownloads/public/mozIDownloadPlatform.idl
@@ +48,5 @@
> +   *         0: My Computer
> +   *         1: Local Intranet
> +   *         2: Trusted Sites
> +   *         3: Internet
> +   *         4: Restricted Sites

I wonder if we should just define constants for these in the IDL?

@@ +50,5 @@
> +   *         2: Trusted Sites
> +   *         3: Internet
> +   *         4: Restricted Sites
> +   */
> +  unsigned long MapUrlToZone(in AString aURL);

nit: mapURLToZone (following Mozilla conventions)

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +576,5 @@
>        // current and future versions of Windows.
>        if (this._shouldSaveZoneInformation()) {
>          try {
> +          let zone = gDownloadPlatform.MapUrlToZone(aDownload.source.url);
> +          if (zone >= 3) {

Please add a comment on why we don't write zone IDs lesser than 3 to the ADS.

::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp
@@ +156,5 @@
> +  nsRefPtr<IInternetSecurityManager> inetSecMgr;
> +  DWORD zone;
> +  if (FAILED(CoCreateInstance(CLSID_InternetSecurityManager, NULL,
> +                              CLSCTX_ALL, IID_IInternetSecurityManager,
> +                              getter_AddRefs(inetSecMgr))) ||

For code clarity, please call CoCreateInstance and check for errors separately first.

I also think that unexpected errors of this type should be reported to the JavaScript caller, leaving this function as a dumb wrapper. As I understand it, if the operating system is set up properly and this is a Windows version supported by Firefox, the CoCreateInstance call should always succeed.

@@ +158,5 @@
> +  if (FAILED(CoCreateInstance(CLSID_InternetSecurityManager, NULL,
> +                              CLSCTX_ALL, IID_IInternetSecurityManager,
> +                              getter_AddRefs(inetSecMgr))) ||
> +      inetSecMgr->MapUrlToZone(PromiseFlatString(aURL).get(),
> +                               &zone, 0) != S_OK ||

The documentation says this can return E_INVALIDARG for local files. Can you test if we can improve our handling of this case? (We should probably not propagate the error to JavaScript if this is the case.)

@@ +159,5 @@
> +                              CLSCTX_ALL, IID_IInternetSecurityManager,
> +                              getter_AddRefs(inetSecMgr))) ||
> +      inetSecMgr->MapUrlToZone(PromiseFlatString(aURL).get(),
> +                               &zone, 0) != S_OK ||
> +      zone >= 5) {

A comment about why we are translating higher zone numbers to the Internet zone would be appreciated. Is there a reason not to just write the higher zone number to the ADS?

Also, this logic could be moved to the JavaScript side.

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Respect Security Zone settings when the download manager marks a downloaded file

(In reply to :Paolo Amadini from comment #5)
> I assume that the API works without causing too much main-thread I/O (except
> maybe for library loading or registry access).

I believe this API is pretty fast because IE will have to determine the security zone for every page load.

> ::: toolkit/components/jsdownloads/public/mozIDownloadPlatform.idl
> @@ +48,5 @@
> > +   *         0: My Computer
> > +   *         1: Local Intranet
> > +   *         2: Trusted Sites
> > +   *         3: Internet
> > +   *         4: Restricted Sites
> 
> I wonder if we should just define constants for these in the IDL?

Done.

> @@ +50,5 @@
> > +   *         2: Trusted Sites
> > +   *         3: Internet
> > +   *         4: Restricted Sites
> > +   */
> > +  unsigned long MapUrlToZone(in AString aURL);
> 
> nit: mapURLToZone (following Mozilla conventions)

OK.

> ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
> @@ +576,5 @@
> >        // current and future versions of Windows.
> >        if (this._shouldSaveZoneInformation()) {
> >          try {
> > +          let zone = gDownloadPlatform.MapUrlToZone(aDownload.source.url);
> > +          if (zone >= 3) {
> 
> Please add a comment on why we don't write zone IDs lesser than 3 to the ADS.

Comment added.

> ::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp
> @@ +156,5 @@
> > +  nsRefPtr<IInternetSecurityManager> inetSecMgr;
> > +  DWORD zone;
> > +  if (FAILED(CoCreateInstance(CLSID_InternetSecurityManager, NULL,
> > +                              CLSCTX_ALL, IID_IInternetSecurityManager,
> > +                              getter_AddRefs(inetSecMgr))) ||
> 
> For code clarity, please call CoCreateInstance and check for errors
> separately first.
> 
> I also think that unexpected errors of this type should be reported to the
> JavaScript caller, leaving this function as a dumb wrapper. As I understand
> it, if the operating system is set up properly and this is a Windows version
> supported by Firefox, the CoCreateInstance call should always succeed.

Done.

> @@ +158,5 @@
> > +  if (FAILED(CoCreateInstance(CLSID_InternetSecurityManager, NULL,
> > +                              CLSCTX_ALL, IID_IInternetSecurityManager,
> > +                              getter_AddRefs(inetSecMgr))) ||
> > +      inetSecMgr->MapUrlToZone(PromiseFlatString(aURL).get(),
> > +                               &zone, 0) != S_OK ||
> 
> The documentation says this can return E_INVALIDARG for local files. Can you
> test if we can improve our handling of this case? (We should probably not
> propagate the error to JavaScript if this is the case.)

I tested for some local (file:) URLs, but I couldn't reproduce the documented behavior. The function returned S_OK with zone number 0 (My Computer) for the most paths, or returned 3 (Internet) for MOTW'ed files, files with zoneIDs, or files under AppData\LocalLow.
Even if the function failed, the only caveat is that files would be incorrectly marked as if it came from Internet Zone when users "downloaded" the files from their local drive. I think it is reasonable to assume Internet Zone in such corner cases for a fail-safe.

> @@ +159,5 @@
> > +                              CLSCTX_ALL, IID_IInternetSecurityManager,
> > +                              getter_AddRefs(inetSecMgr))) ||
> > +      inetSecMgr->MapUrlToZone(PromiseFlatString(aURL).get(),
> > +                               &zone, 0) != S_OK ||
> > +      zone >= 5) {
> 
> A comment about why we are translating higher zone numbers to the Internet
> zone would be appreciated. Is there a reason not to just write the higher
> zone number to the ADS?

The zone number >=5 is just invalid. We should treat it as an unexpected error.

> Also, this logic could be moved to the JavaScript side.

OK.

Comment 7

[:Paolo Amadini]

Comment on attachment 8421773 [details] [diff] [review]
Respect Security Zone settings when the download manager marks a downloaded file

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

r+ with the changes below. Thanks for working on this!

::: toolkit/components/jsdownloads/public/mozIDownloadPlatform.idl
@@ +43,5 @@
> +  const unsigned long MY_COMPUTER = 0;
> +  const unsigned long INTRANET = 1;
> +  const unsigned long TRUSTED = 2;
> +  const unsigned long INTERNET = 3;
> +  const unsigned long RESTRICTED = 4;

Call these ZONE_MY_COMPUTER and so on.

@@ +44,5 @@
> +  const unsigned long INTRANET = 1;
> +  const unsigned long TRUSTED = 2;
> +  const unsigned long INTERNET = 3;
> +  const unsigned long RESTRICTED = 4;
> +  const unsigned long ZONE_MAX = 4;

Maybe ZONE_MAX_VALUE would be clearer.

::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp
@@ +162,5 @@
> +
> +  DWORD zone;
> +  if (inetSecMgr->MapUrlToZone(PromiseFlatString(aURL).get(),
> +                               &zone, 0) != S_OK) {
> +    *aZone = 3; // Default to Internet Zone

This should return NS_ERROR_UNEXPECTED as well.

Comment 8

[:Paolo Amadini]

Flagging this bug for Desktop QA verification once resolved, according to comment 0.

Comment 9

[:Paolo Amadini]

(In reply to Masatoshi Kimura [:emk] from comment #6)
> The zone number >=5 is just invalid. We should treat it as an unexpected
> error.

By the way, I'm not sure what this means. If higher zone numbers simply cannot exist, we probably could just trust the Windows API not to return an invalid zone number, and we can remove the check on our side entirely.

Comment 10

[Masatoshi Kimura [:emk]]

(In reply to :Paolo Amadini from comment #7)
> ::: toolkit/components/jsdownloads/public/mozIDownloadPlatform.idl
> @@ +43,5 @@
> > +  const unsigned long MY_COMPUTER = 0;
> > +  const unsigned long INTRANET = 1;
> > +  const unsigned long TRUSTED = 2;
> > +  const unsigned long INTERNET = 3;
> > +  const unsigned long RESTRICTED = 4;
> 
> Call these ZONE_MY_COMPUTER and so on.

OK.

> @@ +44,5 @@
> > +  const unsigned long INTRANET = 1;
> > +  const unsigned long TRUSTED = 2;
> > +  const unsigned long INTERNET = 3;
> > +  const unsigned long RESTRICTED = 4;
> > +  const unsigned long ZONE_MAX = 4;
> 
> Maybe ZONE_MAX_VALUE would be clearer.

Removed the constant because it is not used anymore.

> ::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp
> @@ +162,5 @@
> > +
> > +  DWORD zone;
> > +  if (inetSecMgr->MapUrlToZone(PromiseFlatString(aURL).get(),
> > +                               &zone, 0) != S_OK) {
> > +    *aZone = 3; // Default to Internet Zone
> 
> This should return NS_ERROR_UNEXPECTED as well.

OK.

(In reply to :Paolo Amadini from comment #9)
> By the way, I'm not sure what this means. If higher zone numbers simply
> cannot exist, we probably could just trust the Windows API not to return an
> invalid zone number, and we can remove the check on our side entirely.

Removed the check.

Try runs:
https://tbpl.mozilla.org/?tree=Try&rev=e8169326cb16
https://tbpl.mozilla.org/?tree=Try&rev=6ec25c032907

Landed with changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28e12f7f68be

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/28e12f7f68be

Comment 12

[Masatoshi Kimura [:emk]]

Comment on attachment 8421773 [details] [diff] [review]
Respect Security Zone settings when the download manager marks a downloaded file

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 952961
User impact if declined: Corporate users will see an extra dialog when they download a file from their Intranet.
Testing completed (on m-c, etc.): on m-c.
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: mozIDownloadPlatform.idl changed

Comment 13

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/50bfc782f73a

Comment 14

[Catalin Varga [QA][:VarCat]]

Verified as fixed on:

FF 31.0b9
Build id:20140710141843
OS: Win 8.1

For Restricted Site Zone.Identifier=4 for Local Intranet and Trusted Sites there is no Zone.Identifier and for the rest of the downloads Zone.Identifier=3

Comment 15

[Catalin Varga [QA][:VarCat]]

Verified as fixed on:

FF 32.0b1
Build id:20140722195627
OS: Win 8.1

For Restricted Site Zone.Identifier=4 for Local Intranet and Trusted Sites there is no Zone.Identifier and for the rest of the downloads Zone.Identifier=3