Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Files#createTempDir stopped working on Windows with 32.0.0 #6535

Closed
snuyanzin opened this issue Jun 3, 2023 · 17 comments
Closed

Files#createTempDir stopped working on Windows with 32.0.0 #6535

snuyanzin opened this issue Jun 3, 2023 · 17 comments
Assignees
Labels
Milestone

Comments

@snuyanzin
Copy link

After update to guava 32.0.0
on Windows we start having such exception

 Caused by: java.lang.UnsupportedOperationException: 'posix:permissions' not supported as initial attribute
            at sun.nio.fs.WindowsSecurityDescriptor.fromAttribute(WindowsSecurityDescriptor.java:358)
            at sun.nio.fs.WindowsFileSystemProvider.createDirectory(WindowsFileSystemProvider.java:492)
            at java.nio.file.Files.createDirectory(Files.java:674)
            at java.nio.file.TempFileHelper.create(TempFileHelper.java:136)
            at java.nio.file.TempFileHelper.createTempDirectory(TempFileHelper.java:173)
            at java.nio.file.Files.createTempDirectory(Files.java:950)
            at com.google.common.io.TempFileCreator$JavaNioCreator.createTempDir(TempFileCreator.java:102)
            at com.google.common.io.Files.createTempDir(Files.java:439)

seems it is related to
feb83a1

@cpovirk
Copy link
Member

cpovirk commented Jun 5, 2023

Sorry about that. I was aware that some version of Windows had gotten some kind of POSIX support but probably also had heard enough during our jimfs work that I should have realized that that didn't solve this. A teammate ended thinking to test Windows after the release, at which point he confirmed that it didn't work. But all we've done so far is update the release notes, add a warning to the Javadoc at head, and try to read some about how to create the directory securely under Windows:

In response to a similar problem, the JUnit people took the position that createTempDirectory is secure by default, even if the caller doesn't pass any restrictions on permissions. So far, I haven't found any documentation that confirms that (and the first link above implies that opposite), though I can say that I haven't found a way to make it insecure on Linux, and the Windows behavior that my teammate saw looked secure, as well. We would really like to know for sure, though, given how much Guava users have already been through for this vulnerability....

After some more searching around, I've come across https://codeql.github.com/codeql-query-help/java/java-local-temp-file-or-directory-information-disclosure/, which at least suggests that Windows is different than Unix. It says that Windows provides each app with its own temporary directory and so you don't have to worry about permissions there. But that source would be more reassuring if it didn't suggest that you can change the permissions of an existing directory and then rely on it to be secure... and probably also if it didn't assume that all non-POSIX systems have secure temporary directories.

I found some more information that I haven't confirmed on Stack Overflow. (And I found that the Javadoc for java.io.File still suggests that the default is typically "C:\WINNT\TEMP"... :))

I see from the Calcite issue you linked (thanks!) that this comes from a file in embedded-redis, a project that hasn't had a commit in over 4 years. So I assume it's not going to get fixed there.

I also see that you have a workaround. I'd be interested to hear:

  • from other users who are affected
  • from anyone who can provide more official-looking guarantees of how to get secure behavior under Windows

@basil
Copy link

basil commented Jun 6, 2023

I'd be interested to hear […] from other users who are affected

See JENKINS-71375. There are about 21,984 installations (across all operating systems) of the Jenkins Artifactory plugin, which is subject to jfrog/build-info#737 on Windows. As a result, Jenkins Artifactory plugin users who are running Jenkins on Windows are impacted by this issue.

@cpovirk
Copy link
Member

cpovirk commented Jun 7, 2023

Thanks, and sorry: That's pretty impressively bad. I'll see what I can do about this tomorrow.

If anyone else ends up here and has any implementation tips, let me know. I'm tentatively planning to try to set ACL permissions on the directory, even though it's probably already in a secure directory, just to be safe. (By the time I'm convinced that I can prove that the code is running on NTFS or similar, I'm hoping it's not much harder to set the permissions, too.)

@basil
Copy link

basil commented Jun 7, 2023

I think we should only be setting POSIX attributes when FileSystem#supportedFileAttributeViews contains posix. The default value of java.io.tmpdir on Windows is the return value of GetTempPathW, which is defined to return the first of these paths:

  1. The path specified by the TMP environment variable.
  2. The path specified by the TEMP environment variable.
  3. The path specified by the USERPROFILE environment variable.
  4. The Windows directory.

(This blog post goes into the gory details regarding why both TMP and TEMP are set.)

My Windows 10 system had TMP and TEMP defined to %USERPROFILE%\AppData\Local\Temp, to which only SYSTEM, the current user, and members of the Administrators group had permissions. My Windows 2000 system defined these variables to %USERPROFILE%\Local Settings\Temp with the same permissions.

So it appears that the common case is for the directory returned by GetTempPathW to be secure, but one could imagine various corner cases:

  • The user has changed the permissions on the default TMP/TEMP/USERPROFILE to allow broader access
  • The user has redefined TMP/TEMP/USERPROFILE or java.io.tmpdir to a directory with broader access
  • The user has redefined TMP/TEMP/USERPROFILE or java.io.tmpdir to a directory on a less secure filesystem, like FAT32
  • The user has installed Windows on a less secure filesystem, like FAT32 (I think I did this once with Windows 2000, but not sure if that is still supported nowadays)

OpenJDK itself does not attempt to deal with these corner cases. If this approach is good enough for OpenJDK, it might be good enough for Guava as well. If not, it might be best to simply fail fast with e.g. UnsupportedOperationException if we detect a corner case. An approach that correctly handles all of these corner cases sounds like it would be challenging to implement (and test).

@cpovirk
Copy link
Member

cpovirk commented Jun 7, 2023

Thanks, that's all reassuring! I think I have figured out how to create the file with the desired ACL in the first place (by implementing FileAttribute myself with the name "acl:acl" and the value of the desired ACL). (And now that I know how to do it, I can find the ~2 total places on the Internet that it seems to be explained: 1, 2 :)) I have commandeered my wife's Windows machine to test it out. If it falls through, I'll be reasonably comfortable just calling createTempDirectory/createTempFile with no attributes, as you suggest.

copybara-service bot pushed a commit that referenced this issue Jun 7, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538491548
copybara-service bot pushed a commit that referenced this issue Jun 7, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686. (It sure would be convenient to have Windows CI set up!)

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
@cpovirk
Copy link
Member

cpovirk commented Jun 7, 2023

OK, I have pushed #6540 (but you should ignore the PR with the number just before that one...) with my attempt at a fix. If any Windows users are in a position to easily test whether it solves the problems of 32.0.0, please give it a shot. Otherwise, I will probably release 32.0.1 with that change and wait to hear comments after release :)

@creckord
Copy link

creckord commented Jun 7, 2023

I tried it out on Windows 10, but the folder is created without create/write file permissions, so this fails:

    File temp = Files.createTempDir();
    File f = new File(temp, "test.txt");
    f.createNewFile();

(throws IOException: Permission denied)

This is the ACL that is created:
image

Looking at the PR, I think this is just a matter of a missing WRITE_DATA permission in the ACL.
Edit: I can confirm that this simple test works for me when I include WRITE_DATA. This results in Windows showing me the "Vollzugriff" (full access) flag for the directory, and I can create and edit files. I also ran the test on a FAT32 directory, which worked as well, but obviously without any effects of the ACL.

@cpovirk
Copy link
Member

cpovirk commented Jun 7, 2023

Thank you! Yes, when I printed out all the permissions that a file had by default, I missed a newline before the first one, so I had WRITE_DATA hanging out on the same line as the header, and so I overlooked it. So hopefully that's the only mistake. I've pushed a new commit to the PR that adds WRITE_DATA. But, for some reason, I'm seeing it on the branch yet not in the PR.... [edit: It's finally showing in the PR, too.]

