Do not follow Reparse Points during deletion and copy

[schinagl]

Problem

Winfile follows reparse points during deletion and thus deletes linked content, which is not wanted. Currently there is no way of simply unlinking a reparse point. Explorer is aware of this.
I would say this is a critical problem in Winfile, because it really deletes things, which you don't want to be deleted

How to test

Download ln.exe from https://schinagl.priv.at/nt/ln/ln64.zip and place it in the same directory as the below .bat file
Start TestDeleteReparsePoint.bat from an administrative command prompt (or grant yourself the right to create symbolic links) in your e.g. temp directory.

It builds a structure like

├───1
│   ├───iLib
│   │   ├───inc
│   │   └───src
│   ├───iLib_symlink (-> x:\sl\iLib )
│   │   ├───inc
│   │   └───src
│   └───zzz
└───sl
    ├───iLib
    │   ├───inc
    │   └───src
    ├───iLib_symlink (-> x:\sl\iLib )
    │   ├───inc
    │   └───src
    └───zzz

The two symbolic links point to x:\sl\iLib. One can test two main uses-cases

Unlinking a symbolic link:
In Winfile navigate to x:\sl\iLib\iLib_symlink and delete it. This will simply unlink the
symbolic link and does no harm to x:\sl\iLib as the original version does

Unlink as part of recursive deletion
In Winfile navigate to x:\1 and delete it. x:\1\iLib_symlink will be unlinked during the recursive operation

In both cases x:\sl\iLib keeps its content

Comments to the code

Basically there are two changes

• wfcopy.c:GetNextPair() can now skip directories by setting ATTR_SYMBOLIC link in its fd.dwFileAttributes. This change is needed during recursive deletion.
• wfcopy.c:1695 This is needed if the selected item is a reparse point

[malxau]

My reaction to each of these three PRs is the same.

Reparse points just contain arbitrary metadata that can be applied by some arbitrary component to do some arbitrary thing. The list of defined tags are in Winnt.h, but even that is incomplete, since tags are allocated to non-Microsoft companies. Looking at the list now, it might be a file that's single instanced, backed in a WIM, a OneDrive placeholder, a Store app lauch wrapper, container file, handled in usermode, etc, etc.

In theory, there's a macro to tell the difference between things that are links and things that are not links - IsReparseTagNameSurrogate. Getting there requires not just looking at the one-bit attribute, but the reparse tag to see what type of reparse point it is. But if we're doing that, it's possible to set ATTR_JUNCTION or ATTR_SYMBOLIC, and scope this logic to links that WinFile understands.

I think the logic we want is what I tried to do in #297 where the reparse tag is examined as part of enumerate, so logic later on doesn't need to assume that if the reparse attribute is set it must be a link (which is not the case.) Unfortunately that PR is stuck in purgatory.

There's also a relatively new IsReparseTagDirectory macro - historically, if a directory was a reparse point, it couldn't contain child files; since 2016 and WCI containers, it can, which implies that deletes need to enumerate into it. But whichever approach is taken, it requires the reparse tag somehow to know which action to take.

[schinagl]

All correct, but the 3 PRs try to adress obvious problems with as lean as possible changes.
They don't care which Reparse Point it is and thus rely on ATTR_REPARSE_POINT, which is available everywhere ( and enough for the PRs )

[malxau]

It might be easier to show this in code. Here's the same logic, scoped to symbolic links and junctions, once #297 is merged:

malxau@2007f2e

I think it's actually leaner, because it doesn't need to set a flag to communicate back to GetNextPair - the flag is already set.

Edit: I think it also addresses #305 , because it includes logic that would avoid enumerating into link directories in all cases.

This did show I had a bug in #297 where it applied the filter in WFFindFirst but not WFFindNext (I didn't intend to apply it in either.) That's fixed in commit 9ee462d .

[schinagl]

When I did #303 I searched for ATTR_SYMBOLIC or ATTR_JUNCTION used in GetNextPair(), but it was not there.

BUT(!): This whole WFMoveCopyDriverThread() beast does lots. So I decided to selectively switch on the 'Skipping' by reusing the free ATTR_SYMBOLIC bit, which was not used in GetNextPair().

So this raises the question: Do we always skip on ATTR_SYMBOLIC|ATTR_JUNCTION in GetNextPair()? What about search?
If this can be answered truly with yes, I am fine and #297 is smoother.

