mirror of
https://github.com/penpot/penpot.git
synced 2026-08-06 21:08:34 +00:00
🐛 Sanitize SVG files on upload to prevent XSS
Add sanitize-svg function that removes dangerous elements and attributes: - script tags - foreignObject elements - Event handler attributes (onload, onmouseover, etc.) - javascript: URLs from href/xlink:href attributes Apply sanitization in process-main-image before storing SVG files. AI-assisted-by: mimo-v2.5-pro
This commit is contained in:
Andrey Antukh
parent
0ac711aa68
commit
c4dd04353f
@ -7,30 +7,22 @@
|
||||
(ns app.media.local
|
||||
"Local media processing via ImageMagick and FontForge shell commands."
|
||||
(:require
|
||||
[app.common.data :as d]
|
||||
[app.common.data.macros :as dm]
|
||||
[app.common.exceptions :as ex]
|
||||
[app.common.logging :as l]
|
||||
[app.common.media :as cm]
|
||||
[app.common.schema :as sm]
|
||||
[app.common.time :as ct]
|
||||
[app.config :as cf]
|
||||
[app.media.svg :as svg]
|
||||
[app.media.validation :as validation]
|
||||
[app.storage.tmp :as tmp]
|
||||
[app.util.shell :as shell]
|
||||
[buddy.core.bytes :as bb]
|
||||
[buddy.core.codecs :as bc]
|
||||
[clojure.string]
|
||||
[clojure.xml :as xml]
|
||||
[cuerdas.core :as str]
|
||||
[datoteka.fs :as fs]
|
||||
[datoteka.io :as io])
|
||||
(:import
|
||||
clojure.lang.XMLHandler
|
||||
java.io.InputStream
|
||||
javax.xml.parsers.SAXParserFactory
|
||||
javax.xml.XMLConstants
|
||||
org.apache.commons.io.IOUtils))
|
||||
[datoteka.io :as io]))
|
||||
|
||||
(defmulti process (fn [_system params] (:cmd params)))
|
||||
|
||||
@ -40,30 +32,6 @@
|
||||
:code :not-implemented
|
||||
:hint (str/fmt "No impl found for local process cmd: %s" cmd)))
|
||||
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
;; SVG PARSING
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
|
||||
(defn- secure-parser-factory
|
||||
[^InputStream input ^XMLHandler handler]
|
||||
(.. (doto (SAXParserFactory/newInstance)
|
||||
(.setFeature XMLConstants/FEATURE_SECURE_PROCESSING true)
|
||||
(.setFeature "http://apache.org/xml/features/disallow-doctype-decl" true))
|
||||
(newSAXParser)
|
||||
(parse input handler)))
|
||||
|
||||
(defn- strip-doctype
|
||||
[data]
|
||||
(cond-> data
|
||||
(str/includes? data "<!DOCTYPE")
|
||||
(str/replace #"<\!DOCTYPE[^>]*>" "")))
|
||||
|
||||
(defn parse-svg
|
||||
[text]
|
||||
(let [text (strip-doctype text)]
|
||||
(dm/with-open [istream (IOUtils/toInputStream ^String text "UTF-8")]
|
||||
(xml/parse istream secure-parser-factory))))
|
||||
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
;; IMAGE THUMBNAILS
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
@ -167,34 +135,6 @@
|
||||
"-extent" (str width "x" height)
|
||||
"-quality" (str quality)]))))
|
||||
|
||||
(defn get-basic-info-from-svg
|
||||
[{:keys [tag attrs] :as data}]
|
||||
(when (not= tag :svg)
|
||||
(ex/raise :type :validation
|
||||
:code :unable-to-parse-svg
|
||||
:hint "uploaded svg has invalid content"))
|
||||
(reduce (fn [default f]
|
||||
(if-let [res (f attrs)]
|
||||
(reduced res)
|
||||
default))
|
||||
{:width 100 :height 100}
|
||||
[(fn parse-width-and-height
|
||||
[{:keys [width height]}]
|
||||
(when (and (string? width)
|
||||
(string? height))
|
||||
(let [width (d/parse-double width)
|
||||
height (d/parse-double height)]
|
||||
(when (and width height)
|
||||
{:width (int width)
|
||||
:height (int height)}))))
|
||||
(fn parse-viewbox
|
||||
[{:keys [viewBox]}]
|
||||
(let [[x y width height] (->> (str/split viewBox #"\s+" 4)
|
||||
(map d/parse-double))]
|
||||
(when (and x y width height)
|
||||
{:width (int width)
|
||||
:height (int height)})))]))
|
||||
|
||||
(defn- get-dimensions-with-orientation [system ^String path]
|
||||
;; Image magick doesn't give info about exif rotation so we use the identify command
|
||||
;; If we are processing an animated gif we use the first frame with -scene 0
|
||||
@ -217,7 +157,7 @@
|
||||
[system {:keys [input] :as params}]
|
||||
(let [{:keys [path mtype] :as input} (validation/check-input input)]
|
||||
(if (= mtype "image/svg+xml")
|
||||
(let [info (some-> path slurp parse-svg get-basic-info-from-svg)]
|
||||
(let [info (some-> path slurp svg/parse-svg svg/get-basic-info-from-svg)]
|
||||
(when-not info
|
||||
(ex/raise :type :validation
|
||||
:code :invalid-svg-file
|
||||
|
||||
@ -13,7 +13,7 @@
|
||||
[app.common.uri :as uri]
|
||||
[app.config :as cf]
|
||||
[app.http.client :as http]
|
||||
[app.media.local :as local]
|
||||
[app.media.svg :as svg]
|
||||
[app.media.validation :as validation]
|
||||
[app.setup :as-alias setup]
|
||||
[app.storage.tmp :as tmp]
|
||||
@ -182,7 +182,7 @@
|
||||
(let [{:keys [path mtype]} (validation/check-input input)]
|
||||
(if (= mtype "image/svg+xml")
|
||||
;; SVG: parse locally (Sharp doesn't support SVG)
|
||||
(let [info (some-> path slurp local/parse-svg local/get-basic-info-from-svg)]
|
||||
(let [info (some-> path slurp svg/parse-svg svg/get-basic-info-from-svg)]
|
||||
(when-not info
|
||||
(ex/raise :type :validation
|
||||
:code :invalid-svg-file
|
||||
|
||||
130
backend/src/app/media/svg.clj
Normal file
130
backend/src/app/media/svg.clj
Normal file
@ -0,0 +1,130 @@
|
||||
;; This Source Code Form is subject to the terms of the Mozilla Public
|
||||
;; License, v. 2.0. If a copy of the MPL was not distributed with this
|
||||
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
|
||||
;;
|
||||
;; Copyright (c) KALEIDOS INC Sucursal en España SL
|
||||
|
||||
(ns app.media.svg
|
||||
"SVG parsing, sanitization, and info extraction.
|
||||
Centralizes all SVG-related security concerns."
|
||||
(:require
|
||||
[app.common.data :as d]
|
||||
[app.common.data.macros :as dm]
|
||||
[app.common.exceptions :as ex]
|
||||
[app.common.logging :as l]
|
||||
[clojure.xml :as xml]
|
||||
[cuerdas.core :as str])
|
||||
(:import
|
||||
clojure.lang.XMLHandler
|
||||
java.io.InputStream
|
||||
javax.xml.parsers.SAXParserFactory
|
||||
javax.xml.XMLConstants
|
||||
org.apache.commons.io.IOUtils))
|
||||
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
;; SVG PARSING
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
|
||||
(defn- secure-parser-factory
|
||||
[^InputStream input ^XMLHandler handler]
|
||||
(.. (doto (SAXParserFactory/newInstance)
|
||||
(.setFeature XMLConstants/FEATURE_SECURE_PROCESSING true)
|
||||
(.setFeature "http://apache.org/xml/features/disallow-doctype-decl" true))
|
||||
(newSAXParser)
|
||||
(parse input handler)))
|
||||
|
||||
(defn- strip-doctype
|
||||
[data]
|
||||
(cond-> data
|
||||
(str/includes? data "<!DOCTYPE")
|
||||
(str/replace #"<\!DOCTYPE[^>]*>" "")))
|
||||
|
||||
(defn parse-svg
|
||||
[text]
|
||||
(let [text (strip-doctype text)]
|
||||
(dm/with-open [istream (IOUtils/toInputStream ^String text "UTF-8")]
|
||||
(xml/parse istream secure-parser-factory))))
|
||||
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
;; SVG SANITIZATION
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
|
||||
(def ^:private dangerous-attrs-pattern #"(?i)^on\w+$")
|
||||
(def ^:private javascript-href-pattern #"(?i)^javascript:")
|
||||
|
||||
(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)
|
||||
(let [dangerous-tags #{:script :foreignObject :set :animate :animateTransform :animateColor :animateMotion}]
|
||||
(when-not (contains? dangerous-tags tag)
|
||||
(let [clean-attrs (->> attrs
|
||||
(remove (fn [[k v]]
|
||||
(or (re-matches dangerous-attrs-pattern (name k))
|
||||
(and (#{:href :xlink:href} k)
|
||||
(string? v)
|
||||
(re-find javascript-href-pattern (str/trim v))))))
|
||||
(into {}))
|
||||
clean-content (when content
|
||||
(->> content
|
||||
(filter #(or (string? %) (map? %)))
|
||||
(map (fn [child]
|
||||
(if (map? child)
|
||||
(sanitize-svg-element child)
|
||||
child)))
|
||||
(filter some?)
|
||||
vec))]
|
||||
(cond-> {:tag tag :attrs clean-attrs}
|
||||
(seq clean-content) (assoc :content clean-content)))))))
|
||||
|
||||
(defn sanitize-svg
|
||||
"Sanitize SVG content by removing dangerous elements and attributes.
|
||||
Removes <script> tags, <foreignObject> elements, event handlers (on*),
|
||||
and javascript: URLs from href attributes."
|
||||
[svg-text]
|
||||
(try
|
||||
(let [parsed (parse-svg svg-text)
|
||||
sanitized (sanitize-svg-element parsed)]
|
||||
(if sanitized
|
||||
(with-out-str (xml/emit sanitized))
|
||||
(ex/raise :type :validation
|
||||
:code :invalid-svg-file
|
||||
:hint "SVG sanitization produced no output")))
|
||||
(catch Exception e
|
||||
(l/warn :hint "SVG sanitization failed, rejecting upload" :cause e)
|
||||
(ex/raise :type :validation
|
||||
:code :invalid-svg-file
|
||||
:hint "SVG parsing failed during sanitization"
|
||||
:cause e))))
|
||||
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
;; SVG INFO EXTRACTION
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
|
||||
(defn get-basic-info-from-svg
|
||||
[{:keys [tag attrs] :as data}]
|
||||
(when (not= tag :svg)
|
||||
(ex/raise :type :validation
|
||||
:code :unable-to-parse-svg
|
||||
:hint "uploaded svg has invalid content"))
|
||||
(reduce (fn [default f]
|
||||
(if-let [res (f attrs)]
|
||||
(reduced res)
|
||||
default))
|
||||
{:width 100 :height 100}
|
||||
[(fn parse-width-and-height
|
||||
[{:keys [width height]}]
|
||||
(when (and (string? width)
|
||||
(string? height))
|
||||
(let [width (d/parse-double width)
|
||||
height (d/parse-double height)]
|
||||
(when (and width height)
|
||||
{:width (int width)
|
||||
:height (int height)}))))
|
||||
(fn parse-viewbox
|
||||
[{:keys [viewBox]}]
|
||||
(let [[x y width height] (->> (str/split viewBox #"\s+" 4)
|
||||
(map d/parse-double))]
|
||||
(when (and x y width height)
|
||||
{:width (int width)
|
||||
:height (int height)})))]))
|
||||
@ -16,6 +16,7 @@
|
||||
[app.db :as db]
|
||||
[app.loggers.audit :as-alias audit]
|
||||
[app.media :as media]
|
||||
[app.media.svg :as svg]
|
||||
[app.media.validation :as media.v]
|
||||
[app.rpc :as-alias rpc]
|
||||
[app.rpc.climit :as climit]
|
||||
@ -114,13 +115,22 @@
|
||||
|
||||
(defn- process-main-image
|
||||
[info]
|
||||
(let [hash (sto/calculate-hash (:path info))
|
||||
data (-> (sto/content (:path info))
|
||||
(sto/wrap-with-hash hash))]
|
||||
(let [path (:path info)
|
||||
mtype (:mtype info)
|
||||
path (if (= mtype "image/svg+xml")
|
||||
(let [content (slurp path)
|
||||
sanitized (svg/sanitize-svg content)
|
||||
temp-path (tmp/tempfile :prefix "penpot-svg-" :suffix ".svg" :min-age "5m")]
|
||||
(spit (str temp-path) sanitized)
|
||||
temp-path)
|
||||
path)
|
||||
hash (sto/calculate-hash path)
|
||||
data (-> (sto/content path)
|
||||
(sto/wrap-with-hash hash))]
|
||||
{::sto/content data
|
||||
::sto/deduplicate? true
|
||||
::sto/touched-at (:ts info)
|
||||
:content-type (:mtype info)
|
||||
:content-type mtype
|
||||
:bucket "file-media-object"}))
|
||||
|
||||
(defn- process-thumb-image
|
||||
|
||||
@ -8,6 +8,7 @@
|
||||
(:require
|
||||
[app.common.exceptions :as ex]
|
||||
[app.media :as media]
|
||||
[app.media.svg :as svg]
|
||||
[backend-tests.helpers :as th]
|
||||
[clojure.test :as t]
|
||||
[datoteka.fs :as fs]))
|
||||
@ -55,6 +56,87 @@
|
||||
(t/is (pos? (:width info)))
|
||||
(t/is (pos? (:height info))))))
|
||||
|
||||
(t/deftest sanitize-svg-script-tag
|
||||
(t/testing "sanitize-svg removes script tags"
|
||||
(let [svg "<svg xmlns=\"http://www.w3.org/2000/svg\" width=\"100\" height=\"100\"><script>alert('xss')</script><rect width=\"50\" height=\"50\"/></svg>"
|
||||
result (svg/sanitize-svg svg)]
|
||||
(t/is (not (clojure.string/includes? result "<script>")))
|
||||
(t/is (not (clojure.string/includes? result "alert")))
|
||||
(t/is (clojure.string/includes? result "<rect")))))
|
||||
|
||||
(t/deftest sanitize-svg-event-handlers
|
||||
(t/testing "sanitize-svg removes event handler attributes"
|
||||
(let [svg "<svg xmlns=\"http://www.w3.org/2000/svg\" width=\"100\" height=\"100\" onload=\"alert('xss')\"><rect width=\"50\" height=\"50\" onmouseover=\"alert('xss')\"/></svg>"
|
||||
result (svg/sanitize-svg svg)]
|
||||
(t/is (not (clojure.string/includes? result "onload")))
|
||||
(t/is (not (clojure.string/includes? result "onmouseover")))
|
||||
(t/is (not (clojure.string/includes? result "alert")))
|
||||
(t/is (clojure.string/includes? result "<rect")))))
|
||||
|
||||
(t/deftest sanitize-svg-javascript-href
|
||||
(t/testing "sanitize-svg removes javascript: URLs from href attributes"
|
||||
(let [svg "<svg xmlns=\"http://www.w3.org/2000/svg\" xmlns:xlink=\"http://www.w3.org/1999/xlink\" width=\"100\" height=\"100\"><a xlink:href=\"javascript:alert('xss')\"><rect width=\"50\" height=\"50\"/></a></svg>"
|
||||
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 "<a")))))
|
||||
|
||||
(t/deftest sanitize-svg-foreign-object
|
||||
(t/testing "sanitize-svg removes foreignObject elements"
|
||||
(let [svg "<svg xmlns=\"http://www.w3.org/2000/svg\" width=\"100\" height=\"100\"><foreignObject width=\"100\" height=\"100\"><body xmlns=\"http://www.w3.org/1999/xhtml\"><script>alert('xss')</script></body></foreignObject><rect width=\"50\" height=\"50\"/></svg>"
|
||||
result (svg/sanitize-svg svg)]
|
||||
(t/is (not (clojure.string/includes? result "foreignObject")))
|
||||
(t/is (not (clojure.string/includes? result "<script>")))
|
||||
(t/is (clojure.string/includes? result "<rect")))))
|
||||
|
||||
(t/deftest sanitize-svg-clean-content
|
||||
(t/testing "sanitize-svg preserves clean SVG content"
|
||||
(let [svg "<svg xmlns=\"http://www.w3.org/2000/svg\" width=\"100\" height=\"100\"><rect width=\"50\" height=\"50\" fill=\"red\"/><circle cx=\"75\" cy=\"75\" r=\"20\" fill=\"blue\"/></svg>"
|
||||
result (svg/sanitize-svg svg)]
|
||||
(t/is (clojure.string/includes? result "<rect"))
|
||||
(t/is (clojure.string/includes? result "<circle"))
|
||||
(t/is (or (clojure.string/includes? result "fill=\"red\"")
|
||||
(clojure.string/includes? result "fill='red'")))
|
||||
(t/is (or (clojure.string/includes? result "fill=\"blue\"")
|
||||
(clojure.string/includes? result "fill='blue'"))))))
|
||||
|
||||
(t/deftest sanitize-svg-invalid-svg-rejected
|
||||
(t/testing "sanitize-svg rejects malformed SVG input"
|
||||
(let [svg "<svg><not-closed>"]
|
||||
(t/is (thrown-with-msg? Exception #"SVG parsing failed during sanitization"
|
||||
(svg/sanitize-svg svg))))))
|
||||
|
||||
(t/deftest sanitize-svg-preserves-xlink
|
||||
(t/testing "sanitize-svg preserves legitimate xlink:href attributes"
|
||||
(let [svg "<svg xmlns=\"http://www.w3.org/2000/svg\" xmlns:xlink=\"http://www.w3.org/1999/xlink\" width=\"100\" height=\"100\"><use xlink:href=\"#icon\"/></svg>"
|
||||
result (svg/sanitize-svg svg)]
|
||||
(t/is (clojure.string/includes? result "xlink:href"))
|
||||
(t/is (clojure.string/includes? result "#icon")))))
|
||||
|
||||
(t/deftest sanitize-svg-javascript-href-whitespace
|
||||
(t/testing "sanitize-svg catches javascript: URLs with leading whitespace"
|
||||
(let [svg "<svg xmlns=\"http://www.w3.org/2000/svg\" width=\"100\" height=\"100\"><a href=\" javascript:alert('xss')\"><rect width=\"50\" height=\"50\"/></a></svg>"
|
||||
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 "<a")))))
|
||||
|
||||
(t/deftest sanitize-svg-nested-script
|
||||
(t/testing "sanitize-svg removes script tags from nested elements"
|
||||
(let [svg "<svg xmlns=\"http://www.w3.org/2000/svg\" width=\"100\" height=\"100\"><g><script>alert('xss')</script></g></svg>"
|
||||
result (svg/sanitize-svg svg)]
|
||||
(t/is (not (clojure.string/includes? result "<script")))
|
||||
(t/is (not (clojure.string/includes? result "alert")))
|
||||
(t/is (clojure.string/includes? result "<g")))))
|
||||
|
||||
(t/deftest sanitize-svg-smil-bypass
|
||||
(t/testing "sanitize-svg removes SMIL animation elements that can set on* attrs"
|
||||
(let [svg "<svg xmlns=\"http://www.w3.org/2000/svg\" width=\"100\" height=\"100\"><rect width=\"100\" height=\"100\" id=\"r\"/><set attributeName=\"onmouseover\" to=\"alert('xss')\" xlink:href=\"#r\" begin=\"0s\"/></svg>"
|
||||
result (svg/sanitize-svg svg)]
|
||||
(t/is (not (clojure.string/includes? result "<set")))
|
||||
(t/is (not (clojure.string/includes? result "onmouseover")))
|
||||
(t/is (clojure.string/includes? result "<rect")))))
|
||||
|
||||
(t/deftest info-invalid-image
|
||||
(t/testing "info on invalid image raises error"
|
||||
(let [path (fs/create-tempfile :prefix "penpot-test-" :suffix ".jpg")]
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user