From 4a4024a97ee3e87096df6ad9b22c8260bd527772 Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Fri, 23 Sep 2022 16:32:37 -0400 Subject: [PATCH] Fix temporary directory hijacking or temporary directory information disclosure (#1621) This replaces and closes #1617. Fix a vulnerability which allows a malicious user access files created in a temp directory if it was created using IOUtils.createTempDir. The implementation has been changed to use the more secure Files.createTemporaryDirectory(). This has a side effect of making the second "suffix" argument no longer usable and it is now simply appended to the existing prefix. Deprecating IOUtils.createTempDir(String, String) and adding a new single argument version instead which returns a Path. This potentially exposed data created by CoordinateSortedPairInfoMaps on shared systems to a malicious users with access to the global temp directory. It is recommended that user upgrade if they use CoordinateSortedPairInfoMap or IOUTils.createTempDir() in their code. * vuln-fix: Temporary Directory Hijacking or Information Disclosure This fixes either Temporary Directory Hijacking, or Temporary Directory Local Information Disclosure. Weakness: CWE-379: Creation of Temporary File in Directory with Insecure Permissions Severity: High CVSSS: 7.3 Detection: CodeQL & OpenRewrite (https://public.moderne.io/recipes/org.openrewrite.java.security.UseFilesCreateTempDirectory) Reported-by: Jonathan Leitschuh Signed-off-by: Jonathan Leitschuh Bug-tracker: https://github.com/JLLeitschuh/security-research/issues/10 Co-authored-by: Jonathan Leitschuh Co-authored-by: Moderne --- .../samtools/CoordinateSortedPairInfoMap.java | 2 +- .../java/htsjdk/samtools/util/IOUtil.java | 35 ++++++++++++------- .../java/htsjdk/samtools/BAMMergerTest.java | 2 +- .../htsjdk/samtools/CRAMFileWriterTest.java | 4 +-- .../java/htsjdk/samtools/CRAMMergerTest.java | 2 +- .../FastaSequenceIndexCreatorTest.java | 2 +- .../SeekableStreamFactoryTest.java | 2 +- .../java/htsjdk/samtools/util/IOUtilTest.java | 11 +++--- .../tribble/index/IndexFactoryTest.java | 6 ++-- .../java/htsjdk/tribble/index/IndexTest.java | 2 +- .../tribble/index/tabix/TabixIndexTest.java | 2 +- .../htsjdk/variant/vcf/VCFFileReaderTest.java | 2 +- .../htsjdk/variant/vcf/VCFMergerTest.java | 2 +- 13 files changed, 41 insertions(+), 33 deletions(-) mode change 100755 => 100644 src/main/java/htsjdk/samtools/util/IOUtil.java diff --git a/src/main/java/htsjdk/samtools/CoordinateSortedPairInfoMap.java b/src/main/java/htsjdk/samtools/CoordinateSortedPairInfoMap.java index 37c200cc56..f28b003e69 100644 --- a/src/main/java/htsjdk/samtools/CoordinateSortedPairInfoMap.java +++ b/src/main/java/htsjdk/samtools/CoordinateSortedPairInfoMap.java @@ -56,7 +56,7 @@ public class CoordinateSortedPairInfoMap implements Iterable mapInRam = null; private final FileAppendStreamLRUCache outputStreams; diff --git a/src/main/java/htsjdk/samtools/util/IOUtil.java b/src/main/java/htsjdk/samtools/util/IOUtil.java old mode 100755 new mode 100644 index 81351e297a..d473bebbe0 --- a/src/main/java/htsjdk/samtools/util/IOUtil.java +++ b/src/main/java/htsjdk/samtools/util/IOUtil.java @@ -974,25 +974,34 @@ public static void copyDirectoryTree(final File fileOrDirectory, final File dest } /** - * Create a temporary subdirectory in the default temporary-file directory, using the given prefix and suffix to generate the name. + * Create a temporary subdirectory in the default temporary-file directory, using the given prefix and morePrefix to generate the name. * Note that this method is not completely safe, because it create a temporary file, deletes it, and then creates * a directory with the same name as the file. Should be good enough. * - * @param prefix The prefix string to be used in generating the file's name; must be at least three characters long - * @param suffix The suffix string to be used in generating the file's name; may be null, in which case the suffix ".tmp" will be used + * This has been updated to avoid the problem in https://github.com/samtools/htsjdk/pull/1617 + * + * @param prefix The prefix string to be used in generating the file's name; + * @param morePrefix This was previously a suffix but the implementation changed; may be null, in which case the morePrefix ".tmp" will be used * @return File object for new directory + * @deprecated Use {@link #createTempDir(String)} instead. + * It turns out the mechanism was not "good enough" and caused security issues, the new implementation + * combines the prefix/morePrefix into a single prefix. The security flaw is fixed but due to the now + * extraneous morePrefix argument it is recommended to use the 1 argument form. + */ + @Deprecated + public static File createTempDir(final String prefix, final String morePrefix) { + final String dotSeparatedSuffix = morePrefix == null ? ".tmp" : morePrefix.startsWith(".") ? morePrefix : "." + morePrefix; + return createTempDir(prefix + dotSeparatedSuffix).toFile() ; + } + + /* + * Create a temporary subdirectory in the default temporary-file directory, using the given prefix and suffix to generate the name. + * @param prefix The prefix string to be used in generating the file's name, may be null */ - public static File createTempDir(final String prefix, final String suffix) { + public static Path createTempDir(final String prefix) { try { - final File tmp = File.createTempFile(prefix, suffix); - if (!tmp.delete()) { - throw new SAMException("Could not delete temporary file " + tmp); - } - if (!tmp.mkdir()) { - throw new SAMException("Could not create temporary directory " + tmp); - } - return tmp; - } catch (IOException e) { + return Files.createTempDirectory(prefix); + } catch (final IOException e) { throw new SAMException("Exception creating temporary directory.", e); } } diff --git a/src/test/java/htsjdk/samtools/BAMMergerTest.java b/src/test/java/htsjdk/samtools/BAMMergerTest.java index f8dd406499..a86de2de09 100644 --- a/src/test/java/htsjdk/samtools/BAMMergerTest.java +++ b/src/test/java/htsjdk/samtools/BAMMergerTest.java @@ -222,7 +222,7 @@ private static Path textIndexBai(Path bai) { @Test public void test() throws IOException { - final Path outputDir = IOUtil.createTempDir(this.getClass().getSimpleName() + ".", ".tmp").toPath(); + final Path outputDir = IOUtil.createTempDir(this.getClass().getSimpleName() + ".tmp"); IOUtil.deleteOnExit(outputDir); final Path outputBam = File.createTempFile(this.getClass().getSimpleName() + ".", ".bam").toPath(); diff --git a/src/test/java/htsjdk/samtools/CRAMFileWriterTest.java b/src/test/java/htsjdk/samtools/CRAMFileWriterTest.java index c61f9acd49..87482ee2a5 100644 --- a/src/test/java/htsjdk/samtools/CRAMFileWriterTest.java +++ b/src/test/java/htsjdk/samtools/CRAMFileWriterTest.java @@ -271,11 +271,11 @@ public void test_roundtrip_tlen_preserved() throws IOException { public void test_roundtrip_many_reads() throws IOException { // Create the input file - final File outputDir = IOUtil.createTempDir(this.getClass().getSimpleName() + ".", ".tmp"); + final File outputDir = IOUtil.createTempDir(this.getClass().getSimpleName() + ".tmp").toFile(); outputDir.deleteOnExit(); final Path output = new File(outputDir, "input.cram").toPath(); IOUtil.deleteOnExit(output); - final Path fastaDir = IOUtil.createTempDir("CRAMFileWriterTest", "").toPath(); + final Path fastaDir = IOUtil.createTempDir("CRAMFileWriterTest"); IOUtil.deleteOnExit(fastaDir); final Path newFasta = fastaDir.resolve("input.fasta"); IOUtil.deleteOnExit(newFasta); diff --git a/src/test/java/htsjdk/samtools/CRAMMergerTest.java b/src/test/java/htsjdk/samtools/CRAMMergerTest.java index 34f793cc84..3272a776ee 100644 --- a/src/test/java/htsjdk/samtools/CRAMMergerTest.java +++ b/src/test/java/htsjdk/samtools/CRAMMergerTest.java @@ -193,7 +193,7 @@ private static Path indexCram(Path cram, Path crai) throws IOException { @Test public void test() throws IOException { - final Path outputDir = IOUtil.createTempDir(this.getClass().getSimpleName() + ".", ".tmp").toPath(); + final Path outputDir = IOUtil.createTempDir(this.getClass().getSimpleName() + ".tmp"); IOUtil.deleteOnExit(outputDir); final Path outputCram = File.createTempFile(this.getClass().getSimpleName() + ".", FileExtensions.CRAM).toPath(); diff --git a/src/test/java/htsjdk/samtools/reference/FastaSequenceIndexCreatorTest.java b/src/test/java/htsjdk/samtools/reference/FastaSequenceIndexCreatorTest.java index 193770abd1..41c3d7bc43 100644 --- a/src/test/java/htsjdk/samtools/reference/FastaSequenceIndexCreatorTest.java +++ b/src/test/java/htsjdk/samtools/reference/FastaSequenceIndexCreatorTest.java @@ -63,7 +63,7 @@ public void testBuildFromFasta(final File indexedFile) throws Exception { @Test(dataProvider = "indexedSequences") public void testCreate(final File indexedFile) throws Exception { // copy the file to index - final File tempDir = IOUtil.createTempDir("FastaSequenceIndexCreatorTest", "testCreate"); + final File tempDir = IOUtil.createTempDir("FastaSequenceIndexCreatorTest.testCreate").toFile(); final File copied = new File(tempDir, indexedFile.getName()); copied.deleteOnExit(); Files.copy(indexedFile.toPath(), copied.toPath()); diff --git a/src/test/java/htsjdk/samtools/seekablestream/SeekableStreamFactoryTest.java b/src/test/java/htsjdk/samtools/seekablestream/SeekableStreamFactoryTest.java index 26e7868d6c..c5e6126ee9 100644 --- a/src/test/java/htsjdk/samtools/seekablestream/SeekableStreamFactoryTest.java +++ b/src/test/java/htsjdk/samtools/seekablestream/SeekableStreamFactoryTest.java @@ -52,7 +52,7 @@ public void testPathWithEmbeddedSpace() throws IOException { final File testBam = new File(TEST_DATA_DIR, "BAMFileIndexTest/index_test.bam"); //create a temp dir with a space in the name and copy the test file there - final File tempDir = IOUtil.createTempDir("test spaces", ""); + final File tempDir = IOUtil.createTempDir("test spaces").toFile(); Assert.assertTrue(tempDir.getAbsolutePath().contains(" ")); tempDir.deleteOnExit(); final File inputBam = new File(tempDir, "index_test.bam"); diff --git a/src/test/java/htsjdk/samtools/util/IOUtilTest.java b/src/test/java/htsjdk/samtools/util/IOUtilTest.java index 9b3b60ff34..8354b5ba29 100644 --- a/src/test/java/htsjdk/samtools/util/IOUtilTest.java +++ b/src/test/java/htsjdk/samtools/util/IOUtilTest.java @@ -312,7 +312,7 @@ public void testGetDefaultTmpDirPath() throws Exception { @Test(dataProvider = "fileNamesForDelete") public void testDeletePathLocal(final List fileNames) throws Exception { - final File tmpDir = IOUtil.createTempDir("testDeletePath", ""); + final Path tmpDir = IOUtil.createTempDir("testDeletePath"); final List paths = createLocalFiles(tmpDir, fileNames); testDeletePaths(paths); } @@ -341,7 +341,7 @@ public void testDeletePathJims(final List fileNames) throws Exception { @Test(dataProvider = "fileNamesForDelete") public void testDeleteArrayPathLocal(final List fileNames) throws Exception { - final File tmpDir = IOUtil.createTempDir("testDeletePath", ""); + final Path tmpDir = IOUtil.createTempDir("testDeletePath"); final List paths = createLocalFiles(tmpDir, fileNames); testDeletePathArray(paths); } @@ -365,12 +365,11 @@ private static void testDeletePathArray(final List paths) { paths.forEach(p -> Assert.assertFalse(Files.exists(p))); } - private static List createLocalFiles(final File tmpDir, final List fileNames) throws Exception { + private static List createLocalFiles(final Path tmpDir, final List fileNames) throws Exception { final List paths = new ArrayList<>(fileNames.size()); for (final String f: fileNames) { - final File file = new File(tmpDir, f); - Assert.assertTrue(file.createNewFile(), "failed to create test file" +file); - paths.add(file.toPath()); + final Path file = Files.createFile(tmpDir.resolve(f)); + paths.add(file); } return paths; } diff --git a/src/test/java/htsjdk/tribble/index/IndexFactoryTest.java b/src/test/java/htsjdk/tribble/index/IndexFactoryTest.java index e127fd4b2f..3897cbc9f0 100644 --- a/src/test/java/htsjdk/tribble/index/IndexFactoryTest.java +++ b/src/test/java/htsjdk/tribble/index/IndexFactoryTest.java @@ -166,7 +166,7 @@ public void testCreateTabixIndexFromVCF( final File inputVCF, final Interval queryInterval) throws IOException { // copy the original file and create the index for the copy - final File tempDir = IOUtil.createTempDir("testCreateTabixIndexFromVCF", null); + final File tempDir = IOUtil.createTempDir("testCreateTabixIndexFromVCF").toFile(); tempDir.deleteOnExit(); final File tmpVCF = new File(tempDir, inputVCF.getName()); Files.copy(inputVCF, tmpVCF); @@ -208,7 +208,7 @@ public Object[][] getBCFData(){ @Test(dataProvider = "bcfDataFactory") public void testCreateLinearIndexFromBCF(final File inputBCF) throws IOException { // copy the original file and create the index for the copy - final File tempDir = IOUtil.createTempDir("testCreateIndexFromBCF", null); + final File tempDir = IOUtil.createTempDir("testCreateIndexFromBCF").toFile(); tempDir.deleteOnExit(); final File tmpBCF = new File(tempDir, inputBCF.getName()); Files.copy(inputBCF, tmpBCF); @@ -250,7 +250,7 @@ public Object[][] getRedirectFiles(){ @Test(dataProvider = "getRedirectFiles") public void testIndexRedirectedFiles(String input, IndexFactory.IndexType type) throws IOException { final VCFRedirectCodec codec = new VCFRedirectCodec(); - final File dir = IOUtil.createTempDir("redirec-test", "dir"); + final File dir = IOUtil.createTempDir("redirec-test.dir").toFile(); try { final File tmpInput = new File(dir, new File(input).getName()); Files.copy(new File(input), tmpInput); diff --git a/src/test/java/htsjdk/tribble/index/IndexTest.java b/src/test/java/htsjdk/tribble/index/IndexTest.java index 55a92a11f7..516e788300 100644 --- a/src/test/java/htsjdk/tribble/index/IndexTest.java +++ b/src/test/java/htsjdk/tribble/index/IndexTest.java @@ -131,7 +131,7 @@ public void testWritePathIndex(final File inputFile, final IndexFactory.IndexTyp @Test(dataProvider = "writeIndexData") public void testWriteBasedOnNonRegularFeatureFile(final File inputFile, final IndexFactory.IndexType type, final FeatureCodec codec) throws Exception { - final File tmpFolder = IOUtil.createTempDir("NonRegultarFeatureFile", null); + final File tmpFolder = IOUtil.createTempDir("NonRegultarFeatureFile").toFile(); // create the index final Index index = IndexFactory.createIndex(inputFile, codec, type); // try to write based on the tmpFolder diff --git a/src/test/java/htsjdk/tribble/index/tabix/TabixIndexTest.java b/src/test/java/htsjdk/tribble/index/tabix/TabixIndexTest.java index 87a16f6f63..3012952b9f 100644 --- a/src/test/java/htsjdk/tribble/index/tabix/TabixIndexTest.java +++ b/src/test/java/htsjdk/tribble/index/tabix/TabixIndexTest.java @@ -190,7 +190,7 @@ public void testBedTabixIndex( final List queryIntervals ) throws Exception { // copy the input file and create an index for the copy - final File tempDir = IOUtil.createTempDir("testBedTabixIndex", null); + final File tempDir = IOUtil.createTempDir("testBedTabixIndex.tmp").toFile(); tempDir.deleteOnExit(); final File tmpBed = new File(tempDir, inputBed.getName()); Files.copy(inputBed, tmpBed); diff --git a/src/test/java/htsjdk/variant/vcf/VCFFileReaderTest.java b/src/test/java/htsjdk/variant/vcf/VCFFileReaderTest.java index 383d272a8d..d848209d99 100644 --- a/src/test/java/htsjdk/variant/vcf/VCFFileReaderTest.java +++ b/src/test/java/htsjdk/variant/vcf/VCFFileReaderTest.java @@ -110,7 +110,7 @@ public void testTabixFileWithEmbeddedSpaces() throws IOException { // Copy the input files into a temporary directory with embedded spaces in the name. // This test needs to include the associated .tbi file because we want to force execution // of the tabix code path. - final File tempDir = IOUtil.createTempDir("test spaces", ""); + final File tempDir = IOUtil.createTempDir("test spaces").toFile(); Assert.assertTrue(tempDir.getAbsolutePath().contains(" ")); tempDir.deleteOnExit(); final File inputVCF = new File(tempDir, "HiSeq.10000.vcf.bgz"); diff --git a/src/test/java/htsjdk/variant/vcf/VCFMergerTest.java b/src/test/java/htsjdk/variant/vcf/VCFMergerTest.java index 4067e7c868..8c769c85e5 100644 --- a/src/test/java/htsjdk/variant/vcf/VCFMergerTest.java +++ b/src/test/java/htsjdk/variant/vcf/VCFMergerTest.java @@ -206,7 +206,7 @@ private static Path indexVcf(Path vcf, Path tbi) throws IOException { @Test public void test() throws IOException { - final Path outputDir = IOUtil.createTempDir(this.getClass().getSimpleName() + ".", ".tmp").toPath(); + final Path outputDir = IOUtil.createTempDir(this.getClass().getSimpleName() + ".tmp"); IOUtil.deleteOnExit(outputDir); final Path outputVcf = File.createTempFile(this.getClass().getSimpleName() + ".", FileExtensions.COMPRESSED_VCF).toPath();