Skip to content

Commit 0f42095

Browse files
author
“sahvx655-wq”
committed
FELIX-6836: Mitigate path traversal (Zip Slip) vulnerability in cache handlers
1 parent f173791 commit 0f42095

4 files changed

Lines changed: 151 additions & 43 deletions

File tree

‎framework/src/main/java/org/apache/felix/framework/cache/ConnectContentContent.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,19 @@ public Content getEntryAsContent(String name)
146146
{
147147
return this;
148148
}
149+
150+
// Remove any leading slash.
151+
String entryName = (name.startsWith("/")) ? name.substring(1) : name;
152+
153+
String normalizedEntryName = entryName.replace('\\', '/');
154+
if (normalizedEntryName.trim().startsWith("../") ||
155+
normalizedEntryName.contains("/../") ||
156+
normalizedEntryName.trim().endsWith("/..") ||
157+
normalizedEntryName.trim().equals(".."))
158+
{
159+
return null;
160+
}
161+
149162
String dir = name.endsWith("/") ? name : name + "/";
150163

151164
if (hasEntry(dir))

‎framework/src/main/java/org/apache/felix/framework/cache/DirectoryContent.java‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -252,10 +252,11 @@ public Content getEntryAsContent(String entryName)
252252
// entries are relative to the root of the bundle.
253253
entryName = (entryName.startsWith("/")) ? entryName.substring(1) : entryName;
254254

255-
if (entryName.trim().startsWith(".." + File.separatorChar) ||
256-
entryName.contains(File.separator + ".." + File.separatorChar) ||
257-
entryName.trim().endsWith(File.separator + "..") ||
258-
entryName.trim().equals(".."))
255+
String normalizedEntryName = entryName.replace('\\', '/');
256+
if (normalizedEntryName.trim().startsWith("../") ||
257+
normalizedEntryName.contains("/../") ||
258+
normalizedEntryName.trim().endsWith("/..") ||
259+
normalizedEntryName.trim().equals(".."))
259260
{
260261
return null;
261262
}
@@ -305,10 +306,11 @@ public String getEntryAsNativeLibrary(String entryName)
305306
// entries are relative to the root of the bundle.
306307
entryName = (entryName.startsWith("/")) ? entryName.substring(1) : entryName;
307308

308-
if (entryName.trim().startsWith(".." + File.separatorChar) ||
309-
entryName.contains(File.separator + ".." + File.separatorChar) ||
310-
entryName.trim().endsWith(File.separator + "..") ||
311-
entryName.trim().equals(".."))
309+
String normalizedEntryName = entryName.replace('\\', '/');
310+
if (normalizedEntryName.trim().startsWith("../") ||
311+
normalizedEntryName.contains("/../") ||
312+
normalizedEntryName.trim().endsWith("/..") ||
313+
normalizedEntryName.trim().equals(".."))
312314
{
313315
return null;
314316
}

‎framework/src/main/java/org/apache/felix/framework/cache/JarContent.java‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -241,10 +241,11 @@ public Content getEntryAsContent(String entryName)
241241
// Remove any leading slash.
242242
entryName = (entryName.startsWith("/")) ? entryName.substring(1) : entryName;
243243

244-
if (entryName.trim().startsWith(".." + File.separatorChar) ||
245-
entryName.contains(File.separator + ".." + File.separatorChar) ||
246-
entryName.trim().endsWith(File.separator + "..") ||
247-
entryName.trim().equals(".."))
244+
String normalizedEntryName = entryName.replace('\\', '/');
245+
if (normalizedEntryName.trim().startsWith("../") ||
246+
normalizedEntryName.contains("/../") ||
247+
normalizedEntryName.trim().endsWith("/..") ||
248+
normalizedEntryName.trim().equals(".."))
248249
{
249250
return null;
250251
}
@@ -320,10 +321,11 @@ public String getEntryAsNativeLibrary(String entryName)
320321
// Remove any leading slash.
321322
entryName = (entryName.startsWith("/")) ? entryName.substring(1) : entryName;
322323

