From 4874da450f1e1f055e44109ef451ef0452f0bcea Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 10 Nov 2026 15:52:28 +0100 Subject: [PATCH] :bug: Escape SVG values and sanitize every SVG upload (#12887) * :bug: Escape SVG values and sanitize every SVG upload The SVG sanitizer removed dangerous elements and attributes by name and then wrote the tree back with clojure.xml/emit, which escapes nothing. A payload hidden in an attribute value or in a text node became live markup again: an id of "x' onload='alert(1)'" was stored as a real onload handler, and an entity encoded script tag was stored as a real script element. Write the filtered tree with a serializer that escapes text and attribute values, and route every path that stores an uploaded SVG through it. The thumbnail writers (create-file-object-thumbnail and create-file-thumbnail) and the organization logo stored the upload untouched, so they could plant live markup in a public bucket. The filter only knows about SVG elements, so it also accepted documents that are not SVG at all: a page with an iframe or a meta refresh was stored as an image. The sanitizer now rejects any document whose root is not an svg element in the SVG namespace, and it compares the scheme of an url the way the browser does, so a scheme split with a tab or a line break no longer slips through the javascript: filter. The attributes are written sorted by name, because the parser hands them over in the reverse order of the document and the output changed on every sanitization. The profile picture of an identity provider is not stored either when it is an SVG: the avatar is always rendered as an image, so an SVG would only add the risk of keeping live markup in the storage. The dimensions of an SVG are now read from its local name, so an SVG with a namespace prefix can be uploaded like any other. The logo had no size limit and the sanitizer reads the whole file, so it now honors the media max file size. The output shape changes with it: no line breaks, double quotes, and the single quote of an attribute value escaped as an entity. The hash of every stored SVG object changes with it. The media tests use cuerdas.core instead of clojure.string, as the rest of the codebase does. Relates to GHSA-qwhj-rchj-58mg AI-assisted-by: minimax-m3.1-flash * :bug: Drop non-SVG namespace subtrees in the SVG sanitizer The sanitizer only checked the namespace of the root, so a child could rebind xmlns to XHTML and smuggle live HTML (iframe/srcdoc/meta) past the blocklist. The serializer escapes attribute values, but the browser decodes them again, so srcdoc stays live. Track the namespace bindings through the recursion and drop any element that is not in the SVG namespace, with its subtree. The root is validated up front with the same model, so a document that is not an svg document is rejected with an honest hint instead of a misleading one. The default namespace is stored under a namespaced key, because a prefix literally named `default` collided with the internal key and re-opened the same hole through a prefixed binding. The attribute filter now keeps `xmlns`/`xmlns:*` declarations, which are not event handlers, so a prefix named like one does not end up undeclared in the output. The blocklist also drops HTML-only elements written in the svg namespace (iframe, meta, embed, link, base). Add regression tests for the child rebinding, for the prefixed variants, for the `default` prefix, for the root local name, for the create-file-media-object-from-url path (mocked download), and for the HTML-only tags. Assert double-quoted attributes in the clean content test. Relates to GHSA-qwhj-rchj-58mg AI-assisted-by: longcat-2.5-free --- .serena/memories/backend/subtleties.md | 1 + backend/src/app/media/svg.clj | 233 +++++++++-- backend/src/app/rpc/commands/auth.clj | 27 +- .../src/app/rpc/commands/files_thumbnails.clj | 6 +- backend/src/app/rpc/commands/media.clj | 10 +- backend/src/app/rpc/management/nitrate.clj | 9 +- backend/test/backend_tests/helpers.clj | 15 + backend/test/backend_tests/media_test.clj | 394 +++++++++++++++--- backend/test/backend_tests/rpc_auth_test.clj | 32 ++ .../rpc_file_thumbnails_test.clj | 69 +++ .../rpc_management_nitrate_test.clj | 43 ++ backend/test/backend_tests/rpc_media_test.clj | 127 ++++++ 12 files changed, 857 insertions(+), 109 deletions(-) diff --git a/.serena/memories/backend/subtleties.md b/.serena/memories/backend/subtleties.md index 2282c6d792..495da48701 100644 --- a/.serena/memories/backend/subtleties.md +++ b/.serena/memories/backend/subtleties.md @@ -72,6 +72,7 @@ - The `media-storage-bytes-per-team` quota check on `upload-file-media-object` is TEMPORARILY DISABLED (2026-10-19, kept commented in `app.rpc.commands.media` with its `sql:get-team-id-for-file` lookup still active for the audit props): the quote performs a full `SUM` over `storage_object` per upload and was too slow in production. The `upload-sessions-per-profile` quota is unaffected and stays active. Re-enable only with a cheaper approximation. - Storage abstraction, backend configuration, logical buckets, object lifecycle, deduplication, access rules, and garbage collection: `mem:backend/storage`. - SVG validation strips DOCTYPE and uses secure SAX parsing. Basic SVG info falls back to 100x100 dimensions when width/height/viewBox are missing. +- The SVG sanitizer (`app.media.svg`) is a **blocklist**, not an allowlist: it drops dangerous elements and attributes by name and escapes every text and attribute value when it writes the tree back, so a payload hidden in a value cannot become live markup again. The allowlist rework is [#12190](https://github.com/penpot/penpot/issues/12190) and lands in media-processor, not here. `sanitize-svg` also tracks the namespace bindings through the recursion and drops any element that is not in the SVG namespace, so a document whose root is not an `svg` element in the SVG namespace is rejected and a child that rebinds `xmlns` to XHTML (`iframe`/`srcdoc`/`meta`) is dropped with its subtree; it compares the scheme of an URL the way the browser does (dropping tab/CR/LF/spaces), so `java script:` does not slip through the `javascript:` filter. The paths that store an uploaded SVG as its final object go through `app.media.svg/sanitize-svg-upload` (media upload, the URL download, both thumbnail writers and the organization logo); the three binfile imports go through `app.binfile.common/sanitize-imported-svg`, which calls `sanitize-svg` directly. The profile picture is not an SVG: `import-profile-picture` ignores an `image/svg+xml` download, because the avatar is always rendered as an image. The caller checks the size before calling it: the helper reads the whole file, and the size limit applies to the upload, not to the sanitized copy, which can be up to six times bigger. - Raster metadata is shell-derived with ImageMagick `identify`, verifies detected MIME against the supplied MIME, and swaps dimensions for EXIF orientations 6/8. - Remote image download requires 2xx status, `content-length`, a known MIME, and size under the configured maximum before writing the temp file; mismatched byte count is an internal error. - Font processing shells out to FontForge and WOFF conversion tools and can derive TTF/OTF/WOFF variants from uploaded fonts. diff --git a/backend/src/app/media/svg.clj b/backend/src/app/media/svg.clj index 2ef7112815..6d38011b4d 100644 --- a/backend/src/app/media/svg.clj +++ b/backend/src/app/media/svg.clj @@ -12,8 +12,10 @@ [app.common.data.macros :as dm] [app.common.exceptions :as ex] [app.common.logging :as l] + [app.storage.tmp :as tmp] [clojure.xml :as xml] - [cuerdas.core :as str]) + [cuerdas.core :as str] + [datoteka.fs :as fs]) (:import clojure.lang.XMLHandler java.io.InputStream @@ -46,15 +48,85 @@ (xml/parse istream secure-parser-factory)))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; SVG SERIALIZATION +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(def ^:private xml-declaration + "Declaration written at the top of every sanitized document." + "") + +(defn- escape-text + "Escape the characters that are special inside a XML text node." + [s] + (-> s + (str/replace "&" "&") + (str/replace "<" "<") + (str/replace ">" ">"))) + +(defn- escape-attr + "Escape the characters that are special inside a XML attribute value. + The value is written between double quotes, so the quote itself is + escaped too. + + Tabs and line breaks become spaces: that is what the XML parser does + with them on the next read, and it keeps the output stable." + [s] + (-> (str s) + (str/replace #"[\t\r\n]" " ") + (str/replace "&" "&") + (str/replace "<" "<") + (str/replace ">" ">") + (str/replace "\"" """) + (str/replace "'" "'"))) + +(defn- emit-element + "Serialize a parsed element, escaping every text and attribute value. + + The tag and attribute names come from the parser, so they are valid + XML names; only their values can carry markup. + + The attributes are sorted by name: the parser hands them over in the + reverse order of the document, so writing them as they come would + make the output change on every sanitization." + [{:keys [tag attrs content]}] + (let [attrs-str (apply str + (for [[k v] (sort-by (comp name key) attrs)] + (str " " (name k) "=\"" (escape-attr (str v)) "\""))) + content-str (apply str + (for [child content] + (if (map? child) + (emit-element child) + (escape-text child))))] + (if (seq content) + (str "<" (name tag) attrs-str ">" content-str "") + (str "<" (name tag) attrs-str "/>")))) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; SVG SANITIZATION ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; (def ^:private dangerous-attrs-pattern #"(?i)^on\w+$") (def ^:private javascript-href-pattern #"(?i)^javascript:") +(def ^:private url-ignored-chars + "Characters that the URL parser discards before it interprets the + scheme of an URL." + #"[\u0000-\u0020\u007f]") + +(defn- javascript-url? + "True when the value is a javascript: URL. + + The comparison drops the characters the URL parser drops (tab, CR, LF + and spaces), so a scheme split with one of them (`java script:`) + cannot slip through." + [v] + (let [v (str/replace (str v) url-ignored-chars "")] + (boolean (re-find javascript-href-pattern (str/trim v))))) + (def ^:private dangerous-tags #{"script" "foreignObject" "set" - "animate" "animateTransform" "animateColor" "animateMotion"}) + "animate" "animateTransform" "animateColor" "animateMotion" + "iframe" "meta" "embed" "link" "base"}) (defn- local-name "Return the local name of a parsed tag or attribute keyword. @@ -70,24 +142,82 @@ (last (str/split n ":")) n))) +(def ^:private svg-namespace "http://www.w3.org/2000/svg") + +(defn- namespace-bindings + "Return the namespace bindings in scope for an element: `::default` + plus the keyword prefixes declared in `attrs`, merged over the + inherited ones. The default binding is stored under a namespaced + keyword because a prefix cannot contain `/`, so no prefixed binding + can produce the same key. + + The XML parser does not process namespaces, so the declarations are + plain attributes (`:xmlns`, `:xmlns:svg`) and the prefix is part of + the tag name (`:svg:svg`)." + [attrs inherited] + (let [own (reduce-kv (fn [acc k v] + (let [n (name k)] + (cond + (= n "xmlns") (assoc acc ::default v) + (str/starts-with? n "xmlns:") (assoc acc (keyword (subs n 6)) v) + :else acc))) + {} + attrs)] + (merge inherited own))) + +(defn- element-namespace + "Return the namespace URI of `tag` under `bindings`: the default + namespace when the tag has no prefix, the prefixed binding otherwise. + + The prefix is part of the tag name (`:svg:svg`), so the binding key + is the prefix." + [tag bindings] + (let [n (name tag) + idx (str/last-index-of n ":")] + (if (nil? idx) + (::default bindings) + (get bindings (keyword (subs n 0 idx)))))) + +(defn- svg-root? + "True when the element is an `svg` element in the SVG namespace. + + The namespace is resolved with the same model as the recursion + (`namespace-bindings` + `element-namespace`), so the check does not + introduce a second rule." + [{:keys [tag attrs]}] + (let [bindings (namespace-bindings attrs {})] + (and (= "svg" (local-name tag)) + (= svg-namespace (element-namespace tag bindings))))) + (defn- sanitize-svg-element - "Recursively sanitize an SVG element by removing dangerous tags and attributes." - [{:keys [tag attrs content] :as element}] - (when (and (map? element) tag) - (when-not (contains? dangerous-tags (local-name tag)) - (let [clean-attrs (->> attrs - (remove (fn [[k v]] - (or (re-matches dangerous-attrs-pattern (local-name k)) - (and (= "href" (local-name k)) - (string? v) - (re-find javascript-href-pattern (str/trim v)))))) - (into {})) + "Recursively sanitize an SVG element by removing dangerous tags and + attributes. + + The namespace bindings are tracked through the recursion: an element + that is not in the SVG namespace is dropped with its whole subtree, + because the filter only knows about SVG elements and a child that + rebinds the namespace to XHTML can carry live HTML." + [element bindings] + (let [{:keys [tag attrs content]} element + bindings (namespace-bindings attrs bindings)] + (when (and (map? element) tag + (not (contains? dangerous-tags (local-name tag))) + (= svg-namespace (element-namespace tag bindings))) + (let [clean-attrs (->> attrs + (remove (fn [[k v]] + (let [n (name k)] + (or (and (not (str/starts-with? n "xmlns")) + (re-matches dangerous-attrs-pattern (local-name k))) + (and (= "href" (local-name k)) + (string? v) + (javascript-url? v)))))) + (into {})) clean-content (when content (->> content (filter #(or (string? %) (map? %))) (map (fn [child] (if (map? child) - (sanitize-svg-element child) + (sanitize-svg-element child bindings) child))) (filter some?) vec))] @@ -97,22 +227,65 @@ (defn sanitize-svg "Sanitize SVG content by removing dangerous elements and attributes. Removes " result (svg/sanitize-svg svg)] - (t/is (not (clojure.string/includes? result "" result (svg/sanitize-svg svg)] - (t/is (not (clojure.string/includes? result "foreignObject"))) - (t/is (not (clojure.string/includes? result "" result (svg/sanitize-svg svg)] - (t/is (not (clojure.string/includes? result "" result (svg/sanitize-svg svg)] - (t/is (not (clojure.string/includes? result "alert('xss')" result (svg/sanitize-svg svg)] - (t/is (not (clojure.string/includes? result "alert('xss')" result (svg/sanitize-svg svg)] - (t/is (not (clojure.string/includes? result "alert('xss')" result (svg/sanitize-svg svg)] - (t/is (not (clojure.string/includes? result "" result (svg/sanitize-svg svg)] - (t/is (not (clojure.string/includes? result "foreignObject"))) - (t/is (not (clojure.string/includes? result "" result (svg/sanitize-svg svg)] - (t/is (not (clojure.string/includes? result "javascript:"))) - (t/is (not (clojure.string/includes? result "alert"))) - (t/is (clojure.string/includes? result "" result (svg/sanitize-svg svg)] - (t/is (not (clojure.string/includes? result "onload"))) - (t/is (not (clojure.string/includes? result "onclick"))) - (t/is (not (clojure.string/includes? result "alert"))) - (t/is (clojure.string/includes? result "" result (svg/sanitize-svg svg)] - (t/is (not (clojure.string/includes? result "javascript:"))) - (t/is (not (clojure.string/includes? result "alert"))) - (t/is (clojure.string/includes? result "hola" result (svg/sanitize-svg svg)] - (t/is (clojure.string/includes? result "xml:space")) - (t/is (clojure.string/includes? result "hola"))))) + (t/is (str/includes? result "xml:space")) + (t/is (str/includes? result "hola"))))) + +;; --- SVG sanitization: escaping on serialization +;; +;; The sanitizer filters dangerous elements and attributes by name, so the +;; escaping happens when the tree is written back. A payload hidden in an +;; attribute value or in a text node must not become live markup again. + +(def ^:private clean-svg + "") + +(def ^:private evil-attribute-svg + ""] + (with-redefs [app.config/config (assoc app.config/config :media-max-file-size 1)] + (let [params {::th/type :upload-organization-logo + ::rpc/profile-id (:id profile) + :organization-id (uuid/random) + :content {:filename "big.svg" + :path path + :mtype "image/svg+xml" + :size (fs/size path)}} + out (th/multipart-management-command! params)] + (t/is (th/ex-of-type? (:error out) :restriction)) + (t/is (th/ex-of-code? (:error out) :media-max-file-size-reached)))))))) diff --git a/backend/test/backend_tests/rpc_media_test.clj b/backend/test/backend_tests/rpc_media_test.clj index 3b42685b4d..428489e9a4 100644 --- a/backend/test/backend_tests/rpc_media_test.clj +++ b/backend/test/backend_tests/rpc_media_test.clj @@ -10,10 +10,12 @@ [app.db :as db] [app.http.client :as http] [app.media :as media] + [app.media.svg :as svg] [app.rpc :as-alias rpc] [app.storage :as sto] [backend-tests.helpers :as th] [clojure.test :as t] + [cuerdas.core :as str] [datoteka.fs :as fs] [datoteka.io :as io] [mockery.core :refer [with-mocks]]) @@ -23,6 +25,14 @@ (t/use-fixtures :once th/state-init) (t/use-fixtures :each th/database-reset) +(def ^:private evil-svg + "An SVG that smuggles an event handler inside an attribute value." + ""))) + (t/is (= "x' onload='alert(1)" (get (:attrs parsed) :id))) + (t/is (str/includes? content "'"))))))) + +(def ^:private xhtml-child-svg + "An SVG whose root is in the SVG namespace but whose child rebinds the + default namespace to XHTML and embeds live HTML." + "") + +(t/deftest create-file-media-object-from-url-sanitizes-xhtml-child + (let [prof (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + body (.getBytes ^String xhtml-child-svg "UTF-8")] + (with-mocks [http-mock {:target 'app.http.client/req-with-redirects + :return {:status 200 + :headers {"content-type" "image/svg+xml" + "content-length" (str (alength ^bytes body))} + :body (java.io.ByteArrayInputStream. ^bytes body)}}] + (let [params {::th/type :create-file-media-object-from-url + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :url "https://example.com/evil.svg"} + out (th/command! params)] + (t/is (nil? (:error out))) + + (let [{:keys [media-id]} (:result out) + storage (:app.storage/storage th/*system*) + sobject (sto/get-object storage media-id) + content (slurp (sto/get-object-data storage sobject))] + (t/is (not (str/includes? content "iframe"))) + (t/is (not (str/includes? content "srcdoc"))) + (t/is (not (str/includes? content "http-equiv")))))))) + (t/deftest media-object-from-url-command (let [prof (th/create-profile* 1) proj (th/create-project* 1 {:profile-id (:id prof)