But there is still the question on #297 (This is why it is stuck): Does all the code in Winfile handle ATTR_SYMBOLIC|ATTR_JUNCTION being used? In many places it relies on GetFileAttribute() and then sets ATTR_SYMBOLIC|ATTR_JUNCTION respectively. I do not dare to change all these places

Will try a binary based on your changes during the days and will keep you posted

[schinagl]

Used #297 for a full day and took care of the special uses cases, and it was fine
I would #297 a thumbs up

If #297 goes ahead I will update my changes to use it.

[malxau]

When I did #303 I searched for ATTR_SYMBOLIC or ATTR_JUNCTION used in GetNextPair(), but it was not there.

Right, GetNextPair needs some kind of handling for it one way or another.

What about search?

That's a good (and subjective) question. My personal opinion is it probably shouldn't traverse links, since doing so will introduce duplicates and loops, and may traverse into unexpected devices eg. network shares. But others might disagree with me about that.

Does all the code in Winfile handle ATTR_SYMBOLIC|ATTR_JUNCTION being used?

The risk here is a caller expecting these to not be set when they are. I haven't seen any code that does that (yet.) Some code does the opposite and applies ATTR_ALL, thereby discarding these flags.

In many places it relies on GetFileAttribute() and then sets ATTR_SYMBOLIC|ATTR_JUNCTION respectively.

Looking at callers of GetFileAttributes, many seem to be saving/restoring (in wfcopy.c); some are testing specific bits (directory, compressed.) The most concerning are the ones adding attributes into tree nodes, because they're not using ATTR_ALL so they're setting bits whose meaning is different to how Winfile uses them (eg. FILE_ATTRIBUTE_NO_SCRUB_DATA == ATTR_JUNCTION, FILE_ATTRIBUTE_EA == ATTR_SYMBOLIC, etc.)

The cases where I know ATTR_SYMBOLIC and ATTR_JUNCTION are being set outside of this mechanism is in response to DecodeReparsePoint. That can't be completely removed since part of its purpose is to parse the link target, not just identify that it is a link. There's two callers to this function and I think one of them could be removed which doesn't care about the link target.

I do not dare to change all these places

Maybe not all at once :)

I think a lot of this needs to be changed gradually, in small and reviewable pieces. One change I'd like is to have WFFindFirst and WFFindNext apply ATTR_JUNCTION/ATTR_SYMBOLIC filters, so that logic isn't reimplemented in each caller. Another would be to use two DWORDs so there's one 32 bit value with all of the file system attributes and a second value with all of the WinFile derived attributes, since the file system folks seem to want to allocate every one of those bits. But these changes can be kept separate from the ones in these issues.

[craigwi]

Why is this change (to unlink) not in the case 'OPER_RMDIR | FUNC_DELETE' as indicated by the comment above this code?

[schinagl]

Short answer: Because when it recursivley runs it only comes along OPER_MKDIR | FUNC_DELETE, but not OPER_RMDIR | FUNC_DELETE. (which I agree, sounds weird)

Long answer: From trying to understand the code, I guess it all started with a 'recursive runner', which was meant to perform a copy of a complete tree. This involves OPER_MKDIR on the destination.
Later on the code was extended with recursive delete, and the 'recursive runnner' already available for copy was 'abused' to delete recursivley.
That's why it comes along here during recursive delete, but not with OPER_RMDIR.

OPER_RMDIR is only used for non-recursive directory deletion.... I assume.

[malxau]

Fwiw, the first time I looked at this block I had to run it under the debugger to understand it too.

Although the labels OPER_MKDIR and OPER_RMDIR are really misleading, for delete I think the first one means "confirm the action" and the second means "do the action." When recursing, confirmation happens before recursion, but actual deletion has to happen afterwards (since child objects need to be removed before the parent can be.) For link deletion, it doesn't matter, since there's no recursion.

I've been playing around with this code and I think if the first section is changed from

goto SkipThisFile;

to

dwOp = OPER_RMDIR;
goto ReturnPair;

then this block can be removed. But I've only tried the most trivial test cases, so it's possible I'm missing something.

Also, I've been trying to find a case where this matters. I think it matters with a non-fast move (across volumes), where the OPER_MKDIR phase creates the target, and the OPER_RMDIR phase deletes the source. I think the code in this PR won't handle that case, since it creates the target, leaves the source, then fails to remove the parent.

[schinagl]

Thx, this makes the code more lean.
I would have never guessed that OPER_RMDIR is used during 'SLOW_MOVE'. I only saw that during crawling down OPER_MKDIR was used.

One more thing came up, when I retested copying trees with Symbolic links: As long as one has SeCreateSymbolicLinkPrivilege CreateDirectoryEx works fine and it 'copies' symbolic links.
(Un)fortunatley I do have this enabled due to LinkShellExtension development on my home PC, but ... tada.. not on my notebook. Anyhow I upgraded MKDir to create a symbolic link in developer mode.

@malxau : Is there a non documented way that CreateDirectoryEx can be enabled to respect SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE? If not I hope my solution is sufficient.

[malxau]

Ouch, that hurts. No, I don't think there's a way to make CreateDirectoryEx work in this case. Traditionally (without SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE), kernel32/kernelbase would attempt to enable SeCreateSymbolicLinkPrivilege and if that fails, fail the operation. With SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE, it continues if SeCreateSymbolicLinkPrivilege cannot be enabled, effectively letting kernel decide how to proceed. It looks like both CreateDirectoryEx and the CopyFile family are both capable of creating symbolic links but weren't updated to continue if the privilege can't be enabled, and both of these do tons of processing in usermode, so it's not possible to get them to perform their task without this privilege (well, without hooking RtlAcquirePrivilege and altering its return code.)

[craigwi]

"Skip" doesn't seem like the right point; we are returning the reparse point as a directory, but not recursing.

[schinagl]

A better comment would be
'Return reparse point and delete it via OPER_RMDIR'

[malxau]

Ouch, that hurts. No, I don't think there's a way to make CreateDirectoryEx work in this case. Traditionally (without SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE), kernel32/kernelbase would attempt to enable SeCreateSymbolicLinkPrivilege and if that fails, fail the operation. With SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE, it continues if SeCreateSymbolicLinkPrivilege cannot be enabled, effectively letting kernel decide how to proceed. It looks like both CreateDirectoryEx and the CopyFile family are both capable of creating symbolic links but weren't updated to continue if the privilege can't be enabled, and both of these do tons of processing in usermode, so it's not possible to get them to perform their task without this privilege (well, without hooking RtlAcquirePrivilege and altering its return code.)

[malxau]

I think this should check that it's a symbolic link reparse tag. I know that seems gross, but I think it's needed anyway, because....

[malxau]

...this seems to be linking the new object to the old object (the one being deleted in a move or the one being copied.) It presumably wants to extract the reparse data from pSrc, find where pSrc is linked to, and link pName to the same destination as pSrc, not pSrc itself.

[schinagl]

Sry. My bad. It was too late :-)

[schinagl]

The above comit solves the problem for directories but not yet for files. Files are still 'unrolled'. Have to investigate.
Also have to check what else was copied by CreateDirectoryEx(). Attribs? ACLs?

[schinagl]

@malxau: Do you have a smart way to copy the ACLs in MKDir, when going the manual symbolic link way?

[malxau]

CreateDirectoryEx appears to copy:

• File attributes
• EAs
• Named streams
• Reparse buffers

Note that it does not copy security descriptors - the caller is free to provide one, but without that, it just inherits from the target parent. Within reparse buffers, it's doing extra work not for symbolic links (which are just passed verbatim) but to detect mount points and update the mount manager via SetVolumeMountPoint. As you know, directory junctions and mount points are the same thing in terms of the reparse point, the only difference is what they link to, so it's looking for something that links to a volume as opposed to a directory.

Anyway, my suggestion is this PR should be broken in half: half for the original delete/copy change (which I think we're getting to consensus on) and the other half for unprivileged symlink support (which may have more changes.) I think the first has to be freestanding anyway, because a user running one of {Win7, Win8, Win10 without developer mode} is going to get that behavior, so if there are cases where a copy fails with privilege not held we need to make sure the experience is decent.

[schinagl]

Good point. This really explodes.
Will split it and later we move on with developer mode support for symbolic link creation.

[schinagl]

Reverted to the point where it only solves the issue of following reparse points during deletion and copy