Closed Bug 1008778 Opened 6 years ago Closed 6 years ago

Download manager should respect Security Zone settings

Categories

(Toolkit :: Downloads API, defect)

All
Windows 8.1
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox31 + verified
firefox32 --- verified

People

(Reporter: emk, Assigned: emk)

References

Details

(Whiteboard: [qa!])

Attachments

(1 file, 2 obsolete files)

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.
Forgot to bump the iid.
Attachment #8420721 - Attachment is obsolete: true
Attachment #8420721 - Flags: review?(paolo.mozmail)
Attachment #8420723 - Flags: review?(paolo.mozmail)
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).
Tracking because we want ESR to be in good shape.
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.
Attachment #8420723 - Flags: review?(paolo.mozmail) → feedback+
(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.
Attachment #8420723 - Attachment is obsolete: true
Attachment #8421773 - Flags: review?(paolo.mozmail)
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.
Flagging this bug for Desktop QA verification once resolved, according to comment 0.
Flags: firefox-backlog+
Whiteboard: [qa+]
Attachment #8421773 - Flags: review?(paolo.mozmail) → review+
(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.
(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
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/28e12f7f68be
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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
Attachment #8421773 - Flags: approval-mozilla-aurora?
Attachment #8421773 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
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
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.