Closed Bug 224692 Opened 21 years ago Closed 16 years ago

Downloaded files don't inherit NTFS permissions from the parent folder/directory

Categories

(Core Graveyard :: File Handling, defect)

Product:
Core Graveyard
Component:
File Handling
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.1a2

People

(Reporter: lucasz, Assigned: anowack)

References

(Blocks 2 open bugs)

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 2 obsolete files)

Permissions Fix v2 (fixed white-space)
1.58 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
Updated Unit Test
11.56 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
Fix non-inherited permissions
1.44 KB, patch
benjamin
: review+
benjamin
: superreview+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7

When I download a file, the NTFS properties of the parent folder aren't
inherited.  Things like permissions, compression, encryption.  My guess is that
after the download completes, the file is being moved from the download temp
directory.  Instead, it should be copied and the temp file deleted.  By copying,
the file will inherit NTFS Properties.

Reproducible: Always

Steps to Reproduce:
1.Create a folder on the same partition as the mozilla temp directory (C Drive
Usually)
2. Make the folder compressed and give it some specific permissions.
3. Download a file to that directory and see that it doesn't inherit.

Actual Results:  
The file comes out uncompressed and the permissions applied to the parent folder
don't apply.

Expected Results:  
The downloaded file should be compressed and have the same permissions as the
parent folder.
ben, does the file get copied or moved from the temp location?  this one would
be a dealbreaker for security-conscious organizations.

-> major, has security implications on a file level.
Assignee: blake → bugs
Severity: normal → major
> Instead, it should be copied 

no, it shouldn't, that would be pretty slow

how did you download? By clicking "save link target as", or by getting asked
what you want to do with the file? if the latter, this would not be a
firebird-specific bug.
Sounds to me like NTFS is pretty buggy if copy/delete has different final
results from move...
Blocks: 129923
Depends on: 55690
fwiw this would work if $TEMP is on a different volume, according to this msdn
description of MoveFile:
" If a file is moved across volumes, MoveFile does not move the security
descriptor with the file. The file will be assigned the default security
descriptor in the destination directory."

SHFileOperation would also allow us to do what comment 0 requests:
FOF_NOCOPYSECURITYATTRIBS
    Version 4.71. Do not copy the security attributes of the file.
It doesn't matter if I click on a link to a file and it prompts me to download 
or if I do it through a right click.  The results are the same.  Regarding the 
comment made on the design of NTFS, it's not a bug.    If you think about it, 
it all makes perfect sense.  When you move a file to a different partition, it 
actually copies the whole file over, then deletes it from the index table on 
the source partition.  When you move it on the same partition, it just has to 
re-index it.  The file itself is not moved.  NTFS was designed to work that way 
and even in basic certifications, NTFS behavior is taught.  Once you understand 
the basic rules of NTFS, it's really not that difficult to work with.
The right-click version of saving doesn't use the temp dir and does not copy the
file.

Maybe it's my Unix background speaking when I think that partition boundaries
should be largely invisible to users... ;)
According to MSDN, SHFileOperation is available in shell32.dll version 4.0 and
later (NT4 and win95). Perhaps nsLocalFileWin::MoveTo should use this function
instead of MoveFile?

I definitely think this is a bug, not a feature ;)
Status: UNCONFIRMED → NEW
Ever confirmed: true
this will be automatically fixed once we stop predownloading to a temp file,
which I hope to fix during the next month.
QA Contact: mpconnor
This bug is not a Firebird specific bug, nor is it limited to Windows 2000.  I
am experiencing this bug on Windows XP Professional with Mozilla 1.5 & 1.6.
yes, well, this affects all windows-variants that support ACLs

and this happens in exthandler... so -> browser:file handling
Assignee: bugs → file-handling
Component: Downloading → File Handling
Depends on: 69938
Product: Firebird → Browser
QA Contact: mconnor → ian
Version: unspecified → 1.0 Branch
Version: 1.0 Branch → Trunk
This is realy annoying since it also applies to the network settings. I always
have to cut and paste the files around before I have access to them over LAN.
*** Bug 256548 has been marked as a duplicate of this bug. ***
I have to do the same thing, or force the permissions to the files by forcing
the inheritance rules using the security dialog on the parent folder.  This is
really annoying and I hope someone can figure out how to fix it, regardless of
whether it's a "bug in NTFS" or not.
*** Bug 258468 has been marked as a duplicate of this bug. ***
Bug 124307 is fixed, but not this windows specific issue. IMO all duplicated
windows bugs (Bug 123959, Bug 173940, Bug 196199, and Bug 219311) are in fact
duplicates of this one.
*** Bug 267614 has been marked as a duplicate of this bug. ***
This is a bug in Firefox, not in NTFS.  NSPR is explicitly building a custom ACL
for each file created.  See _PR_MD_OPEN_FILE() in
nsprpub\pr\src\md\windows\ntio.c and _PR_NT_MakeSecurityDescriptorACL() in
nsprpub\pr\src\md\windows\ntsec.c.  

I haven't tried to build Firefox yet, but it looks like just commenting out this
block of code in ntio.c would get rid of the issue.

    if (osflags & PR_CREATE_FILE) {
        if (_PR_NT_MakeSecurityDescriptorACL(mode, fileAccessTable,
                &pSD, &pACL) == PR_SUCCESS) {
            sa.nLength = sizeof(sa);
            sa.lpSecurityDescriptor = pSD;
            sa.bInheritHandle = FALSE;
            lpSA = &sa;
        }
    }