(I'll also go back and add a test that we can create a file in the temporary directory, though that won't help much until we implement #2686.)

copybara-service bot pushed a commit that referenced this issue Jun 7, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686. (It sure would be convenient to have Windows CI set up!)

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
copybara-service bot pushed a commit that referenced this issue Jun 7, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686. (It sure would be convenient to have Windows CI set up!)

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
- Fixes #2686
- In some sense addresses #2130, but I'm going to leave that open to track removing the suppressions.
- Provides better testing for the fix for #6535

RELNOTES=n/a
PiperOrigin-RevId: 538841996
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
- Fixes #2686
- In some sense addresses #2130, but I'm going to leave that open to track removing the suppressions.
- Provides better testing for the fix for #6535

RELNOTES=n/a
PiperOrigin-RevId: 538841996
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
- Fixes #2686
- In some sense addresses #2130, but I'm going to leave that open to track removing the suppressions.
- Provides better testing for the fix for #6535

RELNOTES=n/a
PiperOrigin-RevId: 538841996
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
- Fixes #2686
- In some sense addresses #2130, but I'm going to leave that open to track removing the suppressions.
- Provides better testing for the fix for #6535

RELNOTES=n/a
PiperOrigin-RevId: 538862954
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686 (Yay, we have Windows CI set up!)
- #2130

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686 (Yay, we have Windows CI set up!)
- #2130

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686 (Yay, we have Windows CI set up!)
- #2130

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
copybara-service bot pushed a commit that referenced this issue Jun 8, 2023
Based on [some discussions in GitHub](#6535 (comment)), I'm under the impression that Windows would create the temporary directory/file in a secure location even if our call to `java.nio.file.Files.createTempDirectory`/`createTempFile` passes no ACL attribute. However, in case we are in an unusual situation (Linux with NFS temporary directory???) in which ACLs are supported but the temporary directory is _not_ a secure location, I've arranged for the file to be created with an ACL that grants permissions only to the current user. I set the user's permissions to the ones that I saw on a file created in the temporary directory under Java's default settings, and I didn't do anything to set the additional permissions I was seeing for administrators. The resulting file's permissions look plausibly correct in the Windows property dialog, if slightly different than what I get when creating a file/directory myself through Explorer.

Fixes #6535

Relates to:
- #4011
- #2575
- #2686 (Yay, we have Windows CI set up!)
- #2130

RELNOTES=`io`: Fixed `Files.createTempDir` and `FileBackedOutputStream` under Windows.
PiperOrigin-RevId: 538492945
@cpovirk cpovirk added this to the 32.0.1 milestone Jun 9, 2023
@cpovirk
Copy link
Member

cpovirk commented Jun 9, 2023

nikclayton added a commit to tuskyapp/Tusky that referenced this issue Jun 10, 2023
It bundles Guava 32.0.0 which has a bug on Windows where temporary directories can't be created, causing tests to fail.

See google/truth#1137 and google/guava#6535
nikclayton added a commit to tuskyapp/Tusky that referenced this issue Jun 10, 2023
It bundles Guava 32.0.0 which has a bug on Windows where temporary directories can't be created, causing tests to fail.

See google/truth#1137 and google/guava#6535
@JLLeitschuh
Copy link

JLLeitschuh commented Jun 12, 2023

After some more searching around, I've come across codeql.github.com/codeql-query-help/java/java-local-temp-file-or-directory-information-disclosure, which at least suggests that Windows is different than Unix.

I wrote this documentation, and wrote the query, and I'm just going based upon what I heard from someone else. I've never actually confirmed this myself. I don't ever develop code on/for windows

@cpovirk
Copy link
Member

cpovirk commented Jun 12, 2023

Small world :) Thanks.

@radistao
Copy link

still reproduced for me on Windows with guava 32.1.2 😢
Note: when roll-backed to 31.1 - works fine, but neither 32+ version works;
Env:

  • Windows Server 2016
  • JRE 11.0.20.1

@cpovirk
Copy link
Member

cpovirk commented Sep 29, 2023

Thanks, can you point the failure stack trace? I'm wondering if you're seeing #6634 (which I have a pending fix for) or some other issue.

@radistao
Copy link

radistao commented Sep 29, 2023

Unfortunately, the code is a bit more complex: there is a static block inside class ConfigurationFactory:

static {
    if (SystemUtils.IS_OS_WINDOWS && System.getProperty("hadoop.home.dir") == null) {
        winutilsDir = Files.createTempDir();
        createWinUtilsFile();
    }
}

and this class can't be loaded when first time referenced at runtime with the error:

Caused by: java.lang.NoClassDefFoundError: Could not initialize class com.example.ConfigurationFactory

I tried to reproduce with the bare code (see https://github.com/radistao/guava-createTempDir), where only 'ConfigurationFactory' with static block exists, but not reproducible.
Unfortunately i don't have an access to Windows environment to develop and debug (only build and run), otherwise i'd try to debug in the full project.

The only thing i can confirm: in our jar (we use fat jar with bundled spring boot application) only guava has different version, the rest of the code an dependencies remained the same:
image

If i have access to Windows development environment soon - i'll try to reproduce in debug mode.

@radistao
Copy link

radistao commented Oct 3, 2023

today i tried to debug in Intellij IDEA and it works successfully (team directory is created), but when i build to a fat jar and run as a spring boot application, i observe the following errors:

Caused by: 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)
at my.domain.ConfigurationFactory.(ConfigurationFactory.java:30)
... 16 common frames omitted
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)
... 18 common frames omitted
Caused by: java.io.IOException: MY-HOST-NAME$: The trust relationship between this workstation and the primary domain failed.

at java.base/sun.nio.fs.WindowsUserPrincipals.lookup(Unknown Source)
at java.base/sun.nio.fs.WindowsFileSystem$LookupService$1.lookupPrincipalByName(Unknown Source)
at com.google.common.io.TempFileCreator$JavaNioCreator.userPermissions(TempFileCreator.java:153)
at com.google.common.io.TempFileCreator$JavaNioCreator.(TempFileCreator.java:138)
at com.google.common.io.TempFileCreator.pickSecureCreator(TempFileCreator.java:65)
at com.google.common.io.TempFileCreator.(TempFileCreator.java:51)
... 18 common frames omitted

The trust relationship between this workstation and the primary domain failed.
- this is something new

again: this happens when first time initializing class ConfigurationFactory which has a static block with Files.createTempDir

Yes, we use Windows Active Directory configuration, but currently i'm logged as non-domain user with admin privileges, and i run the application on behalf of the same user.

Maybe also worth to mention: we run the application using Windows Service Wrapper for .NET4 (https://github.com/winsw/winsw), and the application registered and executed as a Windows Service.

Hope this will shed some light on the issue.

@cpovirk
Copy link
Member

cpovirk commented Oct 3, 2023

Thanks, that does sound likely to be the same problem as #6634. I have a fix out for review.

@radistao
Copy link

radistao commented Oct 3, 2023

ah, i missed that #6634 also refers to Windows Services. so yes, seems similar (at least now i was able to collect the logs to prove it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment