Advisory fix 1 (#1089)

* Update cache and comparison downloads to use https

* Zip Slip tests and fix

* Zip Slip tests 2 and fix

* Add missing tempDir child in ScannerTest

* Add win format zip test

* Add tests to r4b

* Add tests and fixes for slips in tgz processing

* Update fhir-test-cases version
This commit is contained in:
dotasek 2023-01-20 16:56:57 -05:00 committed by GitHub
parent b5da39ffe8
commit b50aec5912
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
26 changed files with 376 additions and 42 deletions

View File

@ -1,9 +1,6 @@
package org.hl7.fhir.convertors.misc;
import java.io.BufferedOutputStream;
import java.io.ByteArrayOutputStream;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.*;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.HashMap;
@ -39,6 +36,7 @@ import org.hl7.fhir.utilities.json.parser.JsonParser;
import org.hl7.fhir.utilities.npm.NpmPackageIndexBuilder;
import com.google.common.base.Charsets;
import org.jetbrains.annotations.NotNull;
public class NpmPackageVersionConverter {
@ -76,33 +74,7 @@ public class NpmPackageVersionConverter {
}
public void execute() throws IOException {
GzipCompressorInputStream gzipIn;
try {
gzipIn = new GzipCompressorInputStream(new FileInputStream(source));
} catch (Exception e) {
throw new IOException("Error reading " + source + ": " + e.getMessage(), e);
}
Map<String, byte[]> content = new HashMap<>();
try (TarArchiveInputStream tarIn = new TarArchiveInputStream(gzipIn)) {
TarArchiveEntry entry;
while ((entry = (TarArchiveEntry) tarIn.getNextEntry()) != null) {
String n = entry.getName();
if (!entry.isDirectory()) {
int count;
byte[] data = new byte[BUFFER_SIZE];
ByteArrayOutputStream fos = new ByteArrayOutputStream();
try (BufferedOutputStream dest = new BufferedOutputStream(fos, BUFFER_SIZE)) {
while ((count = tarIn.read(data, 0, BUFFER_SIZE)) != -1) {
dest.write(data, 0, count);
}
}
fos.close();
content.put(n, fos.toByteArray());
}
}
}
Map<String, byte[]> content = loadContentMap(new FileInputStream(source));
Map<String, byte[]> output = new HashMap<>();
output.put("package/package.json", convertPackage(content.get("package/package.json")));
@ -180,6 +152,41 @@ public class NpmPackageVersionConverter {
TextFile.bytesToFile(b, dest);
}
@NotNull
protected Map<String, byte[]> loadContentMap(InputStream inputStream) throws IOException {
GzipCompressorInputStream gzipIn;
try {
gzipIn = new GzipCompressorInputStream(inputStream);
} catch (Exception e) {
throw new IOException("Error reading " + source + ": " + e.getMessage(), e);
}
Map<String, byte[]> content = new HashMap<>();
try (TarArchiveInputStream tarIn = new TarArchiveInputStream(gzipIn)) {
TarArchiveEntry entry;
while ((entry = (TarArchiveEntry) tarIn.getNextEntry()) != null) {
String n = entry.getName();
if (n.contains("..")) {
throw new RuntimeException("Entry with an illegal name: " + n);
}
if (!entry.isDirectory()) {
int count;
byte[] data = new byte[BUFFER_SIZE];
ByteArrayOutputStream fos = new ByteArrayOutputStream();
try (BufferedOutputStream dest = new BufferedOutputStream(fos, BUFFER_SIZE)) {
while ((count = tarIn.read(data, 0, BUFFER_SIZE)) != -1) {
dest.write(data, 0, count);
}
}
fos.close();
content.put(n, fos.toByteArray());
}
}
}
return content;
}
private byte[] convertPackage(byte[] cnt) throws IOException {
JsonObject json = JsonParser.parseObject(cnt);
currentVersion = json.getJsonArray("fhirVersions").get(0).asString();

View File

@ -0,0 +1,37 @@
package org.hl7.fhir.convertors.misc;
import org.hl7.fhir.utilities.npm.NpmPackage;
import org.hl7.fhir.utilities.tests.ResourceLoaderTests;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.Map;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
public class NpmPackageVersionConverterTests implements ResourceLoaderTests {
@org.junit.jupiter.api.Test
public void testNormalTgz() throws IOException {
InputStream tgzStream = getResourceAsInputStream("misc", "npmPackageVersionConverter", "tgz-normal.tgz");
NpmPackageVersionConverter converter = new NpmPackageVersionConverter(null, null, "r5", null);
Map<String, byte[]> contents = converter.loadContentMap(tgzStream);
String actual = new String(contents.get("depth1/test.txt"), StandardCharsets.UTF_8);
assertEquals("dummy file content", actual);
}
@Test
public void testEvilTgz() throws IOException {
RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, () -> {
InputStream tgzStream = getResourceAsInputStream("misc", "npmPackageVersionConverter", "tgz-evil.tgz");
NpmPackageVersionConverter converter = new NpmPackageVersionConverter(null, null, "r5", null);
converter.loadContentMap(tgzStream);
});
assertNotNull(thrown);
assertEquals("Entry with an illegal name: ../evil.txt", thrown.getMessage());
}
}

View File

@ -65,11 +65,11 @@ public class TerminologyCacheManager {
}
if (!version.equals(getCacheVersion())) {
clearCache();
fillCache("http://tx.fhir.org/tx-cache/"+ghOrg+"/"+ghRepo+"/"+ghBranch+".zip");
fillCache("https://tx.fhir.org/tx-cache/"+ghOrg+"/"+ghRepo+"/"+ghBranch+".zip");
}
if (!version.equals(getCacheVersion())) {
clearCache();
fillCache("http://tx.fhir.org/tx-cache/"+ghOrg+"/"+ghRepo+"/default.zip");
fillCache("https://tx.fhir.org/tx-cache/"+ghOrg+"/"+ghRepo+"/default.zip");
}
if (!version.equals(getCacheVersion())) {
clearCache();
@ -97,7 +97,8 @@ public class TerminologyCacheManager {
public static void unzip(InputStream is, String targetDir) throws IOException {
try (ZipInputStream zipIn = new ZipInputStream(is)) {
for (ZipEntry ze; (ze = zipIn.getNextEntry()) != null; ) {
String path = Utilities.path(targetDir, ze.getName());
String path = Path.of(Utilities.path(targetDir, ze.getName())).normalize().toFile().getAbsolutePath();
if (!path.startsWith(targetDir)) {
// see: https://snyk.io/research/zip-slip-vulnerability
throw new RuntimeException("Entry with an illegal path: " + ze.getName());

View File

@ -0,0 +1,77 @@
package org.hl7.fhir.r4b.terminologies;
import org.hl7.fhir.utilities.tests.ResourceLoaderTests;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
public class TerminologyCacheManagerTests implements ResourceLoaderTests {
public static final String ZIP_NORMAL_ZIP = "zip-normal.zip";
public static final String ZIP_SLIP_ZIP = "zip-slip.zip";
public static final String ZIP_SLIP_2_ZIP = "zip-slip-2.zip";
public static final String ZIP_SLIP_WIN_ZIP = "zip-slip-win.zip";
Path tempDir;
@BeforeAll
public void beforeAll() throws IOException {
tempDir = Files.createTempDirectory("terminology-cache-manager");
tempDir.resolve("child").toFile().mkdir();
getResourceAsInputStream("terminologyCacheManager", ZIP_SLIP_ZIP);
}
@Test
public void testNormalZip() throws IOException {
InputStream normalInputStream = getResourceAsInputStream( "terminologyCacheManager", ZIP_NORMAL_ZIP);
TerminologyCacheManager.unzip( normalInputStream, tempDir.toFile().getAbsolutePath());
Path expectedFilePath = tempDir.resolve("zip-normal").resolve("depth1").resolve("test.txt");
String actualContent = Files.readString(expectedFilePath);
assertEquals("dummy file content", actualContent);
}
@Test
public void testSlipZip() throws IOException {
RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, () -> {
InputStream slipInputStream = getResourceAsInputStream( "terminologyCacheManager", ZIP_SLIP_ZIP);
TerminologyCacheManager.unzip( slipInputStream, tempDir.toFile().getAbsolutePath());
//Code under test
});
assertNotNull(thrown);
assertEquals("Entry with an illegal path: ../evil.txt", thrown.getMessage());
}
@Test
public void testSlip2Zip() throws IOException {
RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, () -> {
InputStream slipInputStream = getResourceAsInputStream( "terminologyCacheManager", ZIP_SLIP_2_ZIP);
TerminologyCacheManager.unzip( slipInputStream, tempDir.toFile().getAbsolutePath());
//Code under test
});
assertNotNull(thrown);
assertEquals("Entry with an illegal path: child/../../evil.txt", thrown.getMessage());
}
@Test
public void testSlipZipWin() throws IOException {
RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, () -> {
InputStream slipInputStream = getResourceAsInputStream( "terminologyCacheManager", ZIP_SLIP_WIN_ZIP);
TerminologyCacheManager.unzip( slipInputStream, tempDir.toFile().getAbsolutePath());
//Code under test
});
assertNotNull(thrown);
assertEquals("Entry with an illegal path: ../evil.txt", thrown.getMessage());
}
}

View File

@ -65,11 +65,11 @@ public class TerminologyCacheManager {
}
if (!version.equals(getCacheVersion())) {
clearCache();
fillCache("http://tx.fhir.org/tx-cache/"+ghOrg+"/"+ghRepo+"/"+ghBranch+".zip");
fillCache("https://tx.fhir.org/tx-cache/"+ghOrg+"/"+ghRepo+"/"+ghBranch+".zip");
}
if (!version.equals(getCacheVersion())) {
clearCache();
fillCache("http://tx.fhir.org/tx-cache/"+ghOrg+"/"+ghRepo+"/default.zip");
fillCache("https://tx.fhir.org/tx-cache/"+ghOrg+"/"+ghRepo+"/default.zip");
}
if (!version.equals(getCacheVersion())) {
clearCache();
@ -97,7 +97,7 @@ public class TerminologyCacheManager {
public static void unzip(InputStream is, String targetDir) throws IOException {
try (ZipInputStream zipIn = new ZipInputStream(is)) {
for (ZipEntry ze; (ze = zipIn.getNextEntry()) != null; ) {
String path = Utilities.path(targetDir, ze.getName());
String path = Path.of(Utilities.path(targetDir, ze.getName())).normalize().toFile().getAbsolutePath();
if (!path.startsWith(targetDir)) {
// see: https://snyk.io/research/zip-slip-vulnerability
throw new RuntimeException("Entry with an illegal path: " + ze.getName());

View File

@ -0,0 +1,78 @@
package org.hl7.fhir.r5.terminologies;
import org.hl7.fhir.utilities.tests.ResourceLoaderTests;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
public class TerminologyCacheManagerTests implements ResourceLoaderTests {
public static final String ZIP_NORMAL_ZIP = "zip-normal.zip";
public static final String ZIP_SLIP_ZIP = "zip-slip.zip";
public static final String ZIP_SLIP_2_ZIP = "zip-slip-2.zip";
public static final String ZIP_SLIP_WIN_ZIP = "zip-slip-win.zip";
Path tempDir;
@BeforeAll
public void beforeAll() throws IOException {
tempDir = Files.createTempDirectory("terminology-cache-manager");
tempDir.resolve("child").toFile().mkdir();
getResourceAsInputStream("terminologyCacheManager", ZIP_SLIP_ZIP);
}
@Test
public void testNormalZip() throws IOException {
InputStream normalInputStream = getResourceAsInputStream( "terminologyCacheManager", ZIP_NORMAL_ZIP);
TerminologyCacheManager.unzip( normalInputStream, tempDir.toFile().getAbsolutePath());
Path expectedFilePath = tempDir.resolve("zip-normal").resolve("depth1").resolve("test.txt");
String actualContent = Files.readString(expectedFilePath);
assertEquals("dummy file content", actualContent);
}
@Test
public void testSlipZip() throws IOException {
RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, () -> {
InputStream slipInputStream = getResourceAsInputStream( "terminologyCacheManager", ZIP_SLIP_ZIP);
TerminologyCacheManager.unzip( slipInputStream, tempDir.toFile().getAbsolutePath());
//Code under test
});
assertNotNull(thrown);
assertEquals("Entry with an illegal path: ../evil.txt", thrown.getMessage());
}
@Test
public void testSlip2Zip() throws IOException {
RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, () -> {
InputStream slipInputStream = getResourceAsInputStream( "terminologyCacheManager", ZIP_SLIP_2_ZIP);
TerminologyCacheManager.unzip( slipInputStream, tempDir.toFile().getAbsolutePath());
//Code under test
});
assertNotNull(thrown);
assertEquals("Entry with an illegal path: child/../../evil.txt", thrown.getMessage());
}
@Test
public void testSlipZipWin() throws IOException {
RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, () -> {
InputStream slipInputStream = getResourceAsInputStream( "terminologyCacheManager", ZIP_SLIP_WIN_ZIP);
TerminologyCacheManager.unzip( slipInputStream, tempDir.toFile().getAbsolutePath());
//Code under test
});
assertNotNull(thrown);
assertEquals("Entry with an illegal path: ../evil.txt", thrown.getMessage());
}
}

View File

@ -72,6 +72,7 @@ import org.hl7.fhir.utilities.json.model.JsonObject;
import org.hl7.fhir.utilities.json.model.JsonProperty;
import org.hl7.fhir.utilities.json.parser.JsonParser;
import org.hl7.fhir.utilities.npm.PackageGenerator.PackageType;
import org.jetbrains.annotations.NotNull;
/**
* info and loader for a package
@ -377,7 +378,7 @@ public class NpmPackage {
private static final int BUFFER_SIZE = 1024;
public static NpmPackage fromPackage(InputStream tgz) throws IOException {
public static @NotNull NpmPackage fromPackage(InputStream tgz) throws IOException {
return fromPackage(tgz, null, false);
}
@ -406,6 +407,9 @@ public class NpmPackage {
while ((entry = (TarArchiveEntry) tarIn.getNextEntry()) != null) {
i++;
String n = entry.getName();
if (n.contains("..")) {
throw new RuntimeException("Entry with an illegal name: " + n);
}
if (entry.isDirectory()) {
String dir = n.substring(0, n.length()-1);
if (dir.startsWith("package/")) {

View File

@ -0,0 +1,35 @@
package org.hl7.fhir.utilities.npm;
import org.apache.commons.io.IOUtils;
import org.hl7.fhir.utilities.tests.ResourceLoaderTests;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
public class NpmPackageTests implements ResourceLoaderTests {
@Test
public void testNormalTgz() throws IOException {
InputStream tgzStream = getResourceAsInputStream("npm", "tar", "tgz-normal.tgz");
NpmPackage npmPackage = NpmPackage.fromPackage(tgzStream);
InputStream inputStream = npmPackage.load("depth1", "test.txt");
String actual = IOUtils.toString(inputStream, StandardCharsets.UTF_8.name());
assertEquals("dummy file content", actual);
}
@Test
public void testEvilTgz() throws IOException {
RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, () -> {
InputStream tgzStream = getResourceAsInputStream("npm", "tar", "tgz-evil.tgz");
NpmPackage npmPackage = NpmPackage.fromPackage(tgzStream);
});
assertNotNull(thrown);
assertEquals("Entry with an illegal name: ../evil.txt", thrown.getMessage());
}
}

View File

@ -133,7 +133,7 @@ public class Scanner {
protected void genScanOutput(String folder, List<ScanOutputItem> items) throws IOException, FHIRException, EOperationOutcome {
String f = Utilities.path(folder, "comparison.zip");
download("http://fhir.org/archive/comparison.zip", f);
download("https://fhir.org/archive/comparison.zip", f);
unzip(f, folder);
for (int i = 0; i < items.size(); i++) {
@ -342,13 +342,18 @@ public class Scanner {
// iterates over entries in the zip file
while (entry != null) {
String filePath = destDirectory + File.separator + entry.getName();
final File zipEntryFile = new File(destDirectory, entry.getName());
if (!zipEntryFile.toPath().normalize().startsWith(destDirectory)) {
throw new RuntimeException("Entry with an illegal path: " + entry.getName());
}
if (!entry.isDirectory()) {
// if the entry is a file, extracts it
// if the entry is a file, extract it
extractFile(zipIn, filePath);
} else {
// if the entry is a directory, make the directory
File dir = new File(filePath);
dir.mkdir();
zipEntryFile.mkdir();
}
zipIn.closeEntry();
entry = zipIn.getNextEntry();

View File

@ -0,0 +1,90 @@
package org.hl7.fhir.validation;
import org.hl7.fhir.utilities.tests.ResourceLoaderTests;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
public class ScannerTest implements ResourceLoaderTests {
public static final String ZIP_NORMAL_ZIP = "zip-normal.zip";
public static final String ZIP_SLIP_ZIP = "zip-slip.zip";
public static final String ZIP_SLIP_2_ZIP = "zip-slip-2.zip";
public static final String ZIP_SLIP_WIN_ZIP = "zip-slip-win.zip";
Path tempDir;
Path zipNormalPath;
Path zipSlipPath;
Path zipSlip2Path;
Path zipSlipWinPath;
@BeforeAll
public void beforeAll() throws IOException {
tempDir = Files.createTempDirectory("scanner-zip");
tempDir.resolve("child").toFile().mkdir();
zipNormalPath = tempDir.resolve(ZIP_NORMAL_ZIP);
zipSlipPath = tempDir.resolve(ZIP_SLIP_ZIP);
zipSlip2Path = tempDir.resolve(ZIP_SLIP_2_ZIP);
zipSlipWinPath = tempDir.resolve(ZIP_SLIP_WIN_ZIP);
copyResourceToFile(zipNormalPath, "scanner", ZIP_NORMAL_ZIP);
copyResourceToFile(zipSlipPath, "scanner", ZIP_SLIP_ZIP);
copyResourceToFile(zipSlip2Path, "scanner", ZIP_SLIP_2_ZIP);
copyResourceToFile(zipSlipWinPath, "scanner", ZIP_SLIP_WIN_ZIP);
}
@Test
public void testNormalZip() throws IOException {
Scanner scanner = new Scanner(null,null,null,null);
scanner.unzip(zipNormalPath.toFile().getAbsolutePath(), tempDir.toFile().getAbsolutePath());
Path expectedFilePath = tempDir.resolve("zip-normal").resolve("depth1").resolve("test.txt");
String actualContent = Files.readString(expectedFilePath);
assertEquals("dummy file content", actualContent);
}
@Test
public void testSlipZip() throws IOException {
RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, () -> {
Scanner scanner = new Scanner(null,null,null,null);
scanner.unzip(zipSlipPath.toFile().getAbsolutePath(), tempDir.toFile().getAbsolutePath());
//Code under test
});
assertNotNull(thrown);
assertEquals("Entry with an illegal path: ../evil.txt", thrown.getMessage());
}
@Test
public void testSlipZip2() throws IOException {
RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, () -> {
Scanner scanner = new Scanner(null,null,null,null);
scanner.unzip(zipSlip2Path.toFile().getAbsolutePath(), tempDir.toFile().getAbsolutePath());
//Code under test
});
assertNotNull(thrown);
assertEquals("Entry with an illegal path: child/../../evil.txt", thrown.getMessage());
}
@Test
public void testSlipZipWin() throws IOException {
RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, () -> {
Scanner scanner = new Scanner(null,null,null,null);
scanner.unzip(zipSlipWinPath.toFile().getAbsolutePath(), tempDir.toFile().getAbsolutePath());
//Code under test
});
assertNotNull(thrown);
assertEquals("Entry with an illegal path: ../evil.txt", thrown.getMessage());
}
}