483192 - Downloaded files don't inherit NTFS compression or encryption properties from parent folder/directory

Status: NEW

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

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

+++ This bug was initially created as a clone of Bug #224692 +++

Given by bug 224692 comment 92 the encryption properties from the parent folder aren't inherited when downloading a file. That happens because after the download completes, the file is being moved from the download temp directory and the encryption properties aren't updated.

Steps to Reproduce:
2. Create a folder on the same partition where the temp directory resists (should be an encrypted volume)
2. If not done yet, add the encrypted flag in the folder properties.
3. Download a file to that directory and see that it doesn't inherit the setting.

Comment 1

[Peter Dey]

Somewhat related:
You can't download files to a partition that doesn't support encryption if your Temp directory is encrypted.  This is presumably because the downloaded file in the Temp folder is encrypted, but when Firefox tries to move it, Windows throws up an exception.

Steps to Reproduce:
1. Encrypt your temp directory (e.g., my whole profile directory is encrypted, with inherit)
2. Download a file to a non-encryptable drive (e.g, FAT32, or a Samba share in my case)
3.  Firefox throws the error:
    X:\file.ext could not be saved, because an unknown error occurred.
    Try saving to a different location.

Comment 2

[Oliver Schoett]

Confirming this bug (downloaded files not encrypted) for Firefox 3.5.1 and SeaMonkey 2.0b1 on German Windows XP SP3.

Comment 5

[sillygates]

The .part file is never encrypted. When the .part file is moved to the final filename overwriting the 0 byte placeholder file, the file is still not encrypted.

The .part should be encrypted at all times to prevent leaking of sensitive data.

Also, this is a serious issue, and puts Firefox in violation of several corporate security policies.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So is this happening because the .part file is initially created in a different directory, one that's not encrypted?

Comment 7

[sillygates]

Unlikely. I haven't looked at the code, but my guess is that firefox sets
special permissions on the .part file when it is made, overriding the inherited
permissions from the directory (possibly to set the file as readonly/etc).

If I remember right, in windows, you actually have to get an ACL object
representing the file permissions and manipulate the permissions manually on
it, and then pass that object in to the function when you create the file.

Comment 8

[Brian R. Bondy [:bbondy]]

Boris is correct in Comment 6.  The encryption (and compression) defaults are inherited at creation time from the file's parent directory with NTFS volumes in Windows. 

We use the user's temp directory for the .part file before moving it to the download location.  We could set the encryption [1] or compression [2] after the file is downloaded, but this can be a lengthy blocking operation without using overlapped IO.  And people typically want to use the downloaded file right after it is downloaded anyway.  The temp dir and the destination dir may be of different filesystems too and hence diff encryption and compression capabilities.  So doing it beforehand wouldn't always be possible.  Doing it after would probably cause jank.

I'm not sure of the history on why we don't just create the .part file in the destination directory and then rename to the destination path in the same directory.   If we did that we could also remove the code from Bug bug 224692 in nsLocalFileWin. I think we just do it because that's how the channel code works with nsExternalAppHandler by calling nsExternalAppHandler::SetUpTempFile.

[1]:
To encrypt the file, you can do it at creation time w/ the FILE_ATTRIBUTE_ENCRYPTED flag to CreateFile.  You can also do it after, but it is extra IO and time with EncryptFile.  We can also use the hTemplateFile parameter and probably pass in a handle to the destination directory.

[2]:
You can compress a file after it is created with a call to DeviceIoControl w/ FSCTL_SET_COMPRESSION.  You can't set a file in a compressed state from the CreateFile call other than when it is inherited from the parent directory.  There is no CompressFile API but the IOCTL call is equivalent.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I'm not sure of the history on why we don't just create the .part file in the destination
> directory

The cases when we create the .part file are the cases when we start downloading due to receiving a response we can't handle internally.  Historically, what we would do then is prompt the user asking what to do, and keep downloading to the .part file.  Once the user picked "save" and the destination (note that before this we don't know the final destination!) we move the .part file there and keep writing to it.

If we now have cases when we know the download destination up front, I see no reason to not create the .part file directly there.

Comment 10

[Brian R. Bondy [:bbondy]]

Makes sense thanks for the history there.

There are 2 cases that we create the .part file.
1) The user right clicks and selects save as... and he can then select the location. In this case as you said we don't know the destination path.

2) The user clicks on a file like say a .zip file and he can then open it or save it.  In that case we do know the final destination before the dialog is shown and the user can't change it.

Comment 11

[Brian R. Bondy [:bbondy]]

