pranavshuklaa commented on code in PR #1091:
URL: https://github.com/apache/flink-agents/pull/1091#discussion_r3942510592
##########
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) {
Review Comment:
yes thanks for pointing that out. materialized is now constructed before
extraction runs, using the already-registered shutdown hook. The extraction
block now calls materialized.close() for both IOException and RuntimeException
before re-throwing. Since close() removes the shutdown hook and deletes the
extraction directory, malformed entries such as one containing a NUL character
can no longer leave either the hook or temporary directory behind.
--
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]
