517022 - Windows: Firefox does not tag downloaded files with Mark-of-the-Web if OPEN used instead of SAVE

Status: RESOLVED WORKSFORME

(Toolkit :: Downloads API, defect, P2)

Description

[Eric Lawrence]

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.2; WOW64; Trident/4.0; .NET CLR 3.5.30729; MS-RTC LM 8; InfoPath.2)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3

Firefox typically uses the Windows feature "Attachment Execution Services" to mark downloaded files with a "Mark of the Web" (MOTW) so that Windows knows that this file came from the Internet. This allows, for instance, the Windows Shell to apply additional security. Other applications may have different behavior when opening files from the Internet or other risky locations as well.

The problem is that Firefox only marks the file with the MOTW if the user chooses "SAVE" in the file download dialog. If the user instead chooses "OPEN", the file is opened from the temporary folder without the MOTW being written to the Alternate Data Stream. This means that the client application will not be able to determine that this content was downloaded from the Internet.

Reproducible: Always

Steps to Reproduce:
1. Download a document, e.g. http://www.ericlawrence.com/test/test.doc
2. Choose "Open" in the file download handler
Actual Results:  
View the downloaded file in the Windows shell. Right-click and choose "Properties".  Note that text "Security: This file came from another computer and might be blocked to help protect this computer" is missing, as is the "Unblock" button. 

Alternatively, use the Streams.exe tool to examine the file and note that the Zone.Identifier alternative data stream is missing from the file.

Expected Results:  
The file is marked with the Mark of the Web in the download case as well.

Comment 1

[Jim Mathies [:jimm]]

Will look into this this week, we might want this block 1.9.2.

Comment 2

[Daniel Veditz [:dveditz]]

Would be nice to fix in 3.5.x as well once we've figured it out.

Comment 3

[Jim Mathies [:jimm]]

Ok, so some notes on this. The downloaded file is handled by the oshelperappservice. It executes the desired action on the file after receiving the stop request event on the download. It does communicate with the download manager, but the dm is a passive player in all of this, acting more for display purposes than anything else. The steps we take look like this:

1) helper app service initiates the download
2) h.a.s. communicates download to dm / dm window
3) download completes, h.a.s. calls lauchwithfile on the doc, launching the handler (in this case Word)
4) Word places a write lock on the file.
5) The dm scans the file using IAE, which succeeds. (IAE return S_OK for it's Save call.) However IAE fails to write the alternative data to the file due to the write lock.

Results if the file is a suspect virus or violates security policy is unknown, but it's probably not very pretty.

Either h.a.s. needs to wait for the download manager to finish the scan, which doesn't look promising, or we can double scan the file by calling into the scanner from h.a.s. before launching the helper. In the latter case the issue is that IAE can block the calling thread for UI, which might cause problems.

I'll do some experimenting to see what we can work out here. The risks are that we have good support for virus scanning and security policy, but in this case, we basically jump around it. IAE is called on the file, but after the handler is launched. That's a pretty bad situation to be in IMO.

Comment 4

[Jim Mathies [:jimm]]

one touch up note - it's not the os helper app service, it's the external helper app service that's handling the the file.

Comment 5

[Jim Mathies [:jimm]]

(In reply to comment #3)
> Either h.a.s. needs to wait for the download manager to finish the scan, which
> doesn't look promising, or we can double scan the file by calling into the
> scanner from h.a.s. before launching the helper. In the latter case the issue
> is that IAE can block the calling thread for UI, which might cause problems.


It looks like the best solution here would be to pull the scanner out and make it a service, and give it the responsibility of handling the file after the scan is complete. That information could be handed to it through the helper app service pretty easily. 

Interaction with the dm is a little more sketchy, the scanner needs to communicate display information to the dm window, and we should avoid double scanning the file. The scan result would need to be handed to the dm via something similar to the progress updates. Not sure sure yet how that'll be worked out.

Comment 6

[Benjamin Smedberg]

This is not going to make FF4 final.

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I've spent some time wrapping my head around the relevant code and Jim's analysis from above.  Not that this bug needs another summary, but here are my notes thus far:

`nsExternalAppHandler::LaunchWithApplication` in uriloader\exthandler\nsexternalhelperappservice.cpp
  - Gets called by the helper app dialog which allows the user to say "launch with application" or "save to disk"
  - (the analagous call if the user selects "save" is `nsExternalAppHandler::SaveToDisk`)
  - Calls `nsExternalAppHandler::CreateProgressListener` which initializes the download and calls `nsExternalAppHandler::SetWebProgressListener`
  - If the download finishes during the call to `nsExternalAppHandler::SetWebProgressListener` it will call `nsExternalAppHandler::ExecuteDesiredAction`
  - Otherwise `nsExternalAppHandler::ExecuteDesiredAction` gets called in `nsExternalAppHandler::OnStopRequest`

`nsExternalAppHandler::ExecuteDesiredAction`
  - Calls `nsExternalAppHandler::OpenWithApplication` which causes the file to be opened
  - Note that at this point we've already launched an app and opened the file, regardless of any virus checking that is about to happen
  - `nsExternalAppHandler::ExecuteDesiredAction` then calls `mWebProgressListener->OnStateChange` with arguments indicating the download has finished

`nsDownload::OnStateChange` in toolkit\components\downloads\nsDownloadManager.cpp
  - Calls `nsDownload::SetState(nsIDownloadManager::DOWNLOAD_SCANNING)`
     - Which then calls into the download scanner with this call: `mDownloadManager->mScanner->ScanDownload(this)`

`nsDownloadScanner::ScanDownload` in toolkit\components\downloads\nsdownloadscanner.cpp
  - Creates a new `Scan` object: `new Scan(this, download)`
  - Calls that object's `Start` function which starts a new thread with the theadfxn `nsDownloadScanner::ScannerThreadFunction`
  - `ScannerThreadFunction` calls other functions
    - (`nsDownloadScanner::Scan::DoScan` and `nsDownloadScanner::Scan::DoScanAES`)
    - These perform the actual scan then dispatch the `nsDownloadScanner::Scan` object to the main thread

 In the main thread, the `nsDownloadScanner::Scan` object is dispatched by calling its `Run` function
  - This function first waits for the thread to exit, then closes its handle
  - It then calls `mDownload->SetState(nsIDownloadManager::DOWNLOAD_FINISHED)`
  - `nsDownload::SetState` calls `nsDownload::ExecuteDesiredAction` which does nothing because of this check: `if (!mTempFile || !WasResumed()) return NS_OK;`

My first thought was that, no matter what our final solution is, we can't be calling `nsExternalAppHandler::OpenWithApplication` before doing our virus checking so I skipped that call using a breakpoint.  I was curious to see whether `nsDownload::ExecuteDesiredAction` could launch the external application, so I skipped the check that caused it to return immediately.  It calls into `nsDownload::OpenWithApplication`.

`nsDownload::OpenWithApplication`
  - First attempts to call `MoveTempToTarget` which seems confused because it uses the same filename for both the source and detination files
  - Skipping that call, we launch the file (`mMIMEInfo->LaunchWithFile(target))` and then set up deletion of the temporary file on exit

The net result of skipping three lines of code (`nsExternalAppHandler::OpenWithApplication`, `if (!mTempFile || !WasResumed()) return NS_OK;`, and `nsDownload::MoveTempToTarget`) was that the file was launched *after* the virus checking had already happened, so MOTW was applied (verified by looking at the "security" tab of the file properties).

I believe this is a reasonable first step toward a patch as long as we don't mind letting the nsDownload object use its own OpenWithApplication function to launch the file instead of having nsExternalAppHandler do that.

Any feedback/comments/questions/concerns on any of the above is very much appreciated.  I'll start writing an initial patch this week so there's something more concrete to look at.

Comment 8

[Jim Mathies [:jimm]]

Allowing the downloads manager to do the open sounds reasonable. I believe nsDownload::OpenWithApplication is usually triggered by user action from within the downloads window, but I could be wrong about that.

cc'ing Shawn who overhauled the download manager a while back and jst who owns the ext svc code.

Comment 9

[Shawn Wilsher :sdwilsh]

Open usually goes to nsExternalAppHandler::OpenWithApplication unless we pause and then resume the download.

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Given that in the pause/resume case we allow nsDownload::OpenWithApplication to perform the launch, I think it makes sense to allow it to perform the launch in the regular (non pause/resume) case.  Let me know if there's a reason not to use this approach.

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Minimal patch - fixes MOTW issue and ensures file is scanned for viruses BEFORE being launched

In addition to the MOTW issue in the title, there's another issue with this section of code that has been mentioned implicitly here but I think deserves to be discussed.  In the case that a user selects "open" instead of "save to disk," we are launching the file *before* doing any virus checking.  This seems like a huge security concern, as I think users regularly "open" document types they recognize, and those documents can harbor viruses that will go undetected until after it is too late.

In the attached patch, I attempted to modify as little code as possible while still fixing the MOTW and virus checking security concerns.

As a separate issue, I'd like to explore why there seems to be so much duplication of effort between nsDownload and nsExternalHelperAppService (for example, they both have OpenWithApplication functions that seem to accomplish the same task), but I'd really like to patch up the security holes first.

I'm still testing the patch to make sure it doesn't cause any regressions, but please let me know if you see anything obviously wrong with it.

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Oops, I attached a broken patch ("save to disk" doesn't work).  Will update momentarily.

Comment 13

[Jim Mathies [:jimm]]

I don't see any other calls to OpenWithApplication() in nsExternalHelperAppService independent of the call you removed. Maybe that can be taken out completely?

Comment 14

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #13)
> I don't see any other calls to OpenWithApplication() in
> nsExternalHelperAppService independent of the call you removed. Maybe that
> can be taken out completely?

comm-central looks clean as well. 

http://mxr.mozilla.org/comm-central/search?string=OpenWithApplication

I wonder if anyone is using that call?

Comment 15

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Updated minimal patch

Sorry for the delay: I ran into a slight complication over when exactly the temporary file should be moved to the target location.  The responsibility for doing so seems to have been shared between nsDownload and nsExternalAppHandler, and when the nsDownloadScanner started scanning it expected the file to already be in its target location.  I changed this patch to place the responsibility solely on nsDownload.  If the scanner is enabled, then the temp file gets moved to the target location just before the scan occurs, and if the scanner is disabled then the nsDownload moves the temp file to the target location when it originally would have done so.

Jim: I imagine that nsExternalAppHandler::OpenWithApplication can be removed without issue, and there's potential for removing other code from nsExternalAppHandler as well since functionality is duplicated in nsDownload.  For the purpose of keeping this patch simple and focused on the security issues, would it make sense to clean up sExternalHelperAppService in a separate patch?

In general, this fix and the subsequent cleanup changes are moving responsibilty from nsExternalAppHandler to nsDownload.  So far that seems innocuous, but I'd like to get more input from people who are familiar with the motivations behind nsExternalAppHandler

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to Tim Abraldes from comment #11)
> In addition to the MOTW issue in the title, there's another issue with this
> section of code that has been mentioned implicitly here but I think deserves
> to be discussed.  In the case that a user selects "open" instead of "save to
> disk," we are launching the file *before* doing any virus checking.  This
> seems like a huge security concern, as I think users regularly "open"
> document types they recognize, and those documents can harbor viruses that
> will go undetected until after it is too late.
I'm about 95% sure there's a bug already filed on the virus scanning stuff.  Previously it would have been very difficult to fix, and most virus scanners still scan it when you open it anyway, so there wasn't much of a need to fix it.

> As a separate issue, I'd like to explore why there seems to be so much
> duplication of effort between nsDownload and nsExternalHelperAppService (for
> example, they both have OpenWithApplication functions that seem to
> accomplish the same task), but I'd really like to patch up the security
> holes first.
Mostly because one lives in toolkit, and the other in core, and we couldn't share code.  It's possible that has now changed.  The two code paths should be kept in sync, although people like to sneak stuff in on occasion.  bz or biesi may be able to shed more light on that.

Comment 17

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes from comment #15)
> For the purpose of keeping this patch simple and focused on the
> security issues, would it make sense to clean up sExternalHelperAppService
> in a separate patch?

Yes.

Comment 18

[Jim Mathies [:jimm]]

(In reply to Shawn Wilsher :sdwilsh from comment #16)
> > As a separate issue, I'd like to explore why there seems to be so much
> > duplication of effort between nsDownload and nsExternalHelperAppService (for
> > example, they both have OpenWithApplication functions that seem to
> > accomplish the same task), but I'd really like to patch up the security
> > holes first.
> Mostly because one lives in toolkit, and the other in core, and we couldn't
> share code.  It's possible that has now changed.  The two code paths should
> be kept in sync, although people like to sneak stuff in on occasion.  bz or
> biesi may be able to shed more light on that.

Toolkit should be able to share nsExternalHelperAppService code shouldn't it? From my understanding it's toolkit that's optional, not the helper service.

Comment 19

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes from comment #15)
> Created attachment 553322 [details] [diff] [review]
> Updated minimal patch

I ended up with an exception with these changes, where mTempFile was null in MoveTempToTarget:


>	xul.dll!nsDownload::MoveTempToTarget()  Line 2786	C++
 	xul.dll!nsDownload::SetState(short aState=7)  Line 2180 + 0xb bytes	C++
 	xul.dll!nsDownload::OnStateChange(nsIWebProgress * aWebProgress=0x00000000, nsIRequest * aRequest=0x0b84d81c, unsigned int aStateFlags=327696, unsigned int aStatus=0)  Line 2549	C++
 	xul.dll!nsWebBrowserPersist::OnStopRequest(nsIRequest * request=0x0b84d81c, nsISupports * ctxt=0x00000000, unsigned int status=0)  Line 800	C++
 	xul.dll!nsStreamListenerTee::OnStopRequest(nsIRequest * request=0x0b84d81c, nsISupports * context=0x00000000, unsigned int status=0)  Line 71 + 0x28 bytes	C++
 	xul.dll!nsHttpChannel::OnStopRequest(nsIRequest * request=0x0b810a08, nsISupports * ctxt=0x00000000, unsigned int status=0)  Line 4213	C++
 	xul.dll!nsInputStreamPump::OnStateStop()  Line 579	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x0bd73db0)  Line 403 + 0xb bytes	C++
 	xul.dll!nsInputStreamReadyEvent::Run()  Line 115	C++

Comment 20

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v3 - Fixed issue where multiple instances of launched app would appear

Thanks for taking some time to debug with me earlier, Jim.  I believe this patch should fix the issues that cropped up.  The problem was that I had mistakenly modified logic in a "case" to fall through on success, when it was only supposed to fall through on failure.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 553567 [details] [diff] [review]
Patch v3 - Fixed issue where multiple instances of launched app would appear

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

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +2178,5 @@
>      case nsIDownloadManager::DOWNLOAD_SCANNING:
>      {
> +      nsresult rv = MoveTempToTarget();
> +      if (NS_SUCCEEDED(rv))
> +        rv = mDownloadManager->mScanner ? mDownloadManager->mScanner->ScanDownload(this) : NS_ERROR_NOT_INITIALIZED;

nit - Wrap to 80 character lines.

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ -2019,5 @@
> -      }
> -      else if(action == nsIMIMEInfo::saveToDisk)
> -      {
> -        nsCOMPtr<nsILocalFile> destfile(do_QueryInterface(mFinalFileDestination));
> -        gExtProtSvc->FixFilePermissions(destfile);

We need to figure out what this is doing on unix and make sure the download manager is copying this behavior.

Comment 22

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v4 - Ensure that "FixFilePermissions" still happens on Unix and Mac

One thing we could do is expose `FixFilePermissions` from the external helper app service and call it from the `nsDownload` code using something like this:
     nsCOMPtr<nsPIExternalAppLauncher> extHelperAppService(
                        do_GetService
                        (NS_EXTERNALHELPERAPPSERVICE_CONTRACTID));
     if (extHelperAppService)
       (void)extHelperAppService->FixFilePermissions(target);


However, since `FixFilePermissions` is only called from the line I removed, it probably makes more sense to just put this at the end of `nsDownload::MoveTempToTarget`:
    #if defined (XP_MACOSX) || defined (XP_UNIX)
      mode_t mask = umask(0777);
      umask(mask);
      PRUint32 permissions = 0666 & ~mask;
      target->SetPermissions(permissions);
    #endif

Attached is a patch exhibiting this approach.  I'll test/tryserver and mark for review tomorrow (unless there are comments/concerns)

Comment 23

[Jim Mathies [:jimm]]

You can probably remove all the code associated with FixFilePermissions since it's not in use anymore. Biesi landed that original work, so maybe he can comment on that when you seek reviews. 

https://bugzilla.mozilla.org/show_bug.cgi?id=124307

Comment 24

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v5 - Fixed compilation issue on Unix and Mac

The Linux, Mac, and "DOWNLOAD_SCANNER not defined" codepaths haven't been tested yet, but I believe this patch will fix our issues with minimal changes to the code and hopefully no regressions.  I have a separate cleanup patch for removing unused functions that I will attach momentarily.

Comment 25

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Cleanup patch (removes unused code)

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to Shawn Wilsher :sdwilsh from comment #16)
> > As a separate issue, I'd like to explore why there seems to be so much
> > duplication of effort between nsDownload and nsExternalHelperAppService (for
> > example, they both have OpenWithApplication functions that seem to
> > accomplish the same task), but I'd really like to patch up the security
> > holes first.
> Mostly because one lives in toolkit, and the other in core, and we couldn't
> share code.  It's possible that has now changed.  The two code paths should
> be kept in sync, although people like to sneak stuff in on occasion.  bz or
> biesi may be able to shed more light on that.

Well also ExternalAppHandler predates the download manager. And part of the reason for this code being the way it is is that people embedding Gecko would still want to/have to use nsExternalHelperAppService but provide their own progress dialog and/or download manager. So I'm not too happy with removing the Open functionality from the externalapphandler...

Comment 27

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 554524 [details] [diff] [review]
Patch v5 - Fixed compilation issue on Unix and Mac

Hmm... In that case I'll investigate alternative solutions.

I'm going to be busy with other items next week so I may not have a meaningful status update until after the All-Hands.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Yeah, comment 26 is spot-on in terms of this code.

Now it would be _really_ nice to share the code somehow.  I'm 100% behind an effort to do that!

Comment 29

[Shawn Wilsher :sdwilsh]

(In reply to Shawn Wilsher :sdwilsh from comment #16)
> I'm about 95% sure there's a bug already filed on the virus scanning stuff. 
> Previously it would have been very difficult to fix, and most virus scanners
> still scan it when you open it anyway, so there wasn't much of a need to fix
> it.
Found it: bug 561089

Comment 30

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I've been staring at this for a little while now so it's probably time to record some thoughts.  First, some context (just so I can keep this all straight in my head): 
  `nsExternalHelperAppService`
    - Provided by Gecko
    - Provides "open with application" and "save to disk" functionality
    - Notifies a `nsIWebProgressListener` of download progress

  Download manager (`nsDownloadManager`, `nsDownload`, etc)
    - Part of toolkit
    - Receives progress updates through `nsIWebProgressListener` interface
    - Provides pause/resume functionality (and code duplication exists because, if a download is paused, the `nsExternalHelperAppService` disavows all knowledge so the download manager has to do exactly what the `nsExternalHelperAppService` would have done)
    - Uses `nsDownloadScanner` to scan downloads when it is notified that downloads have completed

As Jim suggested in comment #5, we could have `nsExternalHelperAppService` call into (a modified) `nsDownloadScanner` before it attempts application launch.  If the scan is successful, it will perform the launch and notify its `nsIWebProgressListener` that the download has completed.  That change would fix this bug and bug 561089.

Jim pointed out (also in comment #5) that the download manager would need to receive progress updates indicating that a virus scan is taking place and the success or failure of that scan.  I think we can resolve that issue like so:
  - Create a new interface called `nsIScannerListener` or similar for receiving virus/malware scan state updates
  - Make `nsDownload` and `nsExternalHelperAppService` implement the new interface
  - `nsExternalHelperAppService` will attempt to QI its `nsIWebProgressListener` to `nsIScannerListener`
  - If it succeeds, `nsExternalHelperAppService` will forward along status updates as it receives them from `nsDownloadScanner`
  - Otherwise, it will just notify through the `nsIWebProgressListener` interface of success/failure based on the result of the scan

I have a grand total of 2 months of Mozilla/XPCOM development experience, so if these are crazy ideas please let me know

Comment 31

[Shawn Wilsher :sdwilsh]

That seems totally reasonable and means we could actually rewrite the download manager code in JS.

Comment 32

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I should have time to pick this up after next week, but I'm deassigning myself in the meantime to indicate that I'm not currently working on it.

Comment 33

[Eric Lawrence]

This does not reproduce in Firefox 48.0a1