Of course, it would also break any file-level security Firefox tries to apply to
the profile directory as well, so this approach probably wouldn't be optimal.  
Once I get around to building Firefox, I'll give it a shot, though.  The default
behavior is _extremely_ annoying when combined with XP Home "Simple File
Sharing", because files downloaded by Firefox will _not_ be readable remotely
(since they don't inherit permissions from Guest), and under Home Edition it's
well nigh impossible to get rid of Firefox's custom ACL and inherit the default,
since M$ took away the files' Security tab.
(In reply to comment #17)
> This is a bug in Firefox, not in NTFS.  NSPR is explicitly building a custom ACL
> for each file created.  See _PR_MD_OPEN_FILE() in
> nsprpub\pr\src\md\windows\ntio.c and _PR_NT_MakeSecurityDescriptorACL() in
> nsprpub\pr\src\md\windows\ntsec.c.  


these files are not used by mozilla, which builds the WIN95 configuration of NSPR.

the real problem is that mozilla downloads to $TEMP and moves the file to the
real folder, which under windows does not seem to change the ACL. the fix would
probably be to make nsOSHelperAppService override FixFilePermissions and make it
inherit the ACL from the parent folder.
(In reply to comment #18)
>
> these files are not used by mozilla, which builds the WIN95 configuration of NSPR.
> 
> the real problem is that mozilla downloads to $TEMP and moves the file to the
> real folder, which under windows does not seem to change the ACL. the fix would
> probably be to make nsOSHelperAppService override FixFilePermissions and make it
> inherit the ACL from the parent folder.

By design in NTFS, a file keeps its permissions from the previous folder when it
is moved to a new location using the MOVE command, because NTFS just updates the
file system with the new location of the file, not changing the permissions.  In
order for a file to inherit the permissions from the folder you move it to, you
have to use the COPY command and then delete the file in the original directory,
since this creates a new entry in NTFS.  This is the problem with a file system
that stores its rights in the files instead of a database.

Mozilla 1.7 seems to have fixed this problem by creating the temp file in the
directory that you are downloading the file to, instead of using the $TEMP
directory.  Doesn't the newer versions of Firefox use this download method too?
 I don't use Firefox right now, so I don't know.
#19, This problem still exists in Mozilla 1.7.2 and Firefox 1.0PR (can't
download 1.0, maybe after it cools off I'll be able to).  I've noticed that it
appears to download straight to the dest directory (with a .part extension) and
then rename when it's done, rather than going to temp and move to the
destination, but it's irrelevant; the download still doesn't inherit parent
permissions.

I haven't been able to get Firefox to build (makefile errors), but once I do
I'll try commenting out the ACL generation code and see if it has any effect.
I successfully downloaded Firefox 1.0, and it still exhibits the issue.  Here
are steps to reproduce:

/* Make a test folder */

C:\>mkdir testfolder

/* Grant user angst full access to the folder */

C:\>cacls testfolder /E /G angst:F
processed dir: C:\testfolder

/* Create a file in the folder */

C:\>cd testfolder

C:\testfolder>copy con testfile.txt
This is a test.
^Z
        1 file(s) copied.

/* Check permissions */

C:\testfolder>cacls *
C:\testfolder\testfile.txt BUILTIN\Administrators:F
                           NT AUTHORITY\SYSTEM:F
                           AD\jeremy:F
                           BUILTIN\Users:R
                           ACCESSDA-0ZO3M5\angst:F

/* File inherits folder's permissions--user angst has full access. */

/* Begin download of a file in Firefox 1.0 here... */

C:\testfolder>dir
 Volume in drive C is SYSTEM
 Volume Serial Number is 4CF9-097E

 Directory of C:\testfolder

11/09/2004  10:14    <DIR>          .
11/09/2004  10:14    <DIR>          ..
11/09/2004  10:02                17 testfile.txt
11/09/2004  10:14         3,171,724 Thunderbird Setup 0.9.exe.part
               2 File(s)      3,171,741 bytes
               2 Dir(s)  40,748,904,448 bytes free

/* The .part file is there.  Check its permissions... */

C:\testfolder>cacls *
C:\testfolder\testfile.txt BUILTIN\Administrators:F
                           NT AUTHORITY\SYSTEM:F
                           AD\jeremy:F
                           BUILTIN\Users:R
                           ACCESSDA-0ZO3M5\angst:F

C:\testfolder\Thunderbird Setup 0.9.exe.part AD\jeremy:F
                                             NT AUTHORITY\SYSTEM:F
                                             BUILTIN\Administrators:F

/* The .part file did not inherit the folder's permissions. */

/* Download complete.  Check permissions again... */

C:\testfolder>cacls *
C:\testfolder\testfile.txt BUILTIN\Administrators:F
                           NT AUTHORITY\SYSTEM:F
                           AD\jeremy:F
                           BUILTIN\Users:R
                           ACCESSDA-0ZO3M5\angst:F

C:\testfolder\Thunderbird Setup 0.9.exe AD\jeremy:F
                                        NT AUTHORITY\SYSTEM:F
                                        BUILTIN\Administrators:F

/* Completed file did not inherit folder's permissions. */
(In reply to comment #18)

Okay, I've finally convinced Firefox to build so I could try out my changes. 
The change I referred to in comment #17 is indeed irrelevant and
ineffectual--sorry for jumping to conclusions.

I tried replacing MoveFile() by SHFileOperation() in
nsLocalFileWin.cpp:CopySingleFile(), but that caused strange results.  It could
very well be a mistake on my part, but I don't know if shell functions belong
there anyway.  The FOF_NOCOPYSECURITYATTRIBS requires MSIE 4.0, incidentally
(don't know whether that's important or not).

Doing CopyFile() followed by DeleteFile() instead of MoveFile() does indeed
solve the problem, but as comment #2 points out, that could be slow.

I think I'll just stick $TEMP on a FAT partition until the direct downloading is
implemented.


>Mozilla 1.7 seems to have fixed this problem by creating the temp file in the
>directory that you are downloading the file to

I'm glad that this is the impression people have, although it is not correct.
while the helper app dialog is shown (i.e. before you pick a file location), the
data is stored in $TEMP, and it's moved only after that.
stanmuffin@gmail.com: fwiw we want to support nt3.51 w/o ie. so requiring ie
isn't acceptable, but thanks for looking into this.
Slightly OT:  For those using simple file sharing on XP Home, I've written a
short program that will fix the permissions on all files in a share, including
those downloaded into the share by Mozilla or Firefox.  No need to cut and paste
files around or boot into safe mode to get to the file security tab.

Source and binary here:

http://www.xmission.com/~jstanley/racls.html
Jeremy Stanley commented on moving the $TEMP directory to a FAT partition as a
workaround to this problem.  For people who don't have a FAT partition, or who
don't want to move their directory, another workaround is to add "everyone" to
the permissions list of the temp directory and allowing Read, Execute, and List.
 The default location is C:\Documents and Settings\<username>\Local Settings\Temp
rot_j@yahoo.com: never suggest that. never never never never never.
(In reply to comment #27)
> rot_j@yahoo.com: never suggest that. never never never never never.

Could you explain why?
because that's suggesting a configuration where one user (or virus running as
that user) could drop a malicious file in place which is then run as another user.
*** Bug 273273 has been marked as a duplicate of this bug. ***
*** Bug 276931 has been marked as a duplicate of this bug. ***
Summary: Downloaded Files Don't Inherit NTFS Properties from Parent Folder → Downloaded Files Don't Inherit NTFS Properties (like permissions, compression, encryption) from Parent Folder/Directory
*** Bug 278094 has been marked as a duplicate of this bug. ***
I have run into this problem.  I checked out the most recent build as required.
 The XP downloads always sets the ACL to a specific set of permissions rather
than inheriting the permissions of the directory. In my test cases there was
always 3 explicit entries in the ACL; one for Administrators, one for the local
user name used for the download, and one for SYSTEM.  Of course this leaves out
any other users that may have permission at the directory level.  So,
consequently the network logins which are set up to get to the directory still
can't get to the downloaded file. 

The only exception I noticed was when the download goes to the desktop; but I
expect XP monkeys with this particular directory.

I am not looking at the source so I don't have any idea why; maybe an incomplete
use of the XP API.  I did check IE and it doesn't do this, so there must be some
rational resolution.  

It causes me a problem only because I keep some directories synced up and this
access problem hoses my sync process.  Unfortunately it can be rather confusing
to someone using XP Home since they can't actually get a look at the ACL.  At
least I don't think they can. 


> The XP downloads always sets the ACL to a specific set of permissions rather
> than inheriting the permissions of the directory. 

it inherits permissions from %TEMP%
*** Bug 256472 has been marked as a duplicate of this bug. ***
*** Bug 289308 has been marked as a duplicate of this bug. ***
*** Bug 291479 has been marked as a duplicate of this bug. ***
(In reply to comment #37)
> *** Bug 291479 has been marked as a duplicate of this bug. ***

Oops... sorry developers, before submitting the bug I searched using "save file"
keywords, and this and duplicate bugs didn't come out, maybe because the search
was too generic. Only after I began filling the form, I started doing more use
of the "download" and "ntfs" words, which would have returned good results. My
fault. :-/

Anyway I confirm this bug in Firefox 1.0.3 on WinXP Pro, and add my request to
fix it. In short, here's one reason (quoting from the other report): let's
suppose I usually save files in a shared directory, because I want them to be
available to other users: the current behaviour prevents this.
*** Bug 296162 has been marked as a duplicate of this bug. ***
*** Bug 300400 has been marked as a duplicate of this bug. ***
(In reply to comment #40)
> *** Bug 300400 has been marked as a duplicate of this bug. ***

Yikes, sorry about that. I didn't realize it was this deep, and searching
"download lock" apparently didn't help at all.
*** Bug 303348 has been marked as a duplicate of this bug. ***
*** Bug 311182 has been marked as a duplicate of this bug. ***
*** Bug 316849 has been marked as a duplicate of this bug. ***
I think this bug explains why .PDF and .ps.gz downloaded on to my M: drive
(real Linux disk mapped via network to WindowsNT 5.1 (service pack 2)) by
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051119 Firefox/1.6a1 get "x". Ie Unix thinks they are runable.
Re: comment 2

A copy-then-delete may be slow, but it's MUCH faster than forcing the user to do the same thing by hand after every download (which is what I have to do now). Would it be possible to use the copy-then-delete workaround until someone has the time to investigate a more efficient solution?

If the dev team is currently swamped, I'd be happy to try writing the patch for the workaround if I knew there was a good chance it would find its into a build. Could a committer confirm that the workaround would be acceptable before I sink a lot of time into writing the patch?
Actually, from comment 22 I see that Jeremy has already made the patch I was thinking of. Can you please attach your copy-then-move patch?

I have this problem frequently, but I wouldn't want to burden other people who probably don't have this issue as often as I do. I'm just brainstorming here, but rather than copy every downloaded file (of any size; could be sloooooow) because it didn't get created correctly, how about this - 

1) create a zero byte file in the actual intended download directory; the security descriptor should be created appropriately.
2) Immediately move the zero byte file to the temp directory
3) Append the downloaded data into the file while it's in the temp directory
4) Move the complete downloaded file back to the intended dowload directory

It should still have a security descriptor that was created at the whim of the OS based on it's creation in the download directory, and we don't have to write the file to disk more than once.

I would hope steps 1&2 are quick enough that it doesn't cause confusing appearing/disappearing files in explorer, if the download folder is already open.  I really wouldn't know though.

Other than that, I think I'd prefer that the security descriptor of the desired download directory and the file created in the temp directory be actually compared and updated if necessary.  I guess that duplicates some OS file ops code though?


Problem still exists in Firefox 1.5.0.1 with XP Home.
(In reply to comment #49)
> Problem still exists in Firefox 1.5.0.1 with XP Home.

and in 1.5.04.

It sure would be nice if this could be fixed.

If the developers cannot figure out how to correct this, perhaps someone could add a "post-download script" option to the download manager to allow me to execute a script that sets permissions as I want.  This would allow me to execute batch cmd file that executes the xcacls.exe command on the file just downloaded.



Some have said that microsoft should not have a different behavior for move and copy commands.  I disagree.  It makes sense that copied files inherit the permissions of the parent folder but moved files retain their permissions.  If I choose to set permissions on a file to allow edit access only to certain users and then move that file to a shared folder, I wouldn't want all users who have access to the share to also have access to my file.  If, however, I copied that same file to the share, I would expect that all users who have access to the share would also have access to the file.
Problem still exists in Firefox 2.0.0.1.

Regardless of ones view of whether MOVE and COPY should behave the same, the fact is that they do not on MS Windows platforms, so it would really be nice if the owner of this part of the code would just accept the situation for what it is and fix it.

At a minimum, give us an optionon the download preferences to COPY the temp file so those of us who are willing to accept the slower performance will at least have that option.
BTW, this is the only issue preventing me from using Firefox full time.   Every time I get comfortable w/ Firefix, I manage to forget about this issue and download a file into a shared folder that I expect my coworkers to access.  And then one of them complains they cannot access the file and I have to go modify the file to add the needed permissions, and then I remember why I was using IE.

If doing a copy is all that is needed, why has it taken 3 years to address this?
Comment 48 is the only thing I've seen in the last three plus years that actually provided something constructive over my initial report. Could someone please just implement that change and let this be over with? I too am sick of the bickering and this bug is the primary motive for me not using Firefox. I work in a place that does a lot of NetBT traffic and incorrect permissions is a huge annoyance. If there were some sort of a command that did a move and inherit, then it would probably execute by copying and deleting the original. The fact is that a move on the same partition just updates the MFT. Let's just move past that fact and implement the suggestion from 48 to get this out of the way. Please?
(In reply to comment #56)
The problem with what was suggested in Comment 48 is that we don't necessarily know the final download location when the download begins--that's the reason we started downloading to temp in the first place.

Anyhow, here's a drop-in replacement for MoveFile() that will make sure the destination file inherits its parent directory's permissions.  No need to copy and delete.

#include <aclapi.h>

BOOL MoveFileAndInheritDestPermissions(LPCTSTR lpExistingFileName, LPCTSTR lpNewFileName)
{
	// First, move the file	
	if ( !MoveFile(lpExistingFileName, lpNewFileName) )
	{
		return FALSE;
	}

	// Next, reset the ACL of the destination file 
	ACL empty_acl;
	if ( InitializeAcl(&empty_acl, (DWORD) sizeof(ACL), ACL_REVISION) )
	{
		// Assign the empty ACL.  The UNPROTECTED_DACL_SECURITY_INFORMATION tells it to
		// inherit the parent folder's permissions.
		SetNamedSecurityInfo((LPTSTR)lpNewFileName, SE_FILE_OBJECT, 
			DACL_SECURITY_INFORMATION | UNPROTECTED_DACL_SECURITY_INFORMATION,
			NULL, NULL, &empty_acl, NULL);
	}

	return TRUE;
}

(In reply to comment #57)
Okay, I took a closer look at the docs for SetNamedSecurityInfo and I see that this will only work on Windows 2000 and above.  On NT4, it'd have the unpleasant effect of denying everyone access to the destination file.  

And of course, 9x/Me don't support permissions in the first place, and probably don't implement these functions, so we'd have to explicitly load them from advapi32.dll.
On mozilla trunk we only support win2k and above.
This is really annoying.  If I download a file to my desktop using Firefox, I can't access it, copy, move, or even play it from another computer unless I move it to another folder using the upstairs computer and then moving it back, I guess restting permissions.  I have to go upstairs to move it since I don't have permission to move it from the downstairs comp!

All the file share settings are default in XP Pro/Home (ake, simple file sharing on) and it happens with a Linux (ubuntu) samba client as well.
This is very annoying when you download something to install on multiple computers, because the file is in a shared folder, but is not shared.
the only way to fix it seems to be move the download files to another folder then move them back.

Please fix this, highest priority.
This bug continues with Firefox 3 (still on XP home).  
Okay; I'm going to bite the bullet and attempt to make a patch for this, despite my unfamiliarity with both Mozilla's source and NTFS.

Thanks to earlier comments on this bug (particularly Comment 57), I think I have a decent idea of what needs to be done and where to do it.  Jeremy, if you're still around: are you sure that it's necessary to create an empty ACL and set it?  The documentation I've read at MSDN isn't terribly clear on this point, but it sounds like just setting UNPROTECTED_DACL_SECURITY_INFORMATION tells it to inherit ACEs from its parent object.  Or am I misunderstanding something?
Based on the solution given by Jeremy Stanley in Comment 57.

My own quick tests on Windows XP SP3 show downloads successfully inheriting the security permissions of the parent folder.  This should also affect any other time a file is moved instead of copied.  I don't believe the other NTFS properties mentioned in the bug summary (compression, encryption) will be fixed by this patch.
Attachment #327278 - Flags: review?
Remember.  This only applies to XP Home.  If you are testing on XP Pro... it will work properly.
No, this bug occurs on XP Pro as well.  (Also, Windows 2000 and probably Vista.)  The difference is that in XP Home it's harder for the user to fix manually; see Comment 25.
Attachment #327278 - Flags: review? → review?(benjamin)
When will this be fixed?!

Very annoying...
Comment on attachment 327278 [details] [diff] [review]
Inherit security permissions from parent for moved files

Most importantly, I absolutely need unit-tests for this code. I'm not sure how you'd write them... perhaps a custom "make check" rule. Let me know if you have no clue what I mean, and I can try to walk you through the process.

Secondly, this patch contains whitespace inconsistencies... please replace tabs with spaces and ensure that indenting is consistent.
Attachment #327278 - Flags: review?(benjamin) → review-
Thank you for the review.

It'll be a couple of days before I have I chance to really work at it, but if you could point me in the direction of any documentation I should look at to help get up to speed on how to make a unit test for this, that would be appreciated, as this is my first time working with Mozilla.  (I've already found http://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests which seems to have the basics, and am reading through that now.)
I don't think you're going to be able to use an xpcshell testcase: xpcshell doesn't have any way to check whether NTFS permissions are correct.

Instead, I'd focus on writing a binary executable as testcase, in pseudocode:

int main()
{
  CreateDirectory("foo");
  SetNTFSPermissions("foo");

  CreateDirectory("bar"):
  SetOtherNTFSPermissions("bar");

  CreateFile("foo/file");

  nsCOMPtr<nsIFile> fooFile = NS_NewLocalFile("/path/to/foo/file");
  nsCOMPtr<nsIFile> barDir = NS_NewLocalFile("/path/to/bar");

  fooFile->MoveTo(barDir);

  CheckForCorrectNTFSPermissionsOn("bar/file");
}

You can put this testcase in xpcom/tests/windows. In xpcom/tests/windows/Makefile.in add it to CPPSRCS (which ends up in SIMPLE_PROGRAMS), and then create a check rule to run the program.

check::
    ./CheckNTFSFileMoves.exe


Ok, that's about what I figured after looking through the documentation.  Thank you for the guidance.
Um, this might be complicated by the fact that our tinderboxes build on FAT32 partitions for speed. I dunno about the unittest boxes, but odds are they're the same.

This should fix all the white-space issues I noticed; I'm currently starting work on a test-case as described in Comment 70.
Attachment #327278 - Attachment is obsolete: true
Probably stupid question time:

I've been working on the test case, and am running into unresolved externals related to the Windows NTFS permissions functions (__imp__SetSecurityDescriptorDacl@16, __imp__InitializeSecurityDescriptor@8, __imp__SetEntriesInAclA@16, and __imp__AllocateAndInitializeSid@44).  My best guess for the cause is that advapi32.lib not being linked to the executable for my test.  I can see that this library is linked to other things during the build process (and indeed the use of functions from it in the patch itself hasn't caused any problems), but despite looking through the makefiles and documentation I haven't figured out how to tell the build system to link it to my test.

How could I do this, or am I barking up the wrong tree entirely?  Any guidance would be appreciated.
OS_LIBS += -ladvapi32 in the Makefile.in which links the test executable.
Attachment #329227 - Flags: review?(benjamin)
Attached patch NTFS Permissions Unit Test (obsolete) — — Splinter Review 
Unit test for clearing NTFS permissions on moving a file.

This patch also runs TestCOM.exe on a make check, which was previously being built but not - to the best of my knowledge - run.  There is a third test in the directory (TestHelloXPLoop) which was not being built or run; this patch does not change that.
Attachment #329568 - Flags: review?
Attachment #329568 - Flags: review? → review?(benjamin)
Comment on attachment 329568 [details] [diff] [review]
NTFS Permissions Unit Test

What happened to Makefile.in? did you remove or add windows-style line endings or something?

Also, you're using bareword "true" in a couple places: please use PR_TRUE.
Attachment #329568 - Flags: review?(benjamin) → review-
Attachment #329227 - Flags: review?(benjamin) → review+
Fixed usage of "true" and (hopefully) fixed the line endings.
Attachment #329568 - Attachment is obsolete: true
Attachment #330857 - Flags: review?(benjamin)
Attachment #330857 - Flags: review?(benjamin) → review+
We're in a freeze right now, so I'll try and remember to get this landed once 3.1b1 is unfrozen. Ping me in a week if I forget.
Keywords: checkin-needed
(In reply to comment #79)
> We're in a freeze right now, so I'll try and remember to get this landed once
> 3.1b1 is unfrozen. Ping me in a week if I forget.
> 

Ping. :)

And thank you for all your assistance.
Sorry guys, Im new to this, so basically I have to wait, until it is incorporated into the next beta? There is no fix, right?

Thanks...it bothers me :)
Pushed to mozilla-central, revision 891c485b267d (code) 11043f005a33 (test).

I don't think we should take this on 1.9.0.x until it has baked for a trunk alpha.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 450617
I disabled the unit test because the unit-test boxes are running FAT32 instead of NTFS and so it won't actually help, plus I'm getting the following error:

* first time: "FAIL NTFS Permissions: Creating Temporary File"
* second time: while trying to clean, "Access is denied: D:\builds\slave\trunk_win3k3-01\build\objdir\xpcom\tests\windows\NTFSPERMTEMP1"
(In reply to comment #83)
> * first time: "FAIL NTFS Permissions: Creating Temporary File"
> * second time: while trying to clean, "Access is denied:
> D:\builds\slave\trunk_win3k3-01\build\objdir\xpcom\tests\windows\NTFSPERMTEMP1"
> 

I'm traveling, so I can't look into it right now, but I think that these are caused by running it on FAT32, which I didn't test.  (Possibly due to how Windows creating the directory as read-only by trying to convert the NTFS permissions settings somehow, but that's just an uninformed guess since I'm not overly familiar with this stuff.)  I know I didn't see this myself on NTFS; when/if I get a chance I'll try and run it on FAT32 to see if I get identical behavior.
Additional possibility: do the tests run under an Administrator account or not?  The test case is designed to set read permissions for "Everyone" on the first folder and only Administrators have full access.  Not sure how that would translate to FAT32, though, since from my understanding that file system doesn't support permissions at all.
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.1a2
(In reply to comment #82)
> I don't think we should take this on 1.9.0.x until it has baked for a trunk
> alpha.

Benjamin, I tried to see the difference between current 1.9.1 trunk builds and a latest Gran Paradiso build on Windows XP (VmWare). I can't see a difference when playing around with compression and permissions. Downloaded files with Gran Paradiso inherits the correct settings. Could this be an issue with VmWare? 


Mardak/Aaron: since it doesn't appear practical to unit-test this, can we at least create a litmus test for it? Can one of you write a set of steps to test this using the download manager?
Flags: in-litmus?
(In reply to comment #86)
> Benjamin, I tried to see the difference between current 1.9.1 trunk builds and
> a latest Gran Paradiso build on Windows XP (VmWare). I can't see a difference
> when playing around with compression and permissions. Downloaded files with
> Gran Paradiso inherits the correct settings. Could this be an issue with
> VmWare? 

Hmm.  Does the directory you're downloading to have different permissions from your temporary directory?  Is it on the same volume as the temporary directory?  (If it isn't, Windows will automatically convert the move to a copy-then-delete, avoiding this issue.)

(In reply to comment #87)
> Mardak/Aaron: since it doesn't appear practical to unit-test this, can we at
> least create a litmus test for it? Can one of you write a set of steps to test
> this using the download manager?
> 

Apologies for the delay.  The following steps should test it:

1) Create a shared directory on the same volume as the temporary directory.  (If simple file sharing is off, any alteration of the permissions will do.)

2) Download a file to that directory.

3) Attempt to access the file over the network.  (If simple file sharing is off, the "Security" tag of the properties of the downloaded file can be checked to see if permissions were inherited properly.)

Lets reopen shortly to set the correct assignee.
Status: RESOLVED → REOPENED
QA Contact: ian → file-handling
Resolution: FIXED → ---
Assignee: file-handling → anowack
Status: REOPENED → NEW
Status: NEW → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Thanks Aaron, these steps are perfect. My problem was that my tmp folder was on another drive as the target download folder. I can verify that the correct permissions are set now. I compared the lastest nightly build with Firefox 2.0.0.16.

Public mini-vista\henrik:(OI)(CI)(F)
       VORDEFINIERT\Administratoren:(I)(F)
       VORDEFINIERT\Administratoren:(I)(OI)(CI)(IO)(F)
       NT-AUTORITÄT\SYSTEM:(I)(F)
       NT-AUTORITÄT\SYSTEM:(I)(OI)(CI)(IO)(F)
       VORDEFINIERT\Benutzer:(I)(OI)(CI)(RX)
       NT-AUTORITÄT\Authentifizierte Benutzer:(I)(M)
       NT-AUTORITÄT\Authentifizierte Benutzer:(I)(OI)(CI)(IO)(M)

Firefox Setup 3.0.1 (fx2.0).exe mini-vista\henrik:(F)
                            NT-AUTORITÄT\SYSTEM:(F)
                            VORDEFINIERT\Administratoren:(F)

Firefox Setup 3.0.1.exe mini-vista\henrik:(I)(F)
                        VORDEFINIERT\Administratoren:(I)(F)
                        NT-AUTORITÄT\SYSTEM:(I)(F)
                        VORDEFINIERT\Benutzer:(I)(RX)
                        NT-AUTORITÄT\Authentifizierte Benutzer:(I)(M)

Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/20080823032129 Minefield/3.1a2pre ID:20080823032129

Before we can add it as a test on Litmus we have to wait until the 3.1 testgroup has been cloned.
Status: RESOLVED → VERIFIED
(In reply to comment #91)
> *** Bug 453607 has been marked as a duplicate of this bug. ***

Actually, to the best of my knowledge, my patch does not fix compression or encryption; just permissions.  I took a brief look at fixing those, but on first glance it looked fairly complex, and I didn't have time to get into it.

Maybe open a new bug for those or reopen that dupe?  Or reopen this one, I suppose.
I can confirm that the encryption is not working in the 3.1b1 nightly build.
Well now, looking through this (again) it seems like the permission side is fixed.  However, it will still not inherit other permissions-type attributes of the enclosing folder (encryption and compression are the main two).  In researching this, it appears that the following steps are taken to process a download request:

1) Placeholder file ($P) is created in %TEMP%
2) $P is passed to the download code (which is treated internally as a plugin)
3) Final file ($F) is opened as a 0-byte file at the download folder
4) $P is moved to the download folder and renamed with a ".part" extension
5) Permissions are set on $P (after bug fix)
6) When download completes, $F is deleted
7) $P is renamed to remove the ".part"