Maybe people would see it as a bug though if open file saved the file to the destination directory directly in #2 though :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yeah, we definitely used to show a filepicker in case #2 back when the exthandler code was written.

And of course it's not clear to me that we want to put the temp file used when opening with an app in the Downloads directory.  Is that what comment 11 was about?

Comment 13

[Brian R. Bondy [:bbondy]]

No in Comment 11 I meant that if you select Open file then we don't move the file to the destination folder at all, we run it from the temp directory instead. 

Also I just remembered there is a preference for always asking where to save to, so looks like we can't rely on knowing the dest dir in any of the 2 cases.

I think to fix this we should add the ability to get the encryption status and compression status of a file inside nsLocalFileWin and then also add functions to compress and encrypt.  And we would call those from a different thread after they are downloaded from the download manager.

Comment 14

[Brian R. Bondy [:bbondy]]

or just leave it as is

Comment 15

[research.advocate]

If you compress the temp directory, downloaded files are still not saved in compressed format.

Comment 18

[Andrei Purice]

Hey Henrik,
Does this issue still occur for you or should we close it?

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I don't have Windows to test that but maybe someone else could do? I'm fairly sure that the behavior is still the same.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:Gijs, could you have a look please?

For more information, please visit BugBot documentation.

Comment 22

[:Gijs (he/him)]

