comphead commented on code in PR #6987: URL: https://github.com/apache/datafusion-comet/pull/6987#discussion_r4274438758
########## spark/src/main/java/org/apache/comet/NativeLibrary.java: ########## @@ -0,0 +1,157 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.comet; + +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.channels.Channels; +import java.nio.channels.SeekableByteChannel; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.StandardOpenOption; +import java.nio.file.attribute.PosixFilePermissions; +import java.nio.file.attribute.UserPrincipal; +import java.util.EnumSet; +import java.util.Map; + +/** Owns a bundled native library until it is loaded or discarded. */ +final class NativeLibrary implements AutoCloseable { + // Use a different prefix from legacy libcomet-* files: old JVMs must not clean up this directory. + private static final String DIRECTORY_PREFIX = "comet-native-"; + private final Path directory; + private final Path library; + private boolean retained; + + private NativeLibrary(Path directory, String libraryName) { + this.directory = directory; + this.library = directory.resolve(libraryName); + } + + static NativeLibrary extract(InputStream resource, String libraryName) throws IOException { + // Spark on YARN puts java.io.tmpdir below a NodeManager-owned directory. That account can + // replace descendants, so use a separately validated system root if the preferred root is + // unsafe. + return extract( + resource, + libraryName, + Paths.get(System.getProperty("java.io.tmpdir")), + Paths.get("/tmp"), + Paths.get("/var/tmp")); + } + + static NativeLibrary extract(InputStream resource, String libraryName, Path... tempRoots) + throws IOException { + Path directory = createExtractionDirectory(tempRoots); + NativeLibrary result = new NativeLibrary(directory, libraryName); + try { + copy(resource, result.library); + return result; + } catch (IOException | RuntimeException | Error e) { + try { + result.close(); + } catch (IOException cleanupError) { + e.addSuppressed(cleanupError); + } + throw e; + } + } + + private static Path createExtractionDirectory(Path[] tempRoots) throws IOException { + IOException failure = new IOException("No safe temporary directory for libcomet"); + for (Path tempRoot : tempRoots) { + try { + Path root = tempRoot.toRealPath(); + checkTempRoot(root); + return Files.createTempDirectory( + root, + DIRECTORY_PREFIX, + PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"))); + } catch (IOException e) { + failure.addSuppressed(e); + } + } + throw failure; + } + + private static void checkTempRoot(Path root) throws IOException { + // A private child can still be renamed in a shared, non-sticky parent. Check every ancestor + // after resolving symlinks so another local user cannot replace any component of the load path. + // Fail closed on filesystems where these Unix protections cannot be established. + if (!root.getFileSystem().supportedFileAttributeViews().contains("unix")) { + throw new IOException("Cannot securely extract libcomet on a non-Unix filesystem"); + } + UserPrincipal owner = + root.getFileSystem() + .getUserPrincipalLookupService() + .lookupPrincipalByName(System.getProperty("user.name")); Review Comment: This resolves the current account by name. In a container whose UID has no passwd entry I expect `user.name` to be `?` (from memory of the JDK launcher code, I did not run it). `lookupPrincipalByName("?")` throws `UserPrincipalNotFoundException`, which I did check in a local `jshell`. Then every candidate root is rejected and bundled loading fails, while the base `File.createTempFile` path needs no account lookup. Spark's own Kubernetes `entrypoint.sh` only adds a passwd entry when `/etc/passwd` is writable, so arbitrary-UID deployments with a read-only root filesystem look exposed. Would comparing against the numeric uid work instead? For example read `unix:uid` of the private directory right after `createTempDirectory`, remove it if the ancestor walk then fails, or use `com.sun.security.auth.module.UnixSystem#getUid`. That would also stop the principal lookup and the `supportedFileAttributeViews` check from repeating for each of the three candidates. ########## spark/src/main/java/org/apache/comet/NativeLibrary.java: ########## @@ -0,0 +1,157 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.comet; + +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.channels.Channels; +import java.nio.channels.SeekableByteChannel; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.StandardOpenOption; +import java.nio.file.attribute.PosixFilePermissions; +import java.nio.file.attribute.UserPrincipal; +import java.util.EnumSet; +import java.util.Map; + +/** Owns a bundled native library until it is loaded or discarded. */ +final class NativeLibrary implements AutoCloseable { + // Use a different prefix from legacy libcomet-* files: old JVMs must not clean up this directory. + private static final String DIRECTORY_PREFIX = "comet-native-"; + private final Path directory; + private final Path library; + private boolean retained; + + private NativeLibrary(Path directory, String libraryName) { + this.directory = directory; + this.library = directory.resolve(libraryName); + } + + static NativeLibrary extract(InputStream resource, String libraryName) throws IOException { + // Spark on YARN puts java.io.tmpdir below a NodeManager-owned directory. That account can + // replace descendants, so use a separately validated system root if the preferred root is + // unsafe. + return extract( + resource, + libraryName, + Paths.get(System.getProperty("java.io.tmpdir")), + Paths.get("/tmp"), + Paths.get("/var/tmp")); + } + + static NativeLibrary extract(InputStream resource, String libraryName, Path... tempRoots) + throws IOException { + Path directory = createExtractionDirectory(tempRoots); + NativeLibrary result = new NativeLibrary(directory, libraryName); + try { + copy(resource, result.library); + return result; + } catch (IOException | RuntimeException | Error e) { + try { + result.close(); + } catch (IOException cleanupError) { + e.addSuppressed(cleanupError); + } + throw e; + } + } + + private static Path createExtractionDirectory(Path[] tempRoots) throws IOException { + IOException failure = new IOException("No safe temporary directory for libcomet"); + for (Path tempRoot : tempRoots) { + try { + Path root = tempRoot.toRealPath(); + checkTempRoot(root); + return Files.createTempDirectory( + root, + DIRECTORY_PREFIX, + PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"))); + } catch (IOException e) { + failure.addSuppressed(e); + } + } + throw failure; + } + + private static void checkTempRoot(Path root) throws IOException { Review Comment: A design question that may help with sunchao's two open threads. On YARN the ancestor this check rejects is the NodeManager-owned local dir, and per the Hadoop source (`ResourceLocalizationService`, `usercache/<user>/filecache`) the jars the library is extracted from are localized below that same directory. So an account that can replace the extracted library there can already replace the jar. I read this from the Hadoop sources and docs and did not test it on a cluster. Netty's `NativeLibraryLoader`, snappy-java and zstd-jni all create the file exclusively with owner-only permissions and offer a property to move the directory (`io.netty.native.workdir`, `org.xerial.snappy.tempdir`), and I could not find an ancestor walk in any of them. Would keeping the private `0700` directory plus `CREATE_NEW` in `java.io.tmpdir`, with an override property, cover the original finding while removing `checkTempRoot`, the `/tmp` and `/var/tmp` candidates and most of the selection tests? Separately, `cleanupOldTempLibs` in `NativeBase` only removes names starting with `libcomet-` from `java.io.tmpdir`. A `comet-native-*` directory left by a SIGKILLed executor, especially one created under `/tmp` or `/var/tmp`, looks like it is never cleaned up. I read this and did not test it. ########## spark/src/test/java/org/apache/comet/NativeLibraryTest.java: ########## @@ -0,0 +1,310 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.comet; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.FileAlreadyExistsException; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.PosixFilePermissions; +import java.util.Collections; +import java.util.concurrent.TimeUnit; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +public class NativeLibraryTest { + @Rule public TemporaryFolder temp = new TemporaryFolder(); + + private static final byte[] CONTENT = {1, 2, 3, 4}; + private static final String NAME = "libcomet.so"; + + private ByteArrayInputStream resource() { + return new ByteArrayInputStream(CONTENT); + } + + @Test + public void extractionIsPrivateAndReadOnly() throws Exception { + Path library; + Path directory; + try (NativeLibrary extracted = NativeLibrary.extract(resource(), NAME)) { + library = extracted.path(); + directory = library.getParent(); + assertArrayEquals(CONTENT, Files.readAllBytes(library)); + assertEquals( + PosixFilePermissions.fromString("rwx------"), Files.getPosixFilePermissions(directory)); + assertEquals( + PosixFilePermissions.fromString("r--------"), Files.getPosixFilePermissions(library)); + assertEquals(Files.getOwner(directory), Files.getOwner(library)); + assertFalse(Files.isSymbolicLink(directory)); + assertFalse(Files.isSymbolicLink(library)); + assertTrue(directory.getFileName().toString().startsWith("comet-native-")); + } + assertFalse(Files.exists(library)); + assertFalse(Files.exists(directory)); + } + + @Test + public void extractionsDoNotSharePaths() throws Exception { Review Comment: A few of the 17 tests look redundant. `extractionsDoNotSharePaths` asserts what `Files.createTempDirectory` already guarantees. `copyFailureDoesNotRetryAnotherRoot` also asserts that both roots end up empty, which is what `copyFailureRemovesPartialLibraryAndDirectory` checks for one root, so the two can be one test. `refusesExistingLibrary` and `refusesSymlinkWithoutOverwritingTarget` both pin `CREATE_NEW`, and the symlink case is the one that matters. `defaultExtractionFallsBackFromUnsuitableSparkTempRoot` also changes the JVM-wide `java.io.tmpdir` and extracts into the real `/tmp`, so please check that is acceptable in the shared surefire JVM. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
