1980886 - Download keeps Windows permissions from default download directory when moved to target directory

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[andrej.wolkow]

Steps to reproduce:

Save a file on a network share which has different permissions compared to the configured download dialog. For example you have a User Home and a shared folder on the same fileserver but different folders. User home has a permission group for users and the shared folder has different permission for shared users.

Actual results:

The permissions are copied to the shared folder because of probably firefox saving the file via UNC path and moving the downloaded file from download dir to shared folder.
https://learn.microsoft.com/en-us/troubleshoot/windows-client/windows-security/permissions-on-copying-moving-files

Expected results:

Download the file to the destination and take the permissions of the destination. Don't copy permissions from the temporary download directory.
Either use the defined network Drive or copy and delete.
Tested the same in Edge. There it does work.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Firefox::Downloads Panel' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Negritas Sergiu, Desktop QA]

Hello! I have tried to reproduce the issue with firefox 143.0a1(2025-08-12) on Ubuntu 22.04 and Windows 10, unfortunately I wasn't able to reproduce the issue.
Could you please answer the following questions in order to further investigate this issue

1. Does this issue happen with a new profile? Here is a link on how to create one: https://support.mozilla.org/en-US/kb/profile-manager-create-remove-switch-firefox-profiles
2. Could you please attach a screen recording for the issue?

Comment 3

[:Gijs (he/him)]

How exactly are you downloading from the shared folder? Is this also over file:/// or something? How is the folder shared? I'm guessing we're talking Windows and/or SAMBA sharing but it's not 100% clear from comment 0. It would also be helpful to outline in more detail what kind of permissions you're talking about - is this all in an enterprise system with multi-user access, or whether files are marked read-only, or both?

Comment 4

[andrej.wolkow]

To simplify it I have done this test.
For Example: I have set the default download folder in Firefox to H:\test and to always ask where to save the file
Then i created a new Folder H:\test2. At this stage Both folders have the same permissions.
Now I disable inheritance and remove some of the test folder specific permissions (home read and modify group) and add a different group to be able to write in the test2 folder (new group with full permission as example)
Then i download for example this windows update https://catalog.s.download.windowsupdate.com/d/msdownload/update/software/updt/2025/05/kb5059072_microsoft.systemcenter.orchestrator.webconsole_x64_27d2c518c428936e006c7cb2a1709aa9c02f5995.cab (it doenst matter what it is)

Result: The file has gotten the previously deleted permssions which are still set on test folder, probably due to firefox moving the tmp file via UNC as stated in the microsoft article. Therefore the permissions are copied.

Comment 5

[andrej.wolkow]

Comment correction:
To simplify it I have done this test.
For Example: I have set the default download folder in Firefox to H:\test and to always ask where to save the file.
Then i created a new Folder H:\test2. At this stage Both folders have the same permissions.
Now I disable inheritance.
Remove some of the test folder specific permissions (home read and modify) on folder test2
Add a different group to be able to write in the test2 folder (new group with full permission as example)
Download for example this windows update https://catalog.s.download.windowsupdate.com/d/msdownload/update/software/updt/2025/05/kb5059072_microsoft.systemcenter.orchestrator.webconsole_x64_27d2c518c428936e006c7cb2a1709aa9c02f5995.cab (it doesn't matter what it is)

Result: The file has gotten the previously deleted permssions from the test folder copied. Probably due to firefox moving the tmp file via UNC as stated in the microsoft article. Therefore the permissions are copied.

Comment 6

[:Gijs (he/him)]

The file move/copy abstraction layer gecko applications use has no mechanism for the downloads code to say "move this file and have it not keep its permissions". There also appears to be some ancient history in bug 224692, and more recently in bug 1816694.

I've never had to use Windows in an enterprise environment where I need to care about permissions, so this bug is still not entirely clear to me, but hopefully we can get to the bottom of it.

