This is an automated email from the ASF dual-hosted git repository. pauls pushed a commit to branch FELIX-6535 in repository https://gitbox.apache.org/repos/asf/felix-dev.git
commit 6760756b15597aa522750e0fab12bf2559263b97 Author: Karl Pauls <[email protected]> AuthorDate: Fri May 13 12:41:38 2022 +0200 Cleanup directory revision exception handeling --- .../felix/framework/cache/DirectoryContent.java | 147 +++++++++++++++------ .../felix/framework/cache/BundleCacheTest.java | 13 +- 2 files changed, 116 insertions(+), 44 deletions(-) diff --git a/framework/src/main/java/org/apache/felix/framework/cache/DirectoryContent.java b/framework/src/main/java/org/apache/felix/framework/cache/DirectoryContent.java index c462b5539f..3e4a2ad934 100644 --- a/framework/src/main/java/org/apache/felix/framework/cache/DirectoryContent.java +++ b/framework/src/main/java/org/apache/felix/framework/cache/DirectoryContent.java @@ -27,6 +27,7 @@ import org.osgi.framework.Constants; import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.io.UncheckedIOException; import java.net.MalformedURLException; import java.net.URL; import java.util.Enumeration; @@ -48,9 +49,10 @@ public class DirectoryContent implements Content private final File m_rootDir; private final File m_dir; private Map m_nativeLibMap; + private final String m_canonicalRoot; public DirectoryContent(Logger logger, Map configMap, - WeakZipFileFactory zipFactory, Object revisionLock, File rootDir, File dir) + WeakZipFileFactory zipFactory, Object revisionLock, File rootDir, File dir) { m_logger = logger; m_configMap = configMap; @@ -58,6 +60,16 @@ public class DirectoryContent implements Content m_revisionLock = revisionLock; m_rootDir = rootDir; m_dir = dir; + String canonicalPath = null; + try { + canonicalPath = BundleCache.getSecureAction().getCanonicalPath(m_dir); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + if (!canonicalPath.endsWith(File.separator)) { + canonicalPath = canonicalPath + "/"; + } + m_canonicalRoot = canonicalPath; } public File getFile() @@ -77,9 +89,17 @@ public class DirectoryContent implements Content // Return true if the file associated with the entry exists, // unless the entry name ends with "/", in which case only // return true if the file is really a directory. - File file = new File(m_dir, name); + File file = null; + try + { + file = getFile(name); + } + catch (IOException e) + { + return false; + } return BundleCache.getSecureAction().fileExists(file) - && (name.endsWith("/") + && (name.endsWith("/") ? BundleCache.getSecureAction().isFileDirectory(file) : true); } @@ -91,7 +111,15 @@ public class DirectoryContent implements Content // Return true if the file associated with the entry exists, // unless the entry name ends with "/", in which case only // return true if the file is really a directory. - File file = new File(m_dir, name); + File file = null; + try + { + file = getFile(name); + } + catch (IOException e) + { + return false; + } return BundleCache.getSecureAction().isFileDirectory(file); } @@ -110,36 +138,36 @@ public class DirectoryContent implements Content // Get the embedded resource. - File file = new File(m_dir, name); try { + File file = getFile(name); return BundleCache.getSecureAction().fileExists(file) ? BundleCache.read(BundleCache.getSecureAction().getInputStream(file), file.length()) : null; } catch (Exception ex) { m_logger.log( - Logger.LOG_ERROR, - "DirectoryContent: Unable to read bytes for file " + name + " from file " + file.getAbsolutePath(), ex); + Logger.LOG_ERROR, + "DirectoryContent: Unable to read bytes for file " + name + " from file " + new File(m_dir, name).getAbsolutePath(), ex); return null; } } public InputStream getEntryAsStream(String name) - throws IllegalStateException, IOException + throws IllegalStateException, IOException { name = getName(name); - File file = new File(m_dir, name); try { + File file = getFile(name); return BundleCache.getSecureAction().fileExists(file) ? BundleCache.getSecureAction().getInputStream(file) : null; } catch (Exception ex) { m_logger.log( - Logger.LOG_ERROR, - "DirectoryContent: Unable to create inputstream for file " + name + " from file " + file.getAbsolutePath(), ex); + Logger.LOG_ERROR, + "DirectoryContent: Unable to create inputstream for file " + name + " from file " + new File(m_dir, name).getAbsolutePath(), ex); return null; } } @@ -152,6 +180,22 @@ public class DirectoryContent implements Content return name; } + private File getFile(String name) throws IOException + { + File result = new File(m_dir, name); + + String canonicalPath = BundleCache.getSecureAction().getCanonicalPath(result); + if (BundleCache.getSecureAction().isFileDirectory(result) && !canonicalPath.endsWith(File.separator)) + { + canonicalPath += File.separator; + } + if (!canonicalPath.startsWith(m_canonicalRoot)) + { + throw new IOException("File outside the root: " + canonicalPath); + } + return result; + } + public URL getEntryAsURL(String name) { name = getName(name); @@ -160,9 +204,9 @@ public class DirectoryContent implements Content { try { - return BundleCache.getSecureAction().toURI(new File(m_dir, name)).toURL(); + return BundleCache.getSecureAction().toURI(getFile(name)).toURL(); } - catch (MalformedURLException e) + catch (IOException e) { return null; } @@ -178,7 +222,12 @@ public class DirectoryContent implements Content { name = getName(name); - File file = new File(m_dir, name); + File file = null; + try { + file = getFile(name); + } catch (IOException e) { + return 0L; + } return BundleCache.getSecureAction().getLastModified(file); } @@ -190,7 +239,7 @@ public class DirectoryContent implements Content if (entryName.equals(FelixConstants.CLASS_PATH_DOT)) { return new DirectoryContent( - m_logger, m_configMap, m_zipFactory, m_revisionLock, m_rootDir, m_dir); + m_logger, m_configMap, m_zipFactory, m_revisionLock, m_rootDir, m_dir); } // Remove any leading slash, since all bundle class path @@ -198,9 +247,9 @@ public class DirectoryContent implements Content entryName = (entryName.startsWith("/")) ? entryName.substring(1) : entryName; if (entryName.trim().startsWith(".." + File.separatorChar) || - entryName.contains(File.separator + ".." + File.separatorChar) || - entryName.trim().endsWith(File.separator + "..") || - entryName.trim().equals("..")) + entryName.contains(File.separator + ".." + File.separatorChar) || + entryName.trim().endsWith(File.separator + "..") || + entryName.trim().equals("..")) { return null; } @@ -208,33 +257,38 @@ public class DirectoryContent implements Content // Any embedded JAR files will be extracted to the embedded directory. File embedDir = new File(m_rootDir, m_dir.getName() + EMBEDDED_DIRECTORY); - // Determine if the entry is an emdedded JAR file or + // Determine if the entry is an embedded JAR file or // directory in the bundle JAR file. Ignore any entries // that do not exist per the spec. - File file = new File(m_dir, entryName); + File file = null; + try { + file = getFile(entryName); + } catch (IOException e) { + return null; + } if (BundleCache.getSecureAction().isFileDirectory(file)) { return new DirectoryContent( - m_logger, m_configMap, m_zipFactory, m_revisionLock, m_rootDir, file); + m_logger, m_configMap, m_zipFactory, m_revisionLock, m_rootDir, file); } else if (BundleCache.getSecureAction().fileExists(file) - && entryName.endsWith(".jar")) + && entryName.endsWith(".jar")) { File extractDir = new File(embedDir, - (entryName.lastIndexOf('/') >= 0) - ? entryName.substring(0, entryName.lastIndexOf('/')) - : entryName); + (entryName.lastIndexOf('/') >= 0) + ? entryName.substring(0, entryName.lastIndexOf('/')) + : entryName); return new JarContent( - m_logger, m_configMap, m_zipFactory, m_revisionLock, - extractDir, file, null); + m_logger, m_configMap, m_zipFactory, m_revisionLock, + extractDir, file, null); } // The entry could not be found, so return null. return null; } -// TODO: SECURITY - This will need to consider security. + // TODO: SECURITY - This will need to consider security. public String getEntryAsNativeLibrary(String entryName) { // Return result. @@ -245,9 +299,9 @@ public class DirectoryContent implements Content entryName = (entryName.startsWith("/")) ? entryName.substring(1) : entryName; if (entryName.trim().startsWith(".." + File.separatorChar) || - entryName.contains(File.separator + ".." + File.separatorChar) || - entryName.trim().endsWith(File.separator + "..") || - entryName.trim().equals("..")) + entryName.contains(File.separator + ".." + File.separatorChar) || + entryName.trim().endsWith(File.separator + "..") || + entryName.trim().equals("..")) { return null; } @@ -257,9 +311,18 @@ public class DirectoryContent implements Content // The entry must exist and refer to a file, not a directory, // since we are expecting it to be a native library. - File entryFile = new File(m_dir, entryName); + File entryFile = null; + try + { + entryFile = getFile(entryName); + } + catch (IOException e) + { + return null; + } + if (BundleCache.getSecureAction().fileExists(entryFile) - && !BundleCache.getSecureAction().isFileDirectory(entryFile)) + && !BundleCache.getSecureAction().isFileDirectory(entryFile)) { // Extracting the embedded native library file impacts all other // existing contents for this revision, so we have to grab the @@ -279,16 +342,16 @@ public class DirectoryContent implements Content libCount = (libCount == null) ? new Integer(0) : new Integer(libCount.intValue() + 1); m_nativeLibMap.put(entryName, libCount); File libFile = new File( - libDir, libCount.toString() + File.separatorChar + entryName); + libDir, libCount.toString() + File.separatorChar + entryName); if (!BundleCache.getSecureAction().fileExists(libFile)) { if (!BundleCache.getSecureAction().fileExists(libFile.getParentFile()) - && !BundleCache.getSecureAction().mkdirs(libFile.getParentFile())) + && !BundleCache.getSecureAction().mkdirs(libFile.getParentFile())) { m_logger.log( - Logger.LOG_ERROR, - "Unable to create library directory."); + Logger.LOG_ERROR, + "Unable to create library directory."); } else { @@ -304,7 +367,7 @@ public class DirectoryContent implements Content // Perform exec permission command on extracted library // if one is configured. String command = (String) m_configMap.get( - Constants.FRAMEWORK_EXECPERMISSION); + Constants.FRAMEWORK_EXECPERMISSION); if (command != null) { Properties props = new Properties(); @@ -320,8 +383,8 @@ public class DirectoryContent implements Content catch (Exception ex) { m_logger.log( - Logger.LOG_ERROR, - "Extracting native library.", ex); + Logger.LOG_ERROR, + "Extracting native library.", ex); } } } @@ -367,7 +430,7 @@ public class DirectoryContent implements Content // Convert the file separator character to slashes. String abs = BundleCache.getSecureAction() - .getAbsolutePath(m_children[m_counter]).replace(File.separatorChar, '/'); + .getAbsolutePath(m_children[m_counter]).replace(File.separatorChar, '/'); // Remove the leading path of the reference directory, since the // entry paths are supposed to be relative to the root. @@ -398,7 +461,7 @@ public class DirectoryContent implements Content File[] tmp = new File[combined.length + grandchildren.length]; System.arraycopy(combined, 0, tmp, 0, combined.length); System.arraycopy( - grandchildren, 0, tmp, combined.length, grandchildren.length); + grandchildren, 0, tmp, combined.length, grandchildren.length); combined = tmp; } } diff --git a/framework/src/test/java/org/apache/felix/framework/cache/BundleCacheTest.java b/framework/src/test/java/org/apache/felix/framework/cache/BundleCacheTest.java index 5dbfb74ef3..793334d81a 100644 --- a/framework/src/test/java/org/apache/felix/framework/cache/BundleCacheTest.java +++ b/framework/src/test/java/org/apache/felix/framework/cache/BundleCacheTest.java @@ -152,7 +152,14 @@ public class BundleCacheTest extends TestCase public void testDirectoryReference() throws Exception { - testBundle("reference:" + archiveFile.toURI().toURL(), null); + BundleArchive archive = testBundle("reference:" + archiveFile.toURI().toURL(), null); + String path = "../../../../../../../../../../../../../../../../..".replace("/", File.separator); + assertFalse(archive.getCurrentRevision().getContent().hasEntry(path)); + assertFalse(archive.getCurrentRevision().getContent().isDirectory(path)); + assertNull(archive.getCurrentRevision().getContent().getEntryAsURL(path)); + assertNull(archive.getCurrentRevision().getContent().getEntryAsBytes(path + jarFile.getAbsolutePath())); + assertNull(archive.getCurrentRevision().getContent().getEntryAsNativeLibrary(path + jarFile.getAbsolutePath())); + assertEquals(0L, archive.getCurrentRevision().getContent().getContentTime(path + jarFile.getAbsolutePath())); } public void testJarReference() throws Exception @@ -170,7 +177,7 @@ public class BundleCacheTest extends TestCase testBundle("bla", jarFile); } - private void testBundle(String location, File file) throws Exception + private BundleArchive testBundle(String location, File file) throws Exception { BundleArchive archive = cache.create(1, 1, location, file != null ? new FileInputStream(file) : null, null); @@ -211,6 +218,8 @@ public class BundleCacheTest extends TestCase assertFalse(new File(nativeLib).isFile()); assertTrue(new File(nativeLib2).isFile()); assertTrue(new File(nativeLib3).isFile()); + + return archive; } private void testRevision(BundleArchive archive) throws Exception
