PathUtilities misbehavior on linux

[rmannibucau]

Hi,

https://github.com/dotnet/sdk/blob/main/src/Common/PathUtilities.cs#L24 uses mkdir on linux and Directory.CreateDirectory on windows.
This leads to a behavior difference where some directory can not be created if TMPDIR is a path which doesn't exist on linux (it fails) and it passes on windows.

I propose to drop the linux specificity and keep using the Directory API + chmod after directory creation if 0700 perms are desired.

[KalleOlaviNiemitalo]

One could perhaps use Directory.CreateDirectory(String, UnixFileMode) within #if NET7_0_OR_GREATER. I'm not sure whether PathUtilities.cs is built in any project that targets .NET Standard.

[nagilson]

Thank you for your thoughtful issue.

In reference to https://github.com/dotnet/sdk/pull/45633/files#diff-2c7fe79f19abb90af52e305c3b339ac410b00889d4a4acd5aae1689898cf7ace:

mkdir returns the status code 1 if the directory already exists, while it looks like Directory.CreateDirectory will always return the directory. Docs. I'm not confident we can do this. If you can refute this, please let me know.

We need to be 100% certain that, the directory did not exist before hand and has no possibility of existing beforehand. That's not refutable. If we use an if statement to check if the directory exists, that is not good enough, it could be created between the two lines of code run. I understand wanting to have linux and windows behavior be the same. But my question is, why would you want TMPDIR to be a dir that DNE? I'm not certain on the idea that its wrong for the SDK to fail on one OS if the OS standard environment variable is set to be something 'bad'.

[kasperk81]

mkdir and CreateDirectory difference is clear from the docs, but why does it matter isn't

That's not refutable.

why is it not the case on windows? if something sneaky is racing with Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()) and windows is fine with ignoring this extremely unlikely synthetic corner case, unix can do the same.

[rmannibucau]

We need to be 100% certain that, the directory did not exist before hand and has no possibility of existing beforehand.

I can hear that but it is 100% not the case today on windows so you can reverse my proposal and say windows must be fixed.
Also note that it doesn't do that, if the dir doesn't exist it does create it only by one level - my issue.
So if the file doesn't exist and its parent neither it fails which is the bug for me.

why would you want TMPDIR to be a dir that DNE

There are cases you can not write in /tmp - k8s with a proper security context and mounting /tmp is a workaround but not a clean solution since all the .NET code uses TMPDIR except these lock files. It is also quite bad to use /tmp cause depending the OS setup you can get the file dropped randomly due to /tmp pressure. The good practise in all env is to set a dedicated temp folder per app. What is common is to set a base work dir and configure the temp dir in this work dir but at startup it doesn't exist until you create it with a mkdir -p yourself which is not possible when you do a minimal container without even a shell for security reasons.

I'm fine keeping mkdir but it should be able to try to create the parents, in particular in the case of a custom TMPDIR var - maybe not in default case.

[KalleOlaviNiemitalo]

chmod after mkdir has the risk that another user might be able to access the directory between those calls and maliciously create files or symlinks in it. Even if Guid.NewGuid().ToString() is not predictable, such an attack could succeed if the .NET SDK process is somehow slowed down so that the other user has time to read the directory name. This risk does not apply to Windows, where each user has a separate TEMP directory.

[rmannibucau]

This risk does not apply to Windows, where each user has a separate TEMP directory.

So if on linux the TMPDIR var is set you can assume safe to use the safe code if you assume it on windows

[KalleOlaviNiemitalo]

when you do a minimal container without even a shell for security reasons

Do you run .NET SDK in such a minimal container?

I'm fine keeping mkdir but it should be able to try to create the parents

I suppose it could be changed to do Directory.CreateDirectory(Path.GetTempPath()) just before the recursive return CreateTempSubdirectoryRetry(attemptNo + 1);. That would not cause additional system calls in the usual case where the temp directory exists already.

[rmannibucau]

Do you run .NET SDK in such a minimal container?

Not that minimal but locked (securityContext) so it behaves almost the same (/tmp is not writable and TMPDIR is set to /foo/work/tmp with /foo/work being mounted).

I suppose it could be changed to do Directory.CreateDirectory(Path.GetTempPath()) just before the recursive return CreateTempSubdirectoryRetry(attemptNo + 1);.

Sounds good to me.

[nagilson]

mkdir and CreateDirectory difference is clear from the docs, but why does it matter isn't

That's not refutable.

why is it not the case on windows? if something sneaky is racing with Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()) and windows is fine with ignoring this extremely unlikely synthetic corner case, unix can do the same.

I can hear that but it is 100% not the case today on windows so you can reverse my proposal and say windows must be fixed.

tmp on linux is world writeable, tmp on windows is user secured permissions.

[nagilson]

when you do a minimal container without even a shell for security reasons

Do you run .NET SDK in such a minimal container?

I'm fine keeping mkdir but it should be able to try to create the parents

I suppose it could be changed to do Directory.CreateDirectory(Path.GetTempPath()) just before the recursive return CreateTempSubdirectoryRetry(attemptNo + 1);. That would not cause additional system calls in the usual case where the temp directory exists already.

I think I'm ok with that. I will probably have to run this through some security reviews though to check it in (which sorry @kasperk81, this was part of the delay on the HOME pr.)

[rmannibucau]

FTR

tmp on linux is world writeable

this is an assumption which is not always true (even without containers) so apps/libs can't assume it even if it is most of the time

[kasperk81]

tmp on linux is world writeable

new implementation in #45633 will create the directory with permissions (atomically) and retry until it succeeded. the "exists" behavior is same as windows, which is not the concern if i understood it correctly? with NewGuid, it's hard to fathom there can ever be a conflict.