Note that in this flow, $P has the permissions/extended attributes of the %TEMP% directory, and $F has the permissions/extended attributes of the download folder.  You can verify this by performing a 1GB download and checking permission on the files while the download is occuring.

Due to the decision to have the download process work as a plugin, we cannot tell the main program to download the data to $F directly.  It will only write data to $P, and pass that information off to the download code.  Due to this, the only answer I can see is the "copy at completion" method.  In fact, I believe that's what IE is currently doing (download a large file, and you will see a progress bar show up once the download is complete - it's doing a copy at completion to bypass this issue). 

Note that Vista is implementing a change to make this less of an issue.  Not sure if it's a "move" change or just a shell thing.  Also unclear is if this will affect encryption and compression as well, or just security.  See http://support.microsoft.com/kb/320246 for info on both the initial problem, as well as the note on Vista changes.

So, (coming to an answer!  I promise!) since we already have the correct inherited permissions in $F, why not simply do a bulk data copy from $P to $F, then delete $P?  This would take the place of steps 5-7 in the above flow.
(In reply to comment #94)
> Due to the decision to have the download process work as a plugin, we cannot
> tell the main program to download the data to $F directly.
Also, $P starts downloading in the background before we necessarily know the location of $F, if a Save As box is triggered.

> since we already have the correct inherited permissions in $F, why not simply
> do a bulk data copy from $P to $F
This is IE behaviour and always struck me as rather stupid. What happens if the user is downloading a 4gb DVD image or something, and doesn't have 8gb free? The copy fails and the download is left in the temporary folder, unbeknownst to the user? Why should they have twice the required room available to download a file? As an aside, IE also has no excuse to download large files to a temp dir, given it always waits for $F to be known before doing anything.
I think the solution that's being implemented here is the best, especially if it can be extended to apply the correct encryption and compression settings. As opposed to copying the file, which seems a quick and dirty way out, which wastes both time and disk space for the user.
(In reply to comment #95)
(In reply to comment #94)
> Well now, looking through this (again) it seems like the permission side is
> fixed.  However, it will still not inherit other permissions-type attributes of
> the enclosing folder (encryption and compression are the main two).  In
> researching this, it appears that the following steps are taken to process a
> download request:
> 
> 1) Placeholder file ($P) is created in %TEMP%
> 2) $P is passed to the download code (which is treated internally as a plugin)
> 3) Final file ($F) is opened as a 0-byte file at the download folder
> 4) $P is moved to the download folder and renamed with a ".part" extension
> 5) Permissions are set on $P (after bug fix)
> 6) When download completes, $F is deleted
> 7) $P is renamed to remove the ".part"
> 
> Note that in this flow, $P has the permissions/extended attributes of the
> %TEMP% directory, and $F has the permissions/extended attributes of the
> download folder.  You can verify this by performing a 1GB download and checking
> permission on the files while the download is occuring.
> 
> Due to the decision to have the download process work as a plugin, we cannot
> tell the main program to download the data to $F directly.  It will only write
> data to $P, and pass that information off to the download code.  Due to this,
> the only answer I can see is the "copy at completion" method.  In fact, I
> believe that's what IE is currently doing (download a large file, and you will
> see a progress bar show up once the download is complete - it's doing a copy at
> completion to bypass this issue). 
> 
> Note that Vista is implementing a change to make this less of an issue.  Not
> sure if it's a "move" change or just a shell thing.  Also unclear is if this
> will affect encryption and compression as well, or just security.  See
> http://support.microsoft.com/kb/320246 for info on both the initial problem, as
> well as the note on Vista changes.

The Microsoft article doesn't seem to apply in the encryption issue. My tests show that encryption IS applied on Explorer-moved files/folders in both XP and Vista, although ACLs only in the case of Vista. However, the Firefox behaviour of NOT inheriting encryption is the same on Vista SP1 64-bit and XP SP2 32-bit.

> So, (coming to an answer!  I promise!) since we already have the correct
> inherited permissions in $F, why not simply do a bulk data copy from $P to $F,
> then delete $P?  This would take the place of steps 5-7 in the above flow.

(In reply to comment #94)
> Well now, looking through this (again) it seems like the permission side is
> fixed.  However, it will still not inherit other permissions-type attributes of
> the enclosing folder (encryption and compression are the main two).  In
> researching this, it appears that the following steps are taken to process a
> download request:
> 
> 1) Placeholder file ($P) is created in %TEMP%
> 2) $P is passed to the download code (which is treated internally as a plugin)
> 3) Final file ($F) is opened as a 0-byte file at the download folder
> 4) $P is moved to the download folder and renamed with a ".part" extension
> 5) Permissions are set on $P (after bug fix)
> 6) When download completes, $F is deleted
> 7) $P is renamed to remove the ".part"
> 
> Note that in this flow, $P has the permissions/extended attributes of the
> %TEMP% directory, and $F has the permissions/extended attributes of the
> download folder.  You can verify this by performing a 1GB download and checking
> permission on the files while the download is occuring.
> 
> Due to the decision to have the download process work as a plugin, we cannot
> tell the main program to download the data to $F directly.  It will only write
> data to $P, and pass that information off to the download code.  Due to this,
> the only answer I can see is the "copy at completion" method.  In fact, I
> believe that's what IE is currently doing (download a large file, and you will
> see a progress bar show up once the download is complete - it's doing a copy at
> completion to bypass this issue). 
> 
> Note that Vista is implementing a change to make this less of an issue.  Not
> sure if it's a "move" change or just a shell thing.  Also unclear is if this
> will affect encryption and compression as well, or just security.  See
> http://support.microsoft.com/kb/320246 for info on both the initial problem, as
> well as the note on Vista changes.
> 
> So, (coming to an answer!  I promise!) since we already have the correct
> inherited permissions in $F, why not simply do a bulk data copy from $P to $F,
> then delete $P?  This would take the place of steps 5-7 in the above flow.
(In reply to comment #95)
> I think the solution that's being implemented here is the best, especially if
> it can be extended to apply the correct encryption and compression settings. As
> opposed to copying the file, which seems a quick and dirty way out, which
> wastes both time and disk space for the user.

Agreed on the disk space issue.  That's the one concern that I have at the moment.  However, seeing as this issue is handled the "quick and dirty" way by MS themselves, it's unclear to me if we'll find a better solution.

The other thing here is that (if I remember my NTFS internals) is that the ACL is just one specific class of file attribute inside NTFS.  There may be others (encryption, compression, user defined attributes, etc).  We could add code to handle the encryption and/or compression, but if in the future there is another attribute added, we would have to add the code for that here as well.  The copy/delete would eliminate all these attribute limitations as well as handling the security issue that brought this to light.
If disk space is the only issue for the quick and dirty solution, would it be possible to avoid that using a copy then append as:

1. start download into a temp file.
2. when destination file is known, 
2a. close current temp file
2b. copy temp to destination (lets NTFS establish desired permissions)

3. open destination for append
4. download remainder of file to destination

Sorry if this is what is already on the table.

Michael
(In reply to comment #98)
> If disk space is the only issue for the quick and dirty solution, would it be
> possible to avoid that using a copy then append as:

This would be possible if the architecture was not designed as it is.  The issue with this is that the download code would need to be integrated more tightly with the core than it is now.  Right now, the core downloads the entire file to a temp folder, and passes the information to a plugin to determine any further processing (ie, download to folder, open with application, etc).  Downloads are processed as just another plugin.  We can't interrupt the download process in the core to do the close and reopen.

One possible alternative (which would break the current architecture) would be to move the download code into the core.  We could then do the download directly to the final location, bypassing the temp folder altogether.  I did not raise this possibility as it would change a lot of code in the core, and I figured that the overall design and code responsibility was cleaner the way it's built now.
If you cancel the download and restart it, the file gets encrypted. What's the deal with that?
Blocks: 403014
I'm using Windows XP Pro SP2/SP3 and FF 3 series and I have this issue when I download file to windows desktop directory. File doesn't inherit security/accesibility permissions from shared desktop folder. When I download file to other directories there is no problem.
Please don't neglect the encryption aspect here. If a program such as Firefox downloads a file to an encrypted (EFS) directory without properly propagating the EFS attribute from the directory to the file, that could cause an information disclosure vulnerability (unencrypted data on disk when it should have been encrypted data). I consider this a serious problem and am surprised it has gone unpatched for so long. Perhaps EFS is not as commonly used as I thought?
Ok, so there is not everything fixed when reading comment 92. That's why split those bugs out into new ones.

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

Bug 483193 -  Downloaded files don't inherit NTFS compression properties from parent folder/directory
Summary: Downloaded Files Don't Inherit NTFS Properties (like permissions, compression, encryption) from Parent Folder/Directory → Downloaded files don't inherit NTFS permissions from the parent folder/directory
(In reply to comment #102)
> I'm using Windows XP Pro SP2/SP3 and FF 3 series and I have this issue when I
> download file to windows desktop directory. File doesn't inherit
> security/accesibility permissions from shared desktop folder. When I download
> file to other directories there is no problem.

Please check the latest Firefox 3.1 beta 3 version which was released yesterday. In this version it is fixed. I don't think that it will be fixed in any of the following Firefox 3.0.x versions.

https://developer.mozilla.org/devnews/index.php/2009/03/12/firefox-31-beta-3-now-available-for-download/
Renaming a file (i.e. not actually moving it) still resets the ACL :-(
TestNTFSPermissions.exe still passes with this patch.
Attachment #370000 - Flags: superreview?(benjamin)
Attachment #370000 - Flags: review?(benjamin)
The latest 3.6 nightly on Windows 7 has the same behaviour of not applying the encryption attribute of the download folder.
Attachment #370000 - Flags: superreview?(benjamin)
Attachment #370000 - Flags: superreview+
Attachment #370000 - Flags: review?(benjamin)
Attachment #370000 - Flags: review+
Comment on attachment 370000 [details] [diff] [review]
Fix non-inherited permissions

Pushed changeset da65cb02909d to mozilla-central.
(In reply to comment #109)
> The latest 3.6 nightly on Windows 7 has the same behaviour of not applying the
> encryption attribute of the download folder.

See bug 483192. This is not fixed at all.
(In reply to comment #107)
> Renaming a file (i.e. not actually moving it) still resets the ACL :-(

Neil, how can this be tested manually? Has the temp folder to be the download folder?

(In reply to comment #108)
> Created an attachment (id=370000) [details]
> Fix non-inherited permissions

Please file a new bug in the future instead of attaching a patch to a bug which has been fixed a long time ago. That only raise confusions. Thanks.
Oh, and now we need an approval for 1.9.1 too. :/ Neil, if your patch is ready please ask for that.
(In reply to comment #112)
> (In reply to comment #107)
> > Renaming a file (i.e. not actually moving it) still resets the ACL :-(
> Neil, how can this be tested manually? Has the temp folder to be the download folder?
The regression doesn't affect downloads.

> (In reply to comment #108)
> > Created an attachment (id=370000)
> > Fix non-inherited permissions
> Please file a new bug in the future instead of attaching a patch to a bug
> which has been fixed a long time ago. That only raise confusions. Thanks.
Yeah, I realised about 47 seconds after my mistake... Sorry.

(In reply to comment #113)
> Oh, and now we need an approval for 1.9.1 too. :/ Neil, if your patch is ready
Approval rules suggest baking first... I'll make the request next week.
Comment on attachment 370000 [details] [diff] [review]
Fix non-inherited permissions

Requesting 1.9.1 approval on this regression fix for loss of any explicitly defined permissions that may exist on the file being moved.
Attachment #370000 - Flags: approval1.9.1?
Comment on attachment 370000 [details] [diff] [review]
Fix non-inherited permissions

a191=beltzner
Attachment #370000 - Flags: approval1.9.1? → approval1.9.1+
Pushed changeset b8d9e996c340 to releases/mozilla-1.9.1
Keywords: fixed1.9.1
I tested the download functionality on Firefox 3.5 [version info from "Help--> About Mozilla Firefox": Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5 (.NET CLR 3.5.30729)], and the file permissions are still not being set correctly.

For comparison, when I download a file using IE 7, all NTFS permissions for the downloaded file show as being inherited from a parent folder. Same thing when I download a file in FF 3.5 when the user environment has TMP variable set to a folder on my FAT32 volume: permissions are inherited correctly.

But when I download a file using FF 3.5 and the user environment TMP variable is set to the default user profile temp folder on the NTFS volume, the file includes all the inherited permissions, but also NEW permissions that are "not inherited" but are copies of some (but not all) of the permissions of the inherited properties.
This is still a major bug in Firefox 3.5.1, even today.

I could go off on a long rant about how poor Firefox's download system is, but I'll just leave it at this: Firefox makes a ".part" file with the incorrect permissions, not inherited (in fact: the file specifically shows <not inherited> for each of its 3 ACL entries), and a "dummy" 0-byte file with the correct filename, while it's downloading. The DUMMY FILE is properly set to inherit permissions, and works fine.

However, when the download finishes, Firefox merely deletes the dummy file - the file that had the correct name and permissions - and renames the ".part" file to the downloaded file name. You know what that does? It keeps those ACLs, not because of a bug in Windows or a problem with PEBKAC, but because Firefox set inappropriate permissions (explicitly!) on that temporary "part" file (instead of just having Windows write a "new file" and letting Windows handle it), then renamed the file.

I don't know what the huge problem is. Why can't Firefox just, simply, write a new file, via the OS, and stream the download into it? Why must it be this complicated, and unfixed (Yes: UNFIXED, not "Verified Fixed" - who the hell set that status on this?), problem even exist?
Depends on: 670911
Experienced different issue with NTFS-3G on Linux 14.04.
Download saves as zero byte file if using NTFS-3G option "inherit" for the mount point. Default option produces normal behavior, but then causing some access issue if the directory is opened from Windows.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: