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)