(In reply to andrej.wolkow from comment #5)

Comment correction:
To simplify it I have done this test.
For Example: I have set the default download folder in Firefox to H:\test and to always ask where to save the file.
Then i created a new Folder H:\test2. At this stage Both folders have the same permissions.
Now I disable inheritance.

To be clear, where are you disabling inheritance? In H:\test or H:\test2 or both? How does this affect the outcome (i.e. if test2 has inheritance enabled, do you get the expected results? And why would you disable permission inheritance?)

Comment 7

[andrej.wolkow]

I disable the inheritance in H:\test2. This is just to simplify the test to be able to remove the permission. In the default scenario I have 2 different permissions and enabled inheritance.

Comment 8

[:Gijs (he/him)]

(In reply to andrej.wolkow from comment #7)

I disable the inheritance in H:\test2. This is just to simplify the test to be able to remove the permission. In the default scenario I have 2 different permissions and enabled inheritance.

In that case it sounds like bug 1816694 would have broken the behaviour here, as we avoid manually (re-)applying permissions if permission inheritance is turned on on the parent folder (at least, that seems to be what the patch description says). It's also possible that there's a difference between moves and copies here. Bob, can you take a look at this bug, please? :-)

Comment 9

[Bob Owen (:bobowen)]

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

(In reply to andrej.wolkow from comment #7)

I disable the inheritance in H:\test2. This is just to simplify the test to be able to remove the permission. In the default scenario I have 2 different permissions and enabled inheritance.

In that case it sounds like bug 1816694 would have broken the behaviour here, as we avoid manually (re-)applying permissions if permission inheritance is turned on on the parent folder (at least, that seems to be what the patch description says). It's also possible that there's a difference between moves and copies here. Bob, can you take a look at this bug, please? :-)

The change in bug 1816694 was to not set the file to inherit when it is already inheriting.
This was for performance and to prevent audit messages in certain configurations.
I don't see how that could cause this issue, from a permissions point of view the patch shouldn't have changed anything (unless there is a bug).

Comment 10

[Bob Owen (:bobowen)]

(In reply to Bob Owen (:bobowen) from comment #9)

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

(In reply to andrej.wolkow from comment #7)

I disable the inheritance in H:\test2. This is just to simplify the test to be able to remove the permission. In the default scenario I have 2 different permissions and enabled inheritance.

In that case it sounds like bug 1816694 would have broken the behaviour here, as we avoid manually (re-)applying permissions if permission inheritance is turned on on the parent folder (at least, that seems to be what the patch description says). It's also possible that there's a difference between moves and copies here. Bob, can you take a look at this bug, please? :-)

The change in bug 1816694 was to not set the file to inherit when it is already inheriting.
This was for performance and to prevent audit messages in certain configurations.
I don't see how that could cause this issue, from a permissions point of view the patch shouldn't have changed anything (unless there is a bug).

narrator: there was a bug

Looks like setting to inherit in the way we do removes previous access as well.
So, the detection of the existing settings needs to check all ACEs.

Comment 11

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

Set release status flags based on info from the regressing bug 1816694

Comment 12

[:Gijs (he/him)]

Thanks Bob!

Comment 13

[Bob Owen (:bobowen)]

Looks like we should be able to use GetInheritanceSourceW to work out if we have left over inherited ACEs from before the move.
I have it working, but I want to look into how expensive it might be.

Comment 14

[Bob Owen (:bobowen)]

Unfortunately GetInheritanceSourceW seems particularly expensive.
On my test number of system calls:
GetInheritanceSourceW: 409
GetNamedSecurityInfoW: 17
SetNamedSecurityInfoW: 27

So my plan is to count the number of object inherit ACEs in the parent DACL and the number of inherited ACEs in the moved file and call SetNamedSecurityInfoW if they don't match.

Comment 15

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

Set release status flags based on info from the regressing bug 1816694

Comment 16

[Bob Owen (:bobowen)]

Attachment: Bug 1980886 p3 - Ensure no old inherited ACEs remain when moving files on Windows. r=yjuglaret!

Comment 17

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/jobs?repo=try&revision=e4d217971bfc8adec1befd54bfa5ab887dabb91c

Comment 18

[Bob Owen (:bobowen)]

After Yannis pointed out that this code can be used to move dirs as well, I tried to implement an expected inherited ACE count for them.
While trying this I realised the rules for doing this are more complicated even for non-dirs and not documented.

However, I have found functions for creating new inherited ACLs from a parent, so these should follow the same or similar rules and give the same inherited ACE count.
They don't appear to need any system calls as all the information is already in the security descriptors.

Should have new patches up soon.

Comment 19

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=Q36v26qsR3uYd6tmA7IRlA.0&revision=433f8bd5e44aa374f149a24b4af73be928bd42be

Comment 20

[Bob Owen (:bobowen)]

Attachment: Bug 1980886 p1 - Refactor CopySingleFile to better reflect its usage and behaviour. r=yjuglaret!

Comment 21

[Bob Owen (:bobowen)]

Attachment: Bug 1980886 p2 - Provide a forward_iterator for accessing the ACE_HEADERs in an ACL. r=yjuglaret!

Comment 22

[Bob Owen (:bobowen)]

Attachment: fixup! Bug 1980886 p2 - Provide a forward_iterator for accessing the ACE_HEADERs in an ACL. r=yjuglaret!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6def2987b48b
https://hg.mozilla.org/integration/autoland/rev/25b264ceed3d
p1 - Refactor CopySingleFile to better reflect its usage and behaviour. r=yjuglaret
https://github.com/mozilla-firefox/firefox/commit/21c8e2c40719
https://hg.mozilla.org/integration/autoland/rev/e1b091b14517
p2 - Provide a forward_iterator for accessing the ACE_HEADERs in an ACL. r=yjuglaret,win-reviewers,gstoll
https://github.com/mozilla-firefox/firefox/commit/ac562564b2c8
https://hg.mozilla.org/integration/autoland/rev/7e19c89e818b
p3 - Ensure no old inherited ACEs remain when moving files on Windows. r=yjuglaret

Comment 24

[Yannis Juglaret [:yannis]]

I'm asking for landing on behalf of :bobowen, with his agreement. Since :bobowen will be out for the next two weeks, please contact me rather than him for anything related to these patches during that time frame. Temporarily stealing the assignee token accordingly.

Comment 25

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/25b264ceed3d
https://hg.mozilla.org/mozilla-central/rev/e1b091b14517
https://hg.mozilla.org/mozilla-central/rev/7e19c89e818b