pranavshuklaa commented on code in PR #1091:
URL: https://github.com/apache/flink-agents/pull/1091#discussion_r3942516981
##########
runtime/src/main/java/org/apache/flink/agents/runtime/skill/repository/SkillMaterializer.java:
##########
@@ -119,36 +143,154 @@ private static Thread registerCleanup(Path path) {
/**
* Extract a zip into a fresh temp directory and return a {@link
Materialized} handle owning
- * that directory. Validates every entry against zip-slip (paths must
resolve inside the
- * extraction directory). Registers a JVM shutdown hook as fallback
cleanup; callers should
- * {@link Materialized#close()} the handle to free the dir eagerly.
+ * that directory.
+ *
+ * <p>Security properties:
+ *
+ * <ul>
+ * <li>Validates every entry against zip-slip before any extraction
begins.
+ * <li>Rejects archives with more than {@link #MAX_EXTRACT_ENTRIES}
entries.
+ * <li>Enforces {@link #MAX_EXTRACT_ENTRY_BYTES} per entry and {@link
+ * #MAX_EXTRACT_TOTAL_BYTES} cumulatively, measured against actual
decompressed bytes
+ * written — not against the declared sizes in the zip central
directory, which are
+ * attacker-controlled.
+ * <li>Eagerly deletes the extraction directory on any failure, in
addition to the JVM
+ * shutdown hook registered as a fallback.
+ * </ul>
*
- * @throws IOException if any zip entry resolves outside the extraction
directory.
+ * <p>These bounds apply to all callers (URL, filesystem, classpath,
package sources).
+ *
+ * @throws IOException if any zip entry resolves outside the extraction
directory, if any size
+ * cap is exceeded, or on I/O errors.
*/
public static Materialized extractZipSafely(Path zipPath) throws
IOException {
Path extractDir = Files.createTempDirectory(TEMP_DIR_PREFIX);
- // Register cleanup before validation so the empty tempdir is always
reclaimed,
- // even if validation raises.
+
+ // Register the fallback cleanup hook before any work so the empty dir
is always reclaimed,
+ // even if validation or extraction raises.
Thread hook = registerCleanup(extractDir);
+
+ try {
+ extractZipSafelyInto(zipPath, extractDir);
+ } catch (IOException e) {
+ // Eager cleanup: the hook is the fallback but callers may never
call close() if we
+ // throw. Delete now so a failed extraction leaves no partial
content behind.
+ deleteRecursively(extractDir);
+ throw e;
+ }
+
+ return new Materialized(extractDir, hook);
+ }
+
+ /**
+ * Core extraction logic: validates, checks bounds, then extracts.
Separated from {@link
+ * #extractZipSafely} so the caller can handle cleanup on failure.
+ */
+ private static void extractZipSafelyInto(Path zipPath, Path extractDir)
throws IOException {
try (ZipFile zf = new ZipFile(zipPath.toFile())) {
- Enumeration<? extends ZipEntry> entries = zf.entries();
- while (entries.hasMoreElements()) {
- ZipEntry entry = entries.nextElement();
+ List<? extends ZipEntry> entries = Collections.list(zf.entries());
Review Comment:
changed this. now checking zf.size() before calling Collections.list The
entry count is read directly from the archive's central directory, so an
archive exceeding the configured entry-count limit is rejected before
constructing the ZipEntry list. This avoids making memory usage before
rejection proportional to an attacker controlled number of entries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
