Improve handling of dynamicaly extracted native library

[nextgens]

1. Problem Statement

JNA creates a temporary folder at a known location (harcoded path: /tmp/jna) to do its deed.
Unlike the system's temporary folder, it doesn't set the sticky bit on it... so on multi user systems, several problems arise :

• depending on the umask of the user of the first invocation, the second user might or might not be able to write in the said folder. This materializes as an InvocationTargetException with a root cause of an IOException (as seen on https://stackoverflow.com/questions/8655712/java-error-failed-to-create-temporary-file-for-jnidispatch-library/11461767#11461767 or Fix regression: add relative jna tmpdir path to wrapper if not existing hyphanet/fred#633)
• it creates a security vulnerability (TOCTOU) as highlighted in Review vulnerabilities report #914

2. Suggested changes

I've located at least two separate occurences of the problem:

jna/src/com/sun/jna/Native.java

Line 1277 in d42375c

static File getTempDir() throws IOException {

(runtime error + security vulnerability)

jna/contrib/platform/src/com/sun/jna/platform/win32/COM/tlb/TlbImp.java

Line 138 in ea9de70

private void createDir() throws FileNotFoundException {

(just a security vulnerability)

In both cases, the easy fix is to make the directory name "instance" specific with an unpredictable path:
https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#createTempDirectory(java.lang.String,%20java.nio.file.attribute.FileAttribute...)

3. Version information

Seems irrelevant... but it does affect 4.5.1

[neilcsmith-net]

Just saw this while looking at something else. Do you really get a directory at /tmp/jna/!?

[twall]

JNA uses a user-specific directory unless you set the system property jna.tmpdir (see https://github.com/java-native-access/jna/blob/master/src/com/sun/jna/Native.java#L1277 ). If you're providing JNA as a standard system resource, then you should really just install the shared library and avoid the automatic unpacking and temporary file altogether.

…

On Mon, Jul 9, 2018 at 12:29 PM, Neil C Smith ***@***.***> wrote: Just saw this while looking at something else. Do you really get a directory at /tmp/jna/!? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#985 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAuMrbhX9YBCGD7rCJYlYyRr7iySF8Zaks5uE4UEgaJpZM4VGRMT> .

[matthiasblaesing]

Ok - if someone wants to improve the situation, I'm all for it, BUT:

The problem does not exist in the described manner. At least for the last five years, JNA uses java.io.tmpdir and creates a directory there with the scheme jna-<hashCodeOfUsername>. A collision is highly unlikely. So the referenced bug is most probably not from a multi-user system, but a system, that uses the same user with different JNA versions. This should be improved.

For the security impact: Yes it is a race condition - an attacker can create a colliding directory with a library file, that has access permissions, that are wider than the umask of the calling user (if the umask of the user is that wide open, he/she has worse problems). It is a timing attack, as the file needs to be rewritten after JNA has expanded its embedded library. This should be improved.

If someone wants to take this on, this is what I suggest:

• add an alternative expansion strategy for native libraries
• the alternative could use the users home directory, create a hidden directory there with a directory per native version

That comment is relevant for Native, TlbImp is not used at normal runtime and is expected to be called with an output directory and if not present, then yes, an attacker could change your generated bindings.

[matthiasblaesing]

PR #1013 should address the problems. Please have a look at it.

[nextgens]

Hmm, it will still break when the same user runs separate JVMs with different versions of JNA... You forgot the "per native version" part :)

[neilcsmith-net]

Well, what is "per native version"? Wouldn't the ideal thing be for this all to be process segregated? In which case, I'm wondering whether a hash of the resource URL from the ClassLoader could be added to the folder or file name?

[nextgens]

I'd just pick a non-deterministic (random) value and call it a day. In our specific case on freenet, we have problems with the test-suite running the same code with different JVMs concurrently... and that seems to fail on occasion (probably because there's a race where the files get renamed/deleted and loaded by another JVM)

[matthiasblaesing]

I strongly doubt, that your problem is related to the extraction logic. From Native#extractFromResourcePath:

                File dir = getTempDir();
                lib = File.createTempFile(JNA_TMPLIB_PREFIX, Platform.isWindows()?".dll":null, dir);
                if (!Boolean.getBoolean("jnidispatch.preserve")) {
                    lib.deleteOnExit();
                }
                fos = new FileOutputStream(lib);
                int count;
                byte[] buf = new byte[1024];
                while ((count = is.read(buf, 0, buf.length)) > 0) {
                    fos.write(buf, 0, count);
                }

For your argumentation it is relevant, that File#createTempFile is used. This:

• A secure random is used to generate a random long
• That is combined with prefix and suffix (the random part is shorted to at be at least 5 chars long) to the filename
• It is then checked if the file already exists, if it exists, the filename will be regenerated
• If a non-existing name is found, the final file is created via FileSystem#createFileExclusively. If the file could not be exclusively created, an IOException is raised

I don't see how this can clash - sure SecureRandom can create the same number twice.

So you claim there is a security problem - that is fixed by my patch, as it moves the files into the users home -- if that is not secured, all is lost. You further claim there is an issue when creating the temp file, I don't see how this is possible with the construction above.

Please provide stack traces and reasoning why you think there is an issue in JNA.

[matthiasblaesing]

@nextgens I merged the changeset as it improves the situation. I would be interested in your original problem - stacktraces/detailed logs would be helpful. Feel free to take this to the google group.