mirror of
https://github.com/penpot/penpot.git
synced 2026-09-23 20:36:15 +00:00
🐛 Bound decompressed size of JSON entries on binfile v3 import (#11607)
* 🐛 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 * ♻️ 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 * ♻️ 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 * ♻️ 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 * ♻️ 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 * ♻️ 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
This commit is contained in:
Andrey Antukh
GitHub
parent
2041473cc4
commit
38212e757c
@ -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)
|
||||
|
||||
@ -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)))
|
||||
|
||||
@ -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)))))
|
||||
|
||||
@ -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]
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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/<id>.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))))))
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user