Skip to content
Permalink
Browse files Browse the repository at this point in the history
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 <Jonathan.Leitschuh@gmail.com>
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>

Bug-tracker: JLLeitschuh/security-research#10

Co-authored-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Co-authored-by: Moderne <team@moderne.io>
  • Loading branch information
3 people committed Sep 23, 2022
1 parent 9fd0ecf commit 4a4024a
Show file tree
Hide file tree
Showing 13 changed files with 41 additions and 33 deletions.
Expand Up @@ -56,7 +56,7 @@
/**
* directory where files will go
*/
private final File workDir = IOUtil.createTempDir("CSPI.", null);
private final File workDir = IOUtil.createTempDir("CSPI.tmp").toFile();
private int sequenceIndexOfMapInRam = INVALID_SEQUENCE_INDEX;
private Map<KEY, REC> mapInRam = null;
private final FileAppendStreamLRUCache outputStreams;
Expand Down
35 changes: 22 additions & 13 deletions src/main/java/htsjdk/samtools/util/IOUtil.java 100755 → 100644
Expand Up @@ -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);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/htsjdk/samtools/BAMMergerTest.java
Expand Up @@ -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();
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/htsjdk/samtools/CRAMFileWriterTest.java
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/htsjdk/samtools/CRAMMergerTest.java
Expand Up @@ -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();
Expand Down
Expand Up @@ -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());
Expand Down
Expand Up @@ -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");
Expand Down
11 changes: 5 additions & 6 deletions src/test/java/htsjdk/samtools/util/IOUtilTest.java
Expand Up @@ -312,7 +312,7 @@ public void testGetDefaultTmpDirPath() throws Exception {

@Test(dataProvider = "fileNamesForDelete")
public void testDeletePathLocal(final List<String> fileNames) throws Exception {
final File tmpDir = IOUtil.createTempDir("testDeletePath", "");
final Path tmpDir = IOUtil.createTempDir("testDeletePath");
final List<Path> paths = createLocalFiles(tmpDir, fileNames);
testDeletePaths(paths);
}
Expand Down Expand Up @@ -341,7 +341,7 @@ public void testDeletePathJims(final List<String> fileNames) throws Exception {

@Test(dataProvider = "fileNamesForDelete")
public void testDeleteArrayPathLocal(final List<String> fileNames) throws Exception {
final File tmpDir = IOUtil.createTempDir("testDeletePath", "");
final Path tmpDir = IOUtil.createTempDir("testDeletePath");
final List<Path> paths = createLocalFiles(tmpDir, fileNames);
testDeletePathArray(paths);
}
Expand All @@ -365,12 +365,11 @@ private static void testDeletePathArray(final List<Path> paths) {
paths.forEach(p -> Assert.assertFalse(Files.exists(p)));
}

private static List<Path> createLocalFiles(final File tmpDir, final List<String> fileNames) throws Exception {
private static List<Path> createLocalFiles(final Path tmpDir, final List<String> fileNames) throws Exception {
final List<Path> 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;
}
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/htsjdk/tribble/index/IndexFactoryTest.java
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/htsjdk/tribble/index/IndexTest.java
Expand Up @@ -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
Expand Down
Expand Up @@ -190,7 +190,7 @@ public void testBedTabixIndex(
final List<Interval> 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);
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/htsjdk/variant/vcf/VCFFileReaderTest.java
Expand Up @@ -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");
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/htsjdk/variant/vcf/VCFMergerTest.java
Expand Up @@ -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();
Expand Down

0 comments on commit 4a4024a

Please sign in to comment.