(In reply to Brian R. Bondy [:bbondy] from comment #8)

Boris is correct in Comment 6. The encryption (and compression) defaults
are inherited at creation time from the file's parent directory with NTFS
volumes in Windows.

In Firefox with default settings, 12 years later, we put the temp file in the default download directory, just like the final file, so the inheritance would presumably be correct. However, the same issue could still occur for users who have set up "ask me every time" and pick a different location for an individual file.

We use the user's temp directory for the .part file before moving it to the
download location. We could set the encryption [1] or compression [2] after
the file is downloaded, but this can be a lengthy blocking operation without
using overlapped IO. And people typically want to use the downloaded file
right after it is downloaded anyway. The temp dir and the destination dir
may be of different filesystems too and hence diff encryption and
compression capabilities. So doing it beforehand wouldn't always be
possible. Doing it after would probably cause jank.

This sounds unappetizing given the trade-offs involved, but I don't know if, 12 years later, Windows has better ways of doing this. Either way I guess we need some windows widget support...

I'm not sure of the history on why we don't just create the .part file in
the destination directory

So we do this now, but only if the destination directory is known at download time. In the case of "always ask me where to save files", it isn't, because by the time we realize we've got a download on our hands and prompt the user for a file location, we already have an open channel with bytes flowing in. We have not tried to come up with some "clever" memory caching scheme that avoids having to write all this data to disk (and in any case, that would likely not always be sufficient given the ratios between available memory, internet speed, file size, and how quickly humans pick directories in the file picker).

and then rename to the destination path in the
same directory. If we did that we could also remove the code from Bug bug
224692 in nsLocalFileWin.

This code was surprisingly touched somewhat recently, in bug 1816694, by Bob. Bob, are you in a position to comment on this ticket? Are there ways to address the "the user chose somewhere else to save the file [and now we have to move/copy it and make sure it gets the right encryption/compression bits set]"? And/or is this perhaps wfm? I'm a bit disconcerted that these lines of code setting the security info have no code coverage, though the surrounding code has plenty. Perhaps this is just a function of where/how we're testing this code on infra?

I think we just do it because that's how the
channel code works with nsExternalAppHandler by calling
nsExternalAppHandler::SetUpTempFile.

[1]:
To encrypt the file, you can do it at creation time w/ the
FILE_ATTRIBUTE_ENCRYPTED flag to CreateFile. You can also do it after, but
it is extra IO and time with EncryptFile. We can also use the hTemplateFile
parameter and probably pass in a handle to the destination directory.

[2]:
You can compress a file after it is created with a call to DeviceIoControl
w/ FSCTL_SET_COMPRESSION. You can't set a file in a compressed state from
the CreateFile call other than when it is inherited from the parent
directory. There is no CompressFile API but the IOCTL call is equivalent.

Comment 24

[Ray Kraesig [:rkraesig]]

(In reply to :Gijs (he/him) from comment #22)

In Firefox with default settings, 12 years later, we put the temp file in the default download directory, just like the final file, so the inheritance would presumably be correct. However, the same issue could still occur for users who have set up "ask me every time" and pick a different location for an individual file.

It also occurs for all users, even with default settings, if they right-click a link and select Save Link As....

Conversely, it occurs for no users when using Ctrl+S or Save Page As..., presumably since we're guaranteed to have all the bytes in memory at that point and so don't need to open a temporary file. I assume it also doesn't happen when saving Blobs under "ask me every time".

We use the user's temp directory for the .part file before moving it to the
download location. We could set the encryption [1] or compression [2] after
the file is downloaded, but this can be a lengthy blocking operation without
using overlapped IO. And people typically want to use the downloaded file
right after it is downloaded anyway. The temp dir and the destination dir
may be of different filesystems too and hence diff encryption and
compression capabilities. So doing it beforehand wouldn't always be
possible. Doing it after would probably cause jank.

This sounds unappetizing given the trade-offs involved, but I don't know if, 12 years later, Windows has better ways of doing this. Either way I guess we need some windows widget support...

As far as I'm aware, the only easy way to do it is copy-after-completion.

One option which doesn't appear to have been considered would be to write to the temporary file, but -- once the permanent file location is known -- "move" the temporary file's contents into it block-by-block: that is, with the temporary file open as read+write, read it block-by-block from the top and write zeroes back to it concurrent with writing the buffered data to the final location, relying on sparse-file support in the underlying filesystem to eliminate memory pressure. If we ever catch up to the incoming data, we can discard the temporary buffer file entirely.

This would reduce jank, even in the merely-on-different-physical-volumes case, and so might even be worth generalizing to other OSes. (In many respects, moving between compressed/uncompressed or encrypted/unencrypted regimes is like moving between different physical volumes: ultimately the data has to go from source filesystem to RAM to destination filesystem.) It'd be a lot of moving parts, though, and difficult to test properly.

So we do this now, but only if the destination directory is known at download time. In the case of "always ask me where to save files", it isn't, because by the time we realize we've got a download on our hands and prompt the user for a file location, we already have an open channel with bytes flowing in.

Separately, it might make some sense to have a setting somewhere that disables that, even if only in about:config.

Comment 25

[:Gijs (he/him)]

(In reply to Ray Kraesig [:rkraesig] from comment #24)

(In reply to :Gijs (he/him) from comment #22)

So we do this now, but only if the destination directory is known at download time. In the case of "always ask me where to save files", it isn't, because by the time we realize we've got a download on our hands and prompt the user for a file location, we already have an open channel with bytes flowing in.

Separately, it might make some sense to have a setting somewhere that disables that, even if only in about:config.

What do you mean by "disables that"? What would it disable?

(if you mean "don't start the download before prompting the user"... we don't know it's a download. The page navigates to a new URL, and we make a network request. When the network request provides at least headers, we can process it and decide to load the new webpage or start a download (and potentially prompt the user), depending on Content-Type etc. I don't think there's a way around this where we don't have this conundrum, but I'm open to ideas if I'm missing something...)

Comment 26

[Ray Kraesig [:rkraesig]]

(In reply to :Gijs (he/him) from comment #25)

(if you mean "don't start the download before prompting the user"... we don't know it's a download. The page navigates to a new URL, and we make a network request. When the network request provides at least headers, we can process it and decide to load the new webpage or start a download (and potentially prompt the user), depending on Content-Type etc. I don't think there's a way around this where we don't have this conundrum, but I'm open to ideas if I'm missing something...)

Apologies; I had the Save Link As... case foremost in mind — possibly with some overlap from the functionally-very-similar case where there's a download attribute on an <a> element, which I haven't tested — and didn't think through the ordinary-navigation flow.

But, solely for the sake of completeness, I do have a disgustingly hacky idea for the ordinary-navigation case: if it turns out to be a download rather than a navigation, throttle it to something on the order of 1 byte/second until the user's selected a file, then flush what little we've collected so far to disk. I'm not actually proposing that, though — partly because I don't know how much control we have over the TCP connection versus how much is delegated to the OS; partly because I have no idea whether servers would react poorly to it (it might look like the opening stages of a DDoS attack); and partly because even in isolation it feels like an absurd abstraction-layer violation.

Comment 27

[Bob Owen (:bobowen)]

(In reply to :Gijs (he/him) from comment #22)
...

This code was surprisingly touched somewhat recently, in bug 1816694, by Bob. Bob, are you in a position to comment on this ticket? Are there ways to address the "the user chose somewhere else to save the file [and now we have to move/copy it and make sure it gets the right encryption/compression bits set]"? And/or is this perhaps wfm? I'm a bit disconcerted that these lines of code setting the security info have no code coverage, though the surrounding code has plenty. Perhaps this is just a function of where/how we're testing this code on infra?

I don't have a lot of expertise here and, without further investigation, don't have anything to add that helps unfortunately.
The aim of my change was to reduce the number of existing calls to that function, by not calling it when the permissions were already inherited from the parent.