323-
if (entryName.trim().startsWith(".." + File.separatorChar) ||
324-
entryName.contains(File.separator + ".." + File.separatorChar) ||
325-
entryName.trim().endsWith(File.separator + "..") ||
326-
entryName.trim().equals(".."))
324+
String normalizedEntryName = entryName.replace('\\', '/');
325+
if (normalizedEntryName.trim().startsWith("../") ||
326+
normalizedEntryName.contains("/../") ||
327+
normalizedEntryName.trim().endsWith("/..") ||
328+
normalizedEntryName.trim().equals(".."))
327329
{
328330
return null;
329331
}

‎framework/src/test/java/org/apache/felix/framework/cache/BundleCacheTest.java‎

Lines changed: 118 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,11 @@
3333
import java.io.IOException;
3434
import java.io.InputStream;
3535
import java.net.URL;
36+
import java.util.ArrayList;
3637
import java.util.Arrays;
3738
import java.util.Enumeration;
3839
import java.util.HashMap;
40+
import java.util.List;
3941
import java.util.Map;
4042
import java.util.Set;
4143
import java.util.TreeSet;
@@ -45,13 +47,27 @@
4547

4648
class BundleCacheTest
4749
{
50+
private final List<BundleArchive> archives = new ArrayList<>();
4851
private File tempDir;
4952
private File cacheDir;
5053
private File filesDir;
5154
private BundleCache cache;
5255
private File archiveFile;
5356
private File jarFile;
5457

58+
private static final String SPECIAL_JAR_ENTRY = File.separatorChar == '\\'
59+
? "inner/i+~äö §$%nner.jar"
60+
: "inner/i+~äö \\§$%nner.jar";
61+
62+
private BundleArchive track(BundleArchive archive)
63+
{
64+
if (archive != null)
65+
{
66+
archives.add(archive);
67+
}
68+
return archive;
69+
}
70+
5571
@BeforeEach
5672
void setUp() throws Exception
5773
{
@@ -85,7 +101,7 @@ protected void doLog(int level, String msg, Throwable throwable) {
85101

86102
new File(innerArchiveFile, "empty").mkdirs();
87103

88-
createJar(archiveFile, new File(archiveFile, "inner/i+?äö \\§$%nner.jar"));
104+
createJar(archiveFile, new File(archiveFile, SPECIAL_JAR_ENTRY));
89105

90106
Manifest manifest = new Manifest();
91107
manifest.getMainAttributes().putValue("foo", "bar");
@@ -113,21 +129,28 @@ void noZipSlip() throws Exception
113129

114130
output.putNextEntry(new ZipEntry("../../bar.jar"));
115131

116-
output.write(BundleCache.read(new FileInputStream(jarFile), jarFile.length()));
132+
try (FileInputStream fis = new FileInputStream(jarFile))
133+
{
134+
output.write(BundleCache.read(fis, jarFile.length()));
135+
}
117136

118137
output.closeEntry();
119138

120139
output.close();
121140

122-
BundleArchive archive = cache.create(1, 1, "slip", new FileInputStream(bundle), null);
141+
BundleArchive archive;
142+
try (FileInputStream fis = new FileInputStream(bundle))
143+
{
144+
archive = track(cache.create(1, 1, "slip", fis, null));
145+
}
123146

124147
noZipSlip(archive);
125148

126-
archive = cache.create(1, 1, bundle.toURI().toURL().toString(), null, null);
149+
archive = track(cache.create(1, 1, bundle.toURI().toURL().toString(), null, null));
127150

128151
noZipSlip(archive);
129152

130-
archive = cache.create(1, 1, "reference:" + bundle.toURI().toURL().toString(), null, null);
153+
archive = track(cache.create(1, 1, "reference:" + bundle.toURI().toURL().toString(), null, null));
131154

132155
noZipSlip(archive);
133156

@@ -139,7 +162,7 @@ void noZipSlip() throws Exception
139162
test.createNewFile();
140163
test.deleteOnExit();
141164

142-
archive = cache.create(1, 1, "reference:" + dir.toURI().toURL().toString(), null, null);
165+
archive = track(cache.create(1, 1, "reference:" + dir.toURI().toURL().toString(), null, null));
143166

144167
noZipSlip(archive);
145168
}
@@ -189,7 +212,18 @@ void inputStream() throws Exception
189212
@Test
190213
private BundleArchive bundle(String location, File file) throws Exception
191214
{
192-
BundleArchive archive = cache.create(1, 1, location, file != null ? new FileInputStream(file) : null, null);
215+
BundleArchive archive;
216+
if (file != null)
217+
{
218+
try (FileInputStream fis = new FileInputStream(file))
219+
{
220+
archive = track(cache.create(1, 1, location, fis, null));
221+
}
222+
}
223+
else
224+
{
225+
archive = track(cache.create(1, 1, location, null, null));
226+
}
193227

194228
assertThat(archive).isNotNull();
195229

@@ -202,7 +236,17 @@ private BundleArchive bundle(String location, File file) throws Exception
202236
assertThat(nativeLib).isNotNull();
203237
assertThat(new File(nativeLib)).isFile();
204238

205-
archive.revise(location, file != null ? new FileInputStream(file) : null);
239+
if (file != null)
240+
{
241+
try (FileInputStream fis = new FileInputStream(file))
242+
{
243+
archive.revise(location, fis);
244+
}
245+
}
246+
else
247+
{
248+
archive.revise(location, null);
249+
}
206250

207251
assertThat(archive.getCurrentRevisionNumber()).isEqualTo(1L);
208252

@@ -239,12 +283,13 @@ private void revision(BundleArchive archive) throws Exception
239283
assertThat(revision).isNotNull();
240284
assertThat(revision.getManifestHeader()).isNotNull();
241285
assertThat(revision.getManifestHeader()).containsEntry("foo", "bar");
242-
perRevision(revision.getContent(), new TreeSet<>(Arrays.asList("META-INF/", "META-INF/MANIFEST.MF", "file1", "inner/", "inner/empty/", "inner/file1", "inner/i+?äö \\§$%nner.jar")));
243-
perRevision(revision.getContent().getEntryAsContent("inner"), new TreeSet<>(Arrays.asList("file1", "empty/", "i+?äö \\§$%nner.jar")));
286+
String specialEntry = SPECIAL_JAR_ENTRY.replace(File.separatorChar, '/');
287+
perRevision(revision.getContent(), new TreeSet<>(Arrays.asList("META-INF/", "META-INF/MANIFEST.MF", "file1", "inner/", "inner/empty/", "inner/file1", specialEntry)));
288+
perRevision(revision.getContent().getEntryAsContent("inner"), new TreeSet<>(Arrays.asList("file1", "empty/", specialEntry.substring("inner/".length()))));
244289
assertThat(revision.getContent().getEntryAsContent("inner/inner")).isNull();
245290
assertThat(revision.getContent().getEntryAsContent("inner/empty/")).isNotNull();
246291
assertThat(revision.getContent().getEntryAsContent("inner/empty").getEntries()).isNull();
247-
perRevision(revision.getContent().getEntryAsContent("inner/").getEntryAsContent("i+?äö \\§$%nner.jar"), new TreeSet<>(Arrays.asList("file1", "inner/", "inner/empty/", "inner/file1")));
292+
perRevision(revision.getContent().getEntryAsContent("inner/").getEntryAsContent(specialEntry.substring("inner/".length())), new TreeSet<>(Arrays.asList("file1", "inner/", "inner/empty/", "inner/file1")));
248293
}
249294

250295
private void perRevision(Content content, Set<String> expectedEntries) throws Exception
@@ -269,29 +314,35 @@ private void perRevision(Content content, Set<String> expectedEntries) throws Ex
269314
assertThat(content.getEntryAsBytes("foo/bar")).isNull();
270315

271316

272-
InputStream input = content.getEntryAsStream("file1");
273-
assertThat(input).isNotNull();
274-
entry = new byte[1014];
275-
int j = 0;
276-
for (int i = input.read();i != -1; i = input.read())
317+
try (InputStream input = content.getEntryAsStream("file1"))
277318
{
278-
entry[j++] = (byte) i;
319+
assertThat(input).isNotNull();
320+
entry = new byte[1014];
321+
int j = 0;
322+
for (int i = input.read();i != -1; i = input.read())
323+
{
324+
entry[j++] = (byte) i;
325+
}
326+
assertThat(new String(entry, 0, j, "UTF-8")).isEqualTo("file1");
279327
}
280-
assertThat(new String(entry, 0, j, "UTF-8")).isEqualTo("file1");
281328
assertThat(content.getEntryAsStream("foo")).isNull();
282329
assertThat(content.getEntryAsStream("foo/bar")).isNull();
283330

284331
URL url = content.getEntryAsURL("file1");
285332
assertThat(url).isNotNull();
286-
input = url.openStream();
287-
assertThat(input).isNotNull();
288-
entry = new byte[1014];
289-
j = 0;
290-
for (int i = input.read();i != -1; i = input.read())
333+
java.net.URLConnection conn = url.openConnection();
334+
conn.setUseCaches(false);
335+
try (InputStream input = conn.getInputStream())
291336
{
292-
entry[j++] = (byte) i;
337+
assertThat(input).isNotNull();
338+
entry = new byte[1014];
339+
int j = 0;
340+
for (int i = input.read();i != -1; i = input.read())
341+
{
342+
entry[j++] = (byte) i;
343+
}
344+
assertThat(new String(entry, 0, j, "UTF-8")).isEqualTo("file1");
293345
}
294-
assertThat(new String(entry, 0, j, "UTF-8")).isEqualTo("file1");
295346
assertThat(content.getEntryAsURL("foo")).isNull();
296347
assertThat(content.getEntryAsURL("foo/bar")).isNull();
297348

@@ -304,11 +355,45 @@ private void perRevision(Content content, Set<String> expectedEntries) throws Ex
304355

305356
@AfterEach
306357
void tearDown() throws Exception {
358+
for (BundleArchive archive : archives)
359+
{
360+
try
361+
{
362+
archive.close();
363+
}
364+
catch (Exception e)
365+
{
366+
// ignore
367+
}
368+
}
369+
archives.clear();
370+
cache.release();
307371
cache.delete();
372+
if (cacheDir.exists())
373+
{
374+
System.out.println("--- CACHE DIR DELETION FAILED. FILES REMAINING: ---");
375+
printFiles(cacheDir);
376+
}
308377
assertThat(cacheDir.exists()).isFalse();
309378
assertThat(BundleCache.deleteDirectoryTree(tempDir)).isTrue();
310379
}
311380

381+
private void printFiles(File dir)
382+
{
383+
File[] files = dir.listFiles();
384+
if (files != null)
385+
{
386+
for (File f : files)
387+
{
388+
System.out.println(f.getAbsolutePath());
389+
if (f.isDirectory())
390+
{
391+
printFiles(f);
392+
}
393+
}
394+
}
395+
}
396+
312397
private void createTestArchive(File archiveFile) throws Exception
313398
{
314399
createFile(archiveFile, "file1", "file1".getBytes("UTF-8"));
@@ -334,7 +419,10 @@ private void createJar(File source, File target) throws Exception
334419
JarOutputStream output;
335420
if (new File(source, "META-INF/MANIFEST.MF").isFile())
336421
{
337-
output = new JarOutputStream(new FileOutputStream(tmp),new Manifest(new FileInputStream(new File(source, "META-INF/MANIFEST.MF"))));
422+
try (FileInputStream fis = new FileInputStream(new File(source, "META-INF/MANIFEST.MF")))
423+
{
424+
output = new JarOutputStream(new FileOutputStream(tmp), new Manifest(fis));
425+
}
338426
}
339427
else
340428
{
@@ -376,7 +464,10 @@ private void writeRecursive(File current, String path, JarOutputStream output) t
376464
}
377465
else if (current.isFile())
378466
{
379-
output.write(BundleCache.read(new FileInputStream(current), current.length()));
467+
try (FileInputStream fis = new FileInputStream(current))
468+
{
469+
output.write(BundleCache.read(fis, current.length()));
470+
}
380471
output.closeEntry();
381472
}
382473
}

0 commit comments

Comments
 (0)