From Guava 32.0.1-jre, Files.createTempDir() and FileBackedOutputStream can fail or create un-deletable directories/files when used from a Windows service

[fmarot]

Hello I rencently upgraded from 31.1-jre to 32.0.1-jre.
FYI 32.0.0-jre was KO because of the then-corrected-in-32.0.1-jre error: java.lang.UnsupportedOperationException: 'posix:permissions' not supported as initial attribute. So let's not talk about this version.

In 32.0.1-jre the method Files.createTempDir() has been modified and a program RUNNING ON WINDOWS is now unable to delete its own folders created by this call.

Here is a sample creating a folder with java's own "createTempDirectory" and deleting it (this works fine) and then the same with Guava and it cannot delete it's own folder created with guava.

        @Test
	public void testApp() throws IOException {
		Path tempDir = java.nio.file.Files.createTempDirectory("temp");
		displayDirectoryPermissions(tempDir);
		boolean isDeleted = tempDir.toFile().delete();
		assertTrue(isDeleted);

		System.out.println(" - - - - - - - - - ");

		Path tempDir2 = Files.createTempDir().toPath();
		displayDirectoryPermissions(tempDir2);
		boolean isDeleted2 = tempDir2.toFile().delete();
		assertTrue(isDeleted2);
	}

	private static void displayDirectoryPermissions(Path directory) throws IOException {
        System.out.println("Informations sur les droits d'accès du répertoire : " + directory.toAbsolutePath());

		AclFileAttributeView view = java.nio.file.Files.getFileAttributeView(directory, AclFileAttributeView.class);
		System.out.println(view.toString());

		view.getAcl().forEach(it -> System.out.println(it.toString()));
    }

Here are the (desidentified) trace and there is a huge difference beween both calls:

Informations sur les droits d'accès du répertoire : e:\xxxxxxx\target\temp18063228888024228555
sun.nio.fs.WindowsAclFileAttributeView@7494f96a
BUILTIN\Administrators:READ_DATA/WRITE_DATA/APPEND_DATA/READ_NAMED_ATTRS/WRITE_NAMED_ATTRS/EXECUTE/DELETE_CHILD/READ_ATTRIBUTES/WRITE_ATTRIBUTES/DELETE/READ_ACL/WRITE_ACL/WRITE_OWNER/SYNCHRONIZE:ALLOW
BUILTIN\Administrators:FILE_INHERIT/DIRECTORY_INHERIT/INHERIT_ONLY:ALLOW
NT AUTHORITY\SYSTEM:READ_DATA/WRITE_DATA/APPEND_DATA/READ_NAMED_ATTRS/WRITE_NAMED_ATTRS/EXECUTE/DELETE_CHILD/READ_ATTRIBUTES/WRITE_ATTRIBUTES/DELETE/READ_ACL/WRITE_ACL/WRITE_OWNER/SYNCHRONIZE:ALLOW
NT AUTHORITY\SYSTEM:FILE_INHERIT/DIRECTORY_INHERIT/INHERIT_ONLY:ALLOW
NT AUTHORITY\Authenticated Users:READ_DATA/WRITE_DATA/APPEND_DATA/READ_NAMED_ATTRS/WRITE_NAMED_ATTRS/EXECUTE/READ_ATTRIBUTES/WRITE_ATTRIBUTES/DELETE/READ_ACL/SYNCHRONIZE:ALLOW
NT AUTHORITY\Authenticated Users:DELETE:FILE_INHERIT/DIRECTORY_INHERIT/INHERIT_ONLY:ALLOW
BUILTIN\Users:READ_DATA/READ_NAMED_ATTRS/EXECUTE/READ_ATTRIBUTES/READ_ACL/SYNCHRONIZE:ALLOW
BUILTIN\Users:FILE_INHERIT/DIRECTORY_INHERIT/INHERIT_ONLY:ALLOW
 - - - - - - - - - 
Informations sur les droits d'accès du répertoire : e:\xxxxxxx\target\3980498532511240827
sun.nio.fs.WindowsAclFileAttributeView@32502377
WINDOWS-DOMAIN\computerName$:READ_DATA/WRITE_DATA/APPEND_DATA/READ_NAMED_ATTRS/WRITE_NAMED_ATTRS/EXECUTE/DELETE_CHILD/READ_ATTRIBUTES/WRITE_ATTRIBUTES/DELETE/READ_ACL/WRITE_ACL/WRITE_OWNER/SYNCHRONIZE:FILE_INHERIT/DIRECTORY_INHERIT:ALLOW

[cpovirk]

Thanks for all the details.

At first glance, I'm puzzled: We test that we can delete the directory, and we have had our tests running under Windows. We have also tested manually under Windows, as have other users, and we've heard of apps that started working again at 32.0.1. So there must be some variation between systems that accounts for this.

One thing that I'd wondered was whether the system property user.name is the right one to use. Maybe it's not? It looks like the code ended up granting permissions to a user "computerName$" (if I'm understanding right). Maybe that's not actually the user that we need? (I do think I found that Java would fail if we tried to set a user that does not exist, at least in the testing I did.) It's interesting that the java.nio.file.Files.createTempDirectory() output doesn't appear to mention a specific user at all (again, if I'm understanding right). I think I'd seen a specific user listed in my testing.

One frustrating part of this is that we might not need to set explicit permissions at all: The Windows temporary directory is very likely to be secure, and the Java's Linux implementation appears to set restricted permissions by default (but its docs are coy about this). Maybe I should just rip out all the permissions code. I've been hesitant to do so because it would look quite bad if such a change somehow reintroduced the security bug. (And of course now we've already had a bunch of releases in quick succession....)

Does anything about the username issue give you any ideas? Does anyone else have thoughts?

[cpovirk]

"Computername$ implies that it is coming from something running as local system. Local admins can also do this."

Does that sound like the situation you're in?

Hmm, and regardless, could you have a look at the "e:\xxxxxxx\target" directory and see if it's world-readable? If it's world-readable, then that shows that we do need to set directory permissions for maximum security in this case. If it's not, then that's another data point in favor of the idea that the Windows temporary directory is always secure.

[fmarot]

Hello cpovirk,

I'm sorry I should really have mentionned a few things first in my report:
1- I'm running a Jenkins worker as a Windows Service with default rights so it runs as "local system" (see the bottom of the screenshot)
2- I explicitly set the Java temp directory to -Djava.io.tmpdir=e:\xxxxxxx/target (the classic Maven build directlry) so I do not bother having to clean temporary files left in the temp dir as it is handled by a "mvn clean"

I added this code in the program to obtain the username:

com.sun.security.auth.module.NTSystem NTSystem = new com.sun.security.auth.module.NTSystem();
System.out.println("NTSystem.getName(): " + NTSystem.getName());

System.out.println("System.getProperty(user.name): " + System.getProperty("user.name")); //platform independent 

and it leads to this output:

NTSystem.getName(): SYSTEM
System.getProperty(user.name): COMPUTERNAME$

(yes, COMPUTERNAME is uppercase here where it is lowercase in the ACL list)

Here are the info regarding the 'target' directory rights: it seems world-readable:

[cpovirk]

Wow, I don't even remember hearing about the javax.security.auth classes before. I wonder if there's a way to get at the value of NTSystem.getName() through non-internal APIs. Alternatively, maybe Guava should automatically map a user.name of COMPUTERNAME$ to SYSTEM and see what happens....

At this point, I would love to get away with keeping things as they are. And we might get away with it if you're in a position to migrate off our method and onto java.nio.file.Files.createTempDirectory. But since you mentioned Jenkins, I assume that it's Jenkins that contains the call and thus Jenkins that would have to perform the migration? (Or rather, JENKINS-71375 suggests that the change needs to happen in JFrog, a dependency of Jenkins....)

Aside from my idea of mapping COMPUTERNAME$ to SYSTEM, I could also see making Guava read a system property (or maybe environment variable, which feels marginally safer, since Java doesn't let programs mutate environment variables) that lets you disable setting the ACL. Would you be in a position to set the system property or environment variable?

And, similarly, for the idea of rewriting user.name: Are you able to test running your Java app with -Duser.name=SYSTEM and seeing if that helps? (Hopefully it's not too hard to set that option on the correct VM: I don't know how many different Java processes are involved when you're running Jenkins. Maybe the easier approach would be to call System.setProperty("user.name", "SYSTEM"); sometime before using Files.createTempDir (and before using FileBackedOutputStream, but you probably don't use that in the first place).

[cgdecker]

I'm sure there's context I'm not remembering, but just to double-check: did we determine that it is in fact necessary to explicitly set the ACL when creating temp dirs on Windows, as opposed to passing no FileAttributes at all and letting Windows do its default thing?

[fmarot]

Hello,
a few comments & answer below:

to cgdecker: I'm not in Guava team but I know that the default windows temp dir is user-specific. So from my (limited) point of view, there is not really a need to set anything specific. In my case I explicitly tell Java to use another dir (through -Djava.io.tmpdir=e:\xxxxxxx\target ) so responsibility for setting limited rights could be on my side. I would find it fair.

to cpovirk 1-: please do not modify anything for me. I already simply moved my tests away from using Guava's createTempDir() to java.nio.file.Files.createTempDirectory(). That was an easy move. Ijust reported the bug so that you know there is a problem others may face. It's not a blocker anymore for me.

to cpovirk 2-: you were right, launching my tests with -Duser.name=SYSTEM corrected the problem. The process was able to correctly delete it's own temporary dir.

for the record, the problem mentioned in JENKINS-71375 is specific to version 32.0.0-jre and was corrected in 32.0.1-jre and I think that only daemon services (and maybe especially the ones changing the default temp dir like I did) will be hurt with the same bug as described here.

[cpovirk]

Thanks for the additional details and testing.

It does seem extremely likely that plain java.nio.file.Files.createTempDirectory is secure. But the Linux-related JDK docs are noncommittal, saying something like like "the file/directory created might have more restricted permissions than you asked for." That sounds like it might be referring to the umask, but I was seeing even more restricted permissions than the umask asked for (which is good from the security perspective). Perhaps because of the ambiguity, some docs suggest setting the permissions explicitly. And then Windows is its own story, with the temporary directory itself normally secure, as you said. Here's some previous discussion.

I'm going to re-title this bug to reflect that it applies just to the daemon case. If we end up hearing from more affected users, we can reevaluate. Thanks for your flexibility.

[Uli-Tiger]

@cpovirk
We run into a similar issue. Root cause is the same, but we can not even create a temp dir.
We have here a Jenkins based CI running on windows. The jobs are executed by Jenkins nodes which are installed as windows services, hence running as windows SYSTEM account, which is the default service user under windows.
Some test code calls Files.createTempDir(), which internally resolves system property user.name to PIPELINE$ (PIPELINE is the computer name) instead of SYSTEM. Hence creating of temp dir fails, as there is no user named PIPELINE$.
we see then the following exception

java.lang.IllegalStateException: Failed to create directory
	at com.google.common.io.TempFileCreator$JavaNioCreator.createTempDir(TempFileCreator.java:110)
	at com.google.common.io.Files.createTempDir(Files.java:438)
	[...]
Caused by: java.io.IOException: Could not find user
	at com.google.common.io.TempFileCreator$JavaNioCreator.lambda$userPermissions$4(TempFileCreator.java:178)
	at com.google.common.io.TempFileCreator$JavaNioCreator.createTempDir(TempFileCreator.java:107)
Caused by: java.nio.file.attribute.UserPrincipalNotFoundException
	at java.base/sun.nio.fs.WindowsUserPrincipals.lookup(WindowsUserPrincipals.java:148)
	at java.base/sun.nio.fs.WindowsFileSystem$LookupService$1.lookupPrincipalByName(WindowsFileSystem.java:247)
	at com.google.common.io.TempFileCreator$JavaNioCreator.userPermissions(TempFileCreator.java:153)
	at com.google.common.io.TempFileCreator$JavaNioCreator.<clinit>(TempFileCreator.java:138)
	at com.google.common.io.TempFileCreator.pickSecureCreator(TempFileCreator.java:65)
	at com.google.common.io.TempFileCreator.<clinit>(TempFileCreator.java:51)

windows version

Edition	Windows Server 2022 Standard
Version	21H2
Installed on	‎7/‎18/‎2022
OS build	20348.169

trying to workarround by setting user.name to SYSTEM as java argument when invoking test command

[rishiraj88]

Across various OSs, there are different environment variable names for username. For example, USERNAME in Windows and USER in Linux.

[cpovirk]

Thanks. USERNAME is a possibility.

Are any of the Windows Services users in a position to see what ProcessHandle.current().info().user().get() returns? That might be the true answer.

[cpovirk]

I'm going to implement that approach now, since the Stack Overflow question sounds like just the same situation.

[cpovirk]

@fmarot @Uli-Tiger

Which version of Java are those of you who are affected using? The fix I've been working on would work for Java 9+. If you need support for Java 8, then I would need to add another fix specific to that case.

[fmarot]

thanks @cpovirk , I'm using Java11

[Uli-Tiger]

thanks @cpovirk , we are using java 17 LTS

[cpovirk]

Thanks, I'll stick with the Java 9+ solution for now, then.

Also, I've retitled this bug to reflect that even creating the directories can fail.

If all goes well, I'll submit the fix this week. Then it's just a question of how soon we do a release. How did the workaround of setting user.name go?

[cpovirk]

Aside: Somehow my Google search for "ProcessHandle.current().info().user().get()" turns up only one web page. Luckily, my earlier, different search had turned up the Stack Overflow answer with that content anyway... somehow. This was almost as bad as finding a web page that explains how to set an ACL in the first place :)

[fmarot]

Copy pasting from a previous message above, it seems the user.name workaround worked well in my tests at the time ;)

: you were right, launching my tests with -Duser.name=SYSTEM corrected the problem. The process was able to correctly delete it's own temporary dir.

[cpovirk]

Oops, sorry, I'd seen that but had wanted to check with @Uli-Tiger, since this new problem was slightly different. But I do still expect your workaround to address it.

[Uli-Tiger]

Thanks, I'll stick with the Java 9+ solution for now, then.

Also, I've retitled this bug to reflect that even creating the directories can fail.

If all goes well, I'll submit the fix this week. Then it's just a question of how soon we do a release. How did the workaround of setting user.name go?

Because the java process is launched from another process, i couldn't easily inject that system property. Instead i just let the Jenkins service run with an ordinary Admin User instead of the local service account. That works now fine.

[cpovirk]

For anyone who's following here but not following our repo as a whole: The fix for this is part of last week's https://github.com/google/guava/releases/tag/v32.1.3. If you see any problems with that version, please let us know.