From 38212e757c5f59805e2a85f9ab7a789a506d53c4 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 22 Sep 2026 15:16:59 +0200 Subject: [PATCH] :bug: Bound decompressed size of JSON entries on binfile v3 import (#11607) * :bug: Bound decompressed size of JSON entries on binfile v3 import Every JSON/text zip entry (manifest, files, pages, shapes, colors, components, typographies, tokens, plugin-data) was decompressed without any size limit, letting a small .penpot archive exhaust the backend heap (GHSA-qcw7-v626-g6cf). Only binary storage blobs were guarded. Reuse the existing size-limiting-stream guard on the text path: 20 MiB cap per entry, 200 MiB cumulative budget per import job, plus a cheap declared-size pre-check. Both limits are configurable and wired through the binfile, management and debug entry points. Adds zip-bomb regression tests for the file entry, the synchronous manifest read and the cumulative budget. Closes #11606 AI-assisted-by: muse-spark-1.3-contributor * :recycle: Uniform binfile import limits behind init-limits Move the binfile import limits to a single source of truth in app.binfile.common (default-* vars) and drop the duplicated entries from config/default; env overrides keep working through the schema. Resolve all limits once per job with init-limits (::max-size, ::total-max, ::current-size, ::max-object-size, ::max-zip-entries) instead of rebuilding the map per zip entry. Thread cfg as the first arg through the v3 readers, collapse read-plain-entry into read-entry, and give size-limiting-stream a single explicit-counter arity. v1 keeps using the compiled default (mechanical rename only). No behavior change. AI-assisted-by: muse-spark-1.3-contributor * :recycle: Rename binfile limits to text-entry/binary-entry terms Use text-entry/binary-entry vocabulary consistently across config keys, bfc input keys, default-* vars and the limits resolved by setup-limits (::max-text-entry-size, ::max-text-total-size, ::current-text-size, ::max-binary-entry-size). Rename init-limits to setup-limits. No behavior change. AI-assisted-by: muse-spark-1.3-contributor * :recycle: Rename cumulative text counter and document binary limit Rename ::current-text-size to ::accumulated-total-text-size for clarity and expand the default-max-binary-entry-size comment to match the other limit vars. No behavior change. AI-assisted-by: muse-spark-1.3-contributor * :recycle: Harden binfile guards and prove budget accumulation Add a regression test that only passes when text bytes accumulate across entries (budget between largest entry and summed total; verified red against a per-entry atom). Include the entry name in streaming-guard errors, count skipped bytes against the budget with a direct unit test, and forward all four limit keys in get-manifest. No behavior change. AI-assisted-by: muse-spark-1.3-contributor * :recycle: Thread cfg through get-manifest get-manifest now takes the caller cfg and resolves limits with setup-limits like the import job itself, instead of building a single-use mini-cfg from cf/get. No behavior change. AI-assisted-by: muse-spark-1.3-contributor --- backend/src/app/binfile/common.clj | 28 ++- backend/src/app/binfile/v1.clj | 4 +- backend/src/app/binfile/v3.clj | 163 ++++++++----- backend/src/app/config.clj | 10 +- backend/src/app/http/debug.clj | 8 +- backend/src/app/rpc/commands/binfile.clj | 6 +- backend/src/app/rpc/commands/management.clj | 4 +- backend/test/backend_tests/binfile_test.clj | 239 ++++++++++++++++++-- 8 files changed, 376 insertions(+), 86 deletions(-) diff --git a/backend/src/app/binfile/common.clj b/backend/src/app/binfile/common.clj index 16652c3526..701112a51c 100644 --- a/backend/src/app/binfile/common.clj +++ b/backend/src/app/binfile/common.clj @@ -50,10 +50,34 @@ (def temp-file-threshold (* 1024 1024 2)) -;; A maximum (storage) object size allowed: 100MiB -(def ^:const max-object-size +;; Maximum size allowed for a single binary entry during binfile +;; import: 100MiB. Covers the storage blobs (`objects/` entries in v3, +;; streams in v1), whose declared size and hash are verified against the +;; imported bytes. Legitimate media objects fit comfortably below this; +;; anything larger is rejected instead of being buffered into memory. +(def ^:const default-max-binary-entry-size (* 1024 1024 100)) +;; Maximum decompressed size allowed for a single JSON/text zip entry +;; (manifest, files, pages, shapes, colors, components, typographies, +;; tokens, plugin-data) during binfile import: 20MiB. Legitimate entries +;; are KB-sized, so this is deliberately much lower than +;; default-max-binary-entry-size and bounds the DEFLATE amplification of any +;; single entry. +(def ^:const default-max-text-entry-size + (* 1024 1024 20)) + +;; Maximum total decompressed size allowed for all JSON/text zip entries +;; combined within a single import job: 200MiB. Bounds the case where many +;; entries, each individually under default-max-text-entry-size, still sum +;; to an unreasonable total. +(def ^:const default-max-text-total-size + (* 1024 1024 200)) + +;; Maximum number of entries allowed in the import zip: 500,000. +(def ^:const default-max-zip-entries + (* 500 1000)) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; (declare get-resolved-file-libraries) diff --git a/backend/src/app/binfile/v1.clj b/backend/src/app/binfile/v1.clj index 0f00a7564e..23d7b8795c 100644 --- a/backend/src/app/binfile/v1.clj +++ b/backend/src/app/binfile/v1.clj @@ -174,7 +174,7 @@ (assert-mark m :obj) (let [size (read-long! input)] (assert (pos? size) "incorrect header size found on reading header") - (when (> size bfc/max-object-size) + (when (> size bfc/default-max-binary-entry-size) (ex/raise :type :validation :code :max-file-size-reached :hint (dm/str "unable to import object with size " size " bytes"))) @@ -249,7 +249,7 @@ p (tmp/tempfile :prefix "penpot.binfile.")] (assert-mark m :stream) - (when (> s bfc/max-object-size) + (when (> s bfc/default-max-binary-entry-size) (ex/raise :type :validation :code :max-file-size-reached :hint (str/ffmt "unable to import storage object with size % bytes" s))) diff --git a/backend/src/app/binfile/v3.clj b/backend/src/app/binfile/v3.clj index 9d4801dd4a..7f370ccac9 100644 --- a/backend/src/app/binfile/v3.clj +++ b/backend/src/app/binfile/v3.clj @@ -464,16 +464,21 @@ (defn- size-limiting-stream "Wraps an InputStream to enforce a maximum number of decompressed bytes. - Raises :validation :max-file-size-reached when the limit is exceeded." + Raises :validation :max-file-size-reached when the limit is exceeded. + `counter` holds the bytes accounted so far: pass a fresh atom for a + per-entry cap, or the shared job-wide atom for the cumulative budget. + `entry-name` identifies the entry being read and is included in the + raised ex-data for triage." ^InputStream - [^InputStream input ^long max-size] - (let [counter (atom 0) - on-read (fn [n] + [^InputStream input ^long max-size counter entry-name] + (let [on-read (fn [n] (when (pos? n) (when (> (swap! counter + (long n)) max-size) (ex/raise :type :validation :code :max-file-size-reached - :hint (str "stream exceeded max size: " max-size)))) + :hint (str "stream exceeded max size on entry " entry-name ": " max-size) + :path entry-name + :max max-size))) n)] (proxy [FilterInputStream] [input] (read @@ -486,12 +491,52 @@ ([^bytes buf off] (on-read (.read input buf (int off) (- (alength buf) (int off))))) ([^bytes buf off len] - (on-read (.read input buf (int off) (int len)))))))) + (on-read (.read input buf (int off) (int len))))) + (skip [n] + ;; Skipped bytes were already decompressed, so they count against + ;; the budget like read bytes do. + (let [skipped (.skip input (long n))] + (when (pos? skipped) + (on-read skipped)) + skipped))))) + +(defn- setup-limits + "Resolve the binfile import limits once per job from cfg, falling back + to the namespace defaults when the keys are absent. Returns cfg with + `::max-text-entry-size` (per JSON/text entry cap), `::max-text-total-size` (cumulative + JSON/text budget), `::accumulated-total-text-size` (shared atom holding the cumulative + bytes read so far), `::max-binary-entry-size` (storage blob cap) and + `::max-zip-entries` (zip entry count cap)." + [cfg] + (assoc cfg + ::max-text-entry-size (or (::bfc/import-max-text-entry-size cfg) bfc/default-max-text-entry-size) + ::max-text-total-size (or (::bfc/import-max-text-total-size cfg) bfc/default-max-text-total-size) + ::accumulated-total-text-size (atom 0) + ::max-binary-entry-size (or (::bfc/import-max-binary-entry-size cfg) bfc/default-max-binary-entry-size) + ::max-zip-entries (or (::bfc/import-max-zip-entries cfg) bfc/default-max-zip-entries))) (defn- zip-entry-reader - [^ZipFile input ^ZipEntry entry] - (-> (zip-entry-stream input entry) - (io/reader :encoding "UTF-8"))) + "Opens a UTF-8 reader over a zip entry, enforcing the per-entry + decompressed-size cap on the actual bytes streamed through it and + accounting the same bytes against the shared job-wide budget. + A cheap pre-check on the entry-declared size rejects obvious bombs without + opening the stream; the streaming counters remain the authoritative guard + because the declared size is attacker-controlled metadata." + [cfg ^ZipFile input ^ZipEntry entry] + (let [entry-name (zip-entry-name entry) + declared (get-zip-entry-size entry) + max-size (::max-text-entry-size cfg)] + (when (and (not (neg? declared)) (> declared (long max-size))) + (ex/raise :type :validation + :code :max-file-size-reached + :hint (str "zip entry exceeds maximum size: " entry-name) + :path entry-name + :max max-size + :found declared)) + (-> (zip-entry-stream input entry) + (size-limiting-stream max-size (atom 0) entry-name) + (size-limiting-stream (::max-text-total-size cfg) (::accumulated-total-text-size cfg) entry-name) + (io/reader :encoding "UTF-8")))) (defn- zip-entry-storage-content "Wraps a ZipFile and ZipEntry into a penpot storage compatible @@ -499,7 +544,7 @@ [input entry & {:keys [max-size]}] (let [stream-fn (fn [] (cond-> (zip-entry-stream input entry) - max-size (size-limiting-stream max-size))) + max-size (size-limiting-stream max-size (atom 0) (zip-entry-name entry)))) hash (delay (->> (stream-fn) (sto.impl/calculate-hash)))] (reify @@ -523,9 +568,9 @@ (throw (UnsupportedOperationException. "not implemented")))))) (defn- read-manifest - [^ZipFile input] + [cfg ^ZipFile input] (let [entry (get-zip-entry input "manifest.json")] - (with-open [^AutoCloseable reader (zip-entry-reader input entry)] + (with-open [^AutoCloseable reader (zip-entry-reader cfg input entry)] (let [manifest (json/read reader :key-fn json/read-kebab-key)] (decode-manifest manifest))))) @@ -614,20 +659,17 @@ :id (parse-uuid id)})))) (defn- read-entry - [^ZipFile input entry] - (with-open [^AutoCloseable reader (zip-entry-reader input entry)] - (json/read reader :key-fn json/read-kebab-key))) - -(defn- read-plain-entry - [^ZipFile input entry] - (with-open [^AutoCloseable reader (zip-entry-reader input entry)] - (json/read reader))) + ([cfg ^ZipFile input entry] + (read-entry cfg input entry json/read-kebab-key)) + ([cfg ^ZipFile input entry key-fn] + (with-open [^AutoCloseable reader (zip-entry-reader cfg input entry)] + (json/read reader :key-fn key-fn)))) (defn- read-file - [{:keys [::bfc/input ::bfc/timestamp]} file-id] + [{:keys [::bfc/input ::bfc/timestamp] :as cfg} file-id] (let [path (str "files/" file-id ".json") entry (get-zip-entry input path)] - (-> (read-entry input entry) + (-> (read-entry cfg input entry) (decode-file) (update :revn d/nilv 1) (update :created-at d/nilv timestamp) @@ -635,19 +677,19 @@ (validate-file)))) (defn- read-file-plugin-data - [{:keys [::bfc/input]} file-id] + [{:keys [::bfc/input] :as cfg} file-id] (let [path (str "files/" file-id "/plugin-data.json") entry (get-zip-entry* input path)] - (some->> entry - (read-entry input) - (decode-plugin-data) - (validate-plugin-data)))) + (when entry + (-> (read-entry cfg input entry) + (decode-plugin-data) + (validate-plugin-data))))) (defn- read-file-media - [{:keys [::bfc/input ::entries]} file-id] + [{:keys [::bfc/input ::entries] :as cfg} file-id] (->> (keep (match-media-entry-fn file-id) entries) (reduce (fn [result {:keys [id entry]}] - (let [object (->> (read-entry input entry) + (let [object (->> (read-entry cfg input entry) (decode-media) (validate-media)) object (-> object @@ -664,10 +706,10 @@ (not-empty))) (defn- read-file-colors - [{:keys [::bfc/input ::entries]} file-id] + [{:keys [::bfc/input ::entries] :as cfg} file-id] (->> (keep (match-color-entry-fn file-id) entries) (reduce (fn [result {:keys [id entry]}] - (let [object (->> (read-entry input entry) + (let [object (->> (read-entry cfg input entry) (decode-color) (validate-color))] (events/tap :progress {:section :color :id id :file-id file-id}) @@ -678,7 +720,7 @@ (not-empty))) (defn- read-file-components - [{:keys [::bfc/input ::entries]} file-id] + [{:keys [::bfc/input ::entries] :as cfg} file-id] (let [clean-component-post-decode (fn [component] (d/update-when component :objects @@ -698,7 +740,7 @@ (->> (keep (match-component-entry-fn file-id) entries) (reduce (fn [result {:keys [id entry]}] - (let [object (->> (read-entry input entry) + (let [object (->> (read-entry cfg input entry) (clean-component-pre-decode) (decode-component) (clean-component-post-decode))] @@ -710,10 +752,10 @@ (not-empty)))) (defn- read-file-typographies - [{:keys [::bfc/input ::entries]} file-id] + [{:keys [::bfc/input ::entries] :as cfg} file-id] (->> (keep (match-typography-entry-fn file-id) entries) (reduce (fn [result {:keys [id entry]}] - (let [object (->> (read-entry input entry) + (let [object (->> (read-entry cfg input entry) (decode-typography) (validate-typography))] (events/tap :progress {:section :typography :id id :file-id file-id}) @@ -724,10 +766,10 @@ (not-empty))) (defn- read-file-tokens-lib - [{:keys [::bfc/input ::entries]} file-id] + [{:keys [::bfc/input ::entries] :as cfg} file-id] (when-let [entry (d/seek (match-tokens-lib-entry-fn file-id) entries)] (events/tap :progress {:section :tokens-lib :file-id file-id}) - (->> (read-plain-entry input entry) + (->> (read-entry cfg input entry nil) (decode-tokens-lib) (validate-tokens-lib)))) @@ -735,7 +777,7 @@ [{:keys [::bfc/input ::entries] :as cfg} file-id page-id] (->> (keep (match-shape-entry-fn file-id page-id) entries) (reduce (fn [result {:keys [id entry]}] - (let [object (->> (read-entry input entry) + (let [object (->> (read-entry cfg input entry) (bfl/clean-shape-pre-decode) (decode-shape) (bfl/clean-shape-post-decode))] @@ -749,7 +791,7 @@ [{:keys [::bfc/input ::entries] :as cfg} file-id] (->> (keep (match-page-entry-fn file-id) entries) (keep (fn [{:keys [id entry]}] - (let [page (->> (read-entry input entry) + (let [page (->> (read-entry cfg input entry) (decode-page)) page (dissoc page :options)] (events/tap :progress {:section :page :id id :file-id file-id}) @@ -765,7 +807,7 @@ [{:keys [::bfc/input ::entries] :as cfg} file-id] (->> (keep (match-thumbnail-entry-fn file-id) entries) (reduce (fn [result {:keys [page-id frame-id tag entry]}] - (let [object (->> (read-entry input entry) + (let [object (->> (read-entry cfg input entry) (decode-file-thumbnail) (validate-file-thumbnail))] @@ -906,7 +948,7 @@ entries (keep (match-storage-entry-fn) entries)] (doseq [{:keys [id entry]} entries] - (let [object (-> (read-entry input entry) + (let [object (-> (read-entry cfg input entry) (decode-storage-object) (update :bucket d/nilv sto/default-bucket) (validate-storage-object)) @@ -915,7 +957,7 @@ path (str "objects/" id ext) content (zip-entry-storage-content input (get-zip-entry input path) - :max-size (::bfc/import-max-object-size cfg))] + :max-size (::max-binary-entry-size cfg))] (when (not= (:size object) (sto/get-size content)) (ex/raise :type :validation @@ -925,7 +967,7 @@ :expected-size (:size object) :found-size (sto/get-size content))) - (when-let [max (::bfc/import-max-object-size cfg)] + (let [max (::max-binary-entry-size cfg)] (when (> (sto/get-size content) max) (ex/raise :type :validation :code :max-file-size-reached @@ -1152,17 +1194,22 @@ (assert (instance? ZipFile input) "expected zip file") (assert (ct/inst? timestamp) "expected valid instant") - (let [manifest (-> (read-manifest input) + ;; Resolve all import limits once per job (see `setup-limits`); every + ;; bounded read below accounts its actual decompressed bytes against the + ;; shared job-wide budget, so many entries each under the per-entry cap + ;; cannot sum to an unreasonable total. + (let [cfg (setup-limits cfg) + manifest (-> (read-manifest cfg input) (validate-manifest)) entries (read-zip-entries input) - _ (when-let [max (::bfc/import-max-zip-entries cfg)] - (when (> (count entries) max) - (ex/raise :type :validation - :code :too-many-zip-entries - :hint (str "zip file has too many entries: " (count entries)) - :max max - :found (count entries)))) + max (::max-zip-entries cfg) + _ (when (> (count entries) max) + (ex/raise :type :validation + :code :too-many-zip-entries + :hint (str "zip file has too many entries: " (count entries)) + :max max + :found (count entries))) cfg (-> cfg (assoc ::entries entries) @@ -1284,7 +1331,13 @@ :error? (some? @cs)))))) (defn get-manifest - [path] - (with-open [^AutoCloseable input (ZipFile. ^File (fs/file path))] - (-> (read-manifest input) - (validate-manifest)))) + "Reads and validates the manifest of a `.penpot` file at `path`. + Runs synchronously on the RPC request thread (before the background + import job exists). Limits are resolved from `cfg` like in the import + job itself, so the read is bounded by the same decompressed-size + limits." + [cfg path] + (let [cfg (setup-limits cfg)] + (with-open [^AutoCloseable input (ZipFile. ^File (fs/file path))] + (-> (read-manifest cfg input) + (validate-manifest))))) diff --git a/backend/src/app/config.clj b/backend/src/app/config.clj index 91155de5e9..14de96d158 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -96,11 +96,7 @@ ;; SSRF protection :ssrf-allowed-hosts #{} - :ssrf-extra-blocked-cidrs #{} - - ;; Binfile import limits - :binfile-import-max-object-size (* 1024 1024 100) ;; 100 MiB - :binfile-import-max-zip-entries (* 500 1000)}) ;; 500,000 + :ssrf-extra-blocked-cidrs #{}}) (def schema:config (do #_sm/optional-keys @@ -158,7 +154,9 @@ [:media-processing-service-timeout {:optional true} ::sm/int] ;; Binfile import limits (PENPOT_BINFILE_IMPORT_*) - [:binfile-import-max-object-size {:optional true} ::sm/int] + [:binfile-import-max-binary-entry-size {:optional true} ::sm/int] + [:binfile-import-max-text-entry-size {:optional true} ::sm/int] + [:binfile-import-max-text-total-size {:optional true} ::sm/int] [:binfile-import-max-zip-entries {:optional true} ::sm/int] [:deletion-delay {:optional true} ::ct/duration] diff --git a/backend/src/app/http/debug.clj b/backend/src/app/http/debug.clj index edcef0bb50..0a4929b2ec 100644 --- a/backend/src/app/http/debug.clj +++ b/backend/src/app/http/debug.clj @@ -375,7 +375,9 @@ ::bfc/project-id project-id ::bfc/team-id (:id team) ::bfc/input path - ::bfc/import-max-object-size (cf/get :binfile-import-max-object-size) + ::bfc/import-max-binary-entry-size (cf/get :binfile-import-max-binary-entry-size) + ::bfc/import-max-text-entry-size (cf/get :binfile-import-max-text-entry-size) + ::bfc/import-max-text-total-size (cf/get :binfile-import-max-text-total-size) ::bfc/import-max-zip-entries (cf/get :binfile-import-max-zip-entries))] (bf.v3/import-files! cfg) {::yres/status 200 @@ -634,7 +636,9 @@ ::bfc/input path ::bfc/team-id (:id team) ::bfc/features (cfeat/get-team-enabled-features cf/flags team) - ::bfc/import-max-object-size (cf/get :binfile-import-max-object-size) + ::bfc/import-max-binary-entry-size (cf/get :binfile-import-max-binary-entry-size) + ::bfc/import-max-text-entry-size (cf/get :binfile-import-max-text-entry-size) + ::bfc/import-max-text-total-size (cf/get :binfile-import-max-text-total-size) ::bfc/import-max-zip-entries (cf/get :binfile-import-max-zip-entries))] (if (= format :binfile-v3) diff --git a/backend/src/app/rpc/commands/binfile.clj b/backend/src/app/rpc/commands/binfile.clj index b9bff3cbd7..de0d3faadd 100644 --- a/backend/src/app/rpc/commands/binfile.clj +++ b/backend/src/app/rpc/commands/binfile.clj @@ -104,7 +104,9 @@ (assoc ::bfc/profile-id profile-id) (assoc ::bfc/team-id (:id team)) (assoc ::bfc/name name) - (assoc ::bfc/import-max-object-size (cf/get :binfile-import-max-object-size)) + (assoc ::bfc/import-max-binary-entry-size (cf/get :binfile-import-max-binary-entry-size)) + (assoc ::bfc/import-max-text-entry-size (cf/get :binfile-import-max-text-entry-size)) + (assoc ::bfc/import-max-text-total-size (cf/get :binfile-import-max-text-total-size)) (assoc ::bfc/import-max-zip-entries (cf/get :binfile-import-max-zip-entries))) input-path (:path file) @@ -181,7 +183,7 @@ manifest (case (int version) 1 nil - 3 (bf.v3/get-manifest (-> params :file :path)) + 3 (bf.v3/get-manifest cfg (-> params :file :path)) (throw (ex-info (str "Unsupported binfile version: " version) {:type :validation :code :unsupported-version diff --git a/backend/src/app/rpc/commands/management.clj b/backend/src/app/rpc/commands/management.clj index ecb805d4a3..62d5a88330 100644 --- a/backend/src/app/rpc/commands/management.clj +++ b/backend/src/app/rpc/commands/management.clj @@ -428,7 +428,9 @@ (assoc ::bfc/team-id (:id team)) (assoc ::bfc/input template) (assoc ::bfc/features (cfeat/get-team-enabled-features cf/flags team)) - (assoc ::bfc/import-max-object-size (cf/get :binfile-import-max-object-size)) + (assoc ::bfc/import-max-binary-entry-size (cf/get :binfile-import-max-binary-entry-size)) + (assoc ::bfc/import-max-text-entry-size (cf/get :binfile-import-max-text-entry-size)) + (assoc ::bfc/import-max-text-total-size (cf/get :binfile-import-max-text-total-size)) (assoc ::bfc/import-max-zip-entries (cf/get :binfile-import-max-zip-entries))) result (if (= format :binfile-v3) diff --git a/backend/test/backend_tests/binfile_test.clj b/backend/test/backend_tests/binfile_test.clj index 1f1caa6182..d0f86fd861 100644 --- a/backend/test/backend_tests/binfile_test.clj +++ b/backend/test/backend_tests/binfile_test.clj @@ -34,7 +34,13 @@ [datoteka.io :as io]) (:import java.io.ByteArrayInputStream - java.io.DataInputStream)) + java.io.DataInputStream + java.io.OutputStreamWriter + java.io.Writer + java.util.zip.Deflater + java.util.zip.ZipEntry + java.util.zip.ZipFile + java.util.zip.ZipOutputStream)) (t/use-fixtures :once th/state-init) (t/use-fixtures :each th/database-reset) @@ -248,9 +254,9 @@ (t/is (= "penpot" (get-in imported [:metadata :referer])))))) (t/deftest read-obj-rejects-oversized-buffer - ;; N1-07: read-obj! must reject objects exceeding max-object-size + ;; N1-07: read-obj! must reject objects exceeding default-max-binary-entry-size ;; before attempting to allocate the buffer - (let [size (+ bfc/max-object-size 1) + (let [size (+ bfc/default-max-binary-entry-size 1) baos (java.io.ByteArrayOutputStream. 17) dos (java.io.DataOutputStream. baos)] (.writeByte dos 5) @@ -302,7 +308,7 @@ (io/output-stream output)) ;; Read the manifest and check external-libraries - (let [manifest (v3/get-manifest output)] + (let [manifest (v3/get-manifest th/*system* output)] (t/is (some? (:external-libraries manifest))) (t/is (= 1 (count (:external-libraries manifest)))) (let [ext-lib (first (:external-libraries manifest))] @@ -704,7 +710,7 @@ (io/output-stream output)) ;; Verify manifest has external-libraries - (let [manifest (v3/get-manifest output) + (let [manifest (v3/get-manifest th/*system* output) ext-libs (:external-libraries manifest)] (t/is (some? ext-libs)) (t/is (pos? (count ext-libs)))) @@ -1455,7 +1461,7 @@ (io/output-stream output)) ;; Verify slug in manifest - (let [manifest (v3/get-manifest output) + (let [manifest (v3/get-manifest th/*system* output) ext-lib (first (:external-libraries manifest))] (t/is (= "icons-buttons" (:slug ext-lib))))))) @@ -1482,7 +1488,7 @@ (io/output-stream output)) ;; Library with empty slug should be dropped from external-libraries - (let [manifest (v3/get-manifest output) + (let [manifest (v3/get-manifest th/*system* output) ext-libs (:external-libraries manifest)] (t/is (or (nil? ext-libs) (empty? ext-libs))))))) @@ -1502,7 +1508,7 @@ (io/output-stream output)) ;; Manifest should have no external-libraries - (let [manifest (v3/get-manifest output)] + (let [manifest (v3/get-manifest th/*system* output)] (t/is (nil? (:external-libraries manifest)))) ;; Import should succeed with empty resolution @@ -1620,7 +1626,7 @@ new-file-id (first (:file-ids result))] ;; Get the original library id from the manifest - (let [manifest (v3/get-manifest output) + (let [manifest (v3/get-manifest th/*system* output) original-lib-id (:id (first (:external-libraries manifest)))] ;; Get the imported file's data (let [imported (:result (th/command! {::th/type :get-file @@ -1794,7 +1800,7 @@ ;; Refs must remain as original UUID (dangling), NOT remapped to any ;; of the candidate libraries - (let [manifest (v3/get-manifest output) + (let [manifest (v3/get-manifest th/*system* output) original-lib-id (:id (first (:external-libraries manifest))) imported (:result (th/command! {::th/type :get-file ::rpc/profile-id (:id profile) @@ -1850,7 +1856,7 @@ "viewer should NOT auto-link") ;; Refs must remain as original UUID (dangling) - (let [manifest (v3/get-manifest output) + (let [manifest (v3/get-manifest th/*system* output) original-lib-id (:id (first (:external-libraries manifest))) imported (:result (th/command! {::th/type :get-file ::rpc/profile-id (:id viewer) @@ -1879,14 +1885,14 @@ (io/output-stream output)) ;; Verify manifest has external-libraries (only produced by link-later) - (let [manifest (v3/get-manifest output) + (let [manifest (v3/get-manifest th/*system* output) ext-libs (:external-libraries manifest)] (t/is (some? ext-libs) "type :link-later should produce external-libraries even with include-libraries=true") (t/is (pos? (count ext-libs)))))) (t/deftest import-rejects-too-many-zip-entries - ;; import must reject ZIP files exceeding max-zip-entries + ;; import must reject ZIP files exceeding max-zip-entries (default-max-zip-entries) (let [profile (th/create-profile* 1) file (prepare-simple-file profile) output (tmp/tempfile :suffix ".zip")] @@ -1942,7 +1948,7 @@ (dissoc file :data))) (t/deftest import-rejects-oversized-object - ;; import must reject storage objects exceeding max-object-size + ;; import must reject storage objects exceeding default-max-binary-entry-size (let [profile (th/create-profile* 1) file (prepare-file-with-media profile) output (tmp/tempfile :suffix ".zip")] @@ -1953,12 +1959,12 @@ (assoc ::bfc/export-type :detach-libraries)) (io/output-stream output)) - ;; Import with max-object-size=1 — the media object will exceed this + ;; Import with max-binary-entry-size=1 — the media object will exceed this (let [cfg (-> th/*system* (assoc ::bfc/project-id (:default-project-id profile)) (assoc ::bfc/profile-id (:id profile)) (assoc ::bfc/input output) - (assoc ::bfc/import-max-object-size 1)) + (assoc ::bfc/import-max-binary-entry-size 1)) out (try (v3/import-files! cfg) :no-error @@ -1967,3 +1973,204 @@ d)))] (t/is (= :validation (:type out))) (t/is (= :max-file-size-reached (:code out)))))) + +;; --- GHSA-qcw7-v626-g6cf: decompression-bomb guards on JSON/text entries + +(def ^:private bomb-entry-size + "Decompressed size of the test bomb entries. Over the 20 MiB default + per-entry limit, small enough to stay fast and lean under the guard." + (* 1024 1024 25)) + +(defn- write-bomb-entry! + "Writes a zip entry whose content is a single JSON string of `size` bytes + made of a repeated char. Streams in chunks, so neither the writer nor the + (guarded) reader ever needs to hold the full payload in memory. Compresses + ~1:1000, like the reported exploit." + [^ZipOutputStream zos ^String entry-name ^long size] + (.putNextEntry zos (ZipEntry. entry-name)) + (let [w (OutputStreamWriter. zos "UTF-8") + chunk (apply str (repeat 8192 \A))] + (.write w "\"") + (loop [remaining size] + (when (pos? remaining) + (let [n (min remaining (count chunk))] + (.write ^Writer w ^String chunk (int 0) (int n)) + (recur (- remaining n))))) + (.write w "\"") + (.flush w)) + (.closeEntry zos)) + +(defn- replace-zip-entry! + "Copies the zip at `src-path` to `dst-path`, replacing the entry + `entry-name` with a bomb entry of `bomb-size` decompressed bytes." + [src-path dst-path entry-name bomb-size] + (with-open [zin (ZipFile. (fs/file src-path)) + out (io/output-stream dst-path) + zos (ZipOutputStream. out)] + (.setLevel zos Deflater/BEST_COMPRESSION) + (doseq [entry (iterator-seq (.entries zin))] + (let [entry-name' (.getName ^ZipEntry entry)] + (if (= entry-name' entry-name) + (write-bomb-entry! zos entry-name bomb-size) + (do + (.putNextEntry zos (ZipEntry. entry-name')) + (with-open [in (.getInputStream zin entry)] + (io/copy in zos)) + (.closeEntry zos))))))) + +(defn- try-import-files! + "Runs v3/import-files! and returns the ex-data of the raised error, + or :no-error when the import unexpectedly succeeds." + [cfg] + (try + (v3/import-files! cfg) + :no-error + (catch Throwable e + (or (ex-data e) (some-> (ex-cause e) ex-data))))) + +(t/deftest import-rejects-oversized-json-entry + ;; GHSA-qcw7-v626-g6cf: a single files/.json entry expanding beyond + ;; the per-entry text limit must be rejected with :max-file-size-reached + ;; instead of exhausting the heap. The manifest comes from a real export + ;; so it is valid; only the file entry is replaced by the bomb. + (let [profile (th/create-profile* 1) + file (prepare-simple-file profile) + exported (tmp/tempfile :suffix ".zip")] + + (v3/export-files! + (-> th/*system* + (assoc ::bfc/ids #{(:id file)}) + (assoc ::bfc/embed-assets false) + (assoc ::bfc/include-libraries false)) + (io/output-stream exported)) + + (let [bombed (tmp/tempfile :suffix ".zip")] + (replace-zip-entry! exported bombed + (str "files/" (:id file) ".json") + bomb-entry-size) + (let [cfg (-> th/*system* + (assoc ::bfc/project-id (:default-project-id profile)) + (assoc ::bfc/profile-id (:id profile)) + (assoc ::bfc/input bombed)) + out (try-import-files! cfg)] + (t/is (= :validation (:type out))) + (t/is (= :max-file-size-reached (:code out))))))) + +(t/deftest get-manifest-rejects-oversized-manifest + ;; GHSA-qcw7-v626-g6cf: the synchronous manifest read on the RPC thread + ;; (v3/get-manifest) must reject a manifest.json bomb the same way. + ;; Decoding/validation is never reached, so the payload needs no schema. + (let [bombed (tmp/tempfile :suffix ".zip")] + (with-open [out (io/output-stream bombed) + zos (ZipOutputStream. out)] + (.setLevel zos Deflater/BEST_COMPRESSION) + (write-bomb-entry! zos "manifest.json" bomb-entry-size)) + (let [out (try + (v3/get-manifest th/*system* bombed) + :no-error + (catch Throwable e + (or (ex-data e) (some-> (ex-cause e) ex-data))))] + (t/is (= :validation (:type out))) + (t/is (= :max-file-size-reached (:code out)))))) + +(t/deftest import-rejects-excessive-total-text-size + ;; The job-wide cumulative budget must reject an import whose entries, + ;; each individually under the per-entry cap, exceed the total limit. + ;; A tiny total budget over a legitimate small export proves the + ;; cumulative counter fires independently of the per-entry guard. + (let [profile (th/create-profile* 1) + file (prepare-simple-file profile) + output (tmp/tempfile :suffix ".zip")] + + (v3/export-files! + (-> th/*system* + (assoc ::bfc/ids #{(:id file)}) + (assoc ::bfc/embed-assets false) + (assoc ::bfc/include-libraries false)) + (io/output-stream output)) + + (let [cfg (-> th/*system* + (assoc ::bfc/project-id (:default-project-id profile)) + (assoc ::bfc/profile-id (:id profile)) + (assoc ::bfc/input output) + (assoc ::bfc/import-max-text-total-size 100)) + out (try-import-files! cfg)] + (t/is (= :validation (:type out))) + (t/is (= :max-file-size-reached (:code out)))))) + +(defn- text-entries-sizes + "Returns the decompressed sizes of every `.json` entry in the zip at + `zip-path`. Used to pick a cumulative budget that sits between the + largest single entry and the summed total." + [zip-path] + (with-open [zin (ZipFile. (fs/file zip-path))] + (->> (iterator-seq (.entries zin)) + (filter #(.endsWith ^String (.getName ^ZipEntry %) ".json")) + (map #(.getSize ^ZipEntry %)) + (remove neg?) + vec))) + +(t/deftest import-rejects-accumulated-text-across-entries + ;; The cumulative budget must account bytes across entries sharing one + ;; counter: a budget just over the largest single entry (so no entry + ;; alone can trip it) but under the summed total (so the running total + ;; must trip it) is rejected. If the shared counter ever regressed to + ;; a fresh atom per entry, every entry alone would pass and the valid + ;; import would succeed, so this test would go red. + (let [profile (th/create-profile* 1) + file (prepare-simple-file profile) + exported (tmp/tempfile :suffix ".zip")] + + (v3/export-files! + (-> th/*system* + (assoc ::bfc/ids #{(:id file)}) + (assoc ::bfc/embed-assets false) + (assoc ::bfc/include-libraries false)) + (io/output-stream exported)) + + (let [sizes (text-entries-sizes exported) + max-single (apply max 0 sizes) + summed (reduce + 0 sizes) + budget (inc max-single)] + ;; Preconditions that make the test meaningful: more than one + ;; entry worth of text, so the trip can only come from + ;; accumulation, never from a single entry. + (t/is (> summed budget)) + (let [cfg (-> th/*system* + (assoc ::bfc/project-id (:default-project-id profile)) + (assoc ::bfc/profile-id (:id profile)) + (assoc ::bfc/input exported) + (assoc ::bfc/import-max-text-total-size budget)) + out (try-import-files! cfg)] + (t/is (= :validation (:type out))) + (t/is (= :max-file-size-reached (:code out))) + (t/is (some? (:path out))))))) + +(t/deftest size-limiting-stream-counts-skip + ;; Skipped bytes were already decompressed, so they must count against + ;; the budget like read bytes do. + (let [payload (.getBytes "abcdefghijklmnopqrstuvwxyz" "UTF-8") + mk-stream (fn [cap] + (@#'v3/size-limiting-stream + (ByteArrayInputStream. payload) cap (atom 0) "test-entry"))] + ;; Skipping past the cap trips the guard. + (let [out (try + (with-open [s (mk-stream 10)] + (.skip ^java.io.InputStream s 20) + :no-error) + (catch clojure.lang.ExceptionInfo e + (ex-data e)))] + (t/is (= :validation (:type out))) + (t/is (= :max-file-size-reached (:code out))) + (t/is (= "test-entry" (:path out)))) + ;; Skipping under the cap leaves the remainder accounted: 6 skipped + ;; plus 10 read over a cap of 10 trips the guard. + (let [out (try + (with-open [s (mk-stream 10)] + (.skip ^java.io.InputStream s 6) + (.read ^java.io.InputStream s (byte-array 10)) + :no-error) + (catch clojure.lang.ExceptionInfo e + (ex-data e)))] + (t/is (= :validation (:type out))) + (t/is (= :max-file-size-reached (:code out))))))