explore using DOMPurify's allowlists

flavorjones commented 5 years ago

DOMPurify appears to be well-maintained and has an up-to-date allowlist. Explore using those allowlists instead of the HTML5lib-derived lists currently used by Loofah.

[x] determine how well whitelists match up to the currently-used-set
[x] determine how big the diff is
[ ] look at using IS_ALLOWED_URI to allowlist protocols
[ ] make sure to include the DOMPurify license in any machine-generated file
[ ] consider making a separate gem for Loofah's allowlist and inject it into Loofah by default
[ ] ... and then document how anybody else can modify or inject their own allowlists, which should address a lot of open issues (see the allowlist label)
[ ] ... and deprecate Whitelist in preference to Allowlist, exploring how to do so without totally breaking monkeypatches people may have made to Loofah's allowlists.

flavorjones commented 5 years ago

Links:

DOMPurify: https://github.com/cure53/DOMPurify/
attribute allowlist: https://github.com/cure53/DOMPurify/blob/master/src/attrs.js
tag allowlist: https://github.com/cure53/DOMPurify/blob/master/src/tags.js

flavorjones commented 5 years ago

Here are the diffs (assuming my PR https://github.com/cure53/DOMPurify/pull/309 will be accepted to correct misspellings).

"removal" reflects something allowed today that would not be allowed with DOMPurify's allowlist.

"addition" reflects something not allowed today that would be allowed with DOMPurify's allowlist.

html:tags:
  removals (1):
    ["command"]
  additions (23):
    ["blink", "body", "content", "data", "decorator", "element", "head", "hgroup", "html", "marquee", "menuitem",
     "nobr", "progress", "rp", "rt", "ruby", "shadow", "source", "spacer", "style", "template", "track", "wbr"]

mathml:tags:
  removals (6):
    ["annotation", "annotation-xml", "maction", "mprescripts", "none", "semantics"]
  additions (4):
    ["menclose", "mglyph", "mlabeledtr", "ms"]

svg:tags:
  removals (18):
    ["animate", "animateColor", "animateMotion", "animateTransform", "clipPath", "feGaussianBlur",
     "fegaussianblur", "font-face", "font-face-name", "font-face-src", "foreignObject", "foreignobject",
     "linearGradient", "missing-glyph", "radialGradient", "set", "textPath", "use"]
  additions (14):
    ["altglyph", "altglyphdef", "altglyphitem", "audio", "canvas", "font", "glyphref", "image", "pattern",
     "style", "tref", "video", "view", "vkern"]

html:attrs:
  removals (20):
    ["abbr", "accept-charset", "accesskey", "axis", "char", "charoff", "charset", "compact", "frame", "hspace",
     "longdesc", "loopcount", "loopend", "loopstart", "nohref", "prompt", "rules", "target", "vspace", 
     "xml:lang"]
  additions (30):
    ["autocomplete", "background", "bgcolor", "crossorigin", "default", "download", "face", "hidden", "high", 
     "integrity", "list", "low", "max", "min", "novalidate", "open", "optimum", "pattern", "placeholder", 
     "pubdate", "radiogroup", "required", "reversed", "role", "sizes", "spellcheck", "srclang", "srcset", 
     "step", "xmlns"]

mathml:attrs:
  removals (13):
    ["actiontype", "columnalign", "columnspacing", "encoding", "equalcolumns", "equalrows", "fontstyle", 
     "fontweight", "other", "xlink:href", "xlink:show", "xlink:type", "xmlns:xlink"]
  additions (22):
    ["accent", "accentunder", "bevelled", "columnsalign", "denomalign", "dir", "href", "id", "largeop", 
     "length", "lquote", "mathsize", "movablelimits", "notation", "numalign", "rquote", "scriptminsize", 
     "scriptsizemultiplier", "subscriptshift", "supscriptshift", "symmetric", "voffset"]

svg:attrs:
  removals (82):
    ["alphabetic", "arabic-form", "attributeName", "attributeType", "baseProfile", "baseprofile", "bbox", 
     "calcMode", "calcmode", "cap-height", "content", "descent", "filterRes", "filterUnits", "filterres", 
     "filterunits", "from", "gradientUnits", "hanging", "horiz-adv-x", "horiz-origin-x", "ideographic", 
     "keyPoints", "keySplines", "keyTimes", "markerHeight", "markerUnits", "markerWidth", "maskContentUnits", 
     "maskUnits", "mathematical", "overline-position", "overline-thickness", "panose-1", "pathLength", 
     "patternContentUnits", "patternTransform", "patternUnits", "preserveAspectRatio", "primitiveUnits", 
     "primitiveunits", "refX", "refY", "repeatCount", "repeatDur", "requiredExtensions", "requiredFeatures", 
     "requiredextensions", "requiredfeatures", "slope", "spacing", "startOffset", "startoffset", "stdDeviation", 
     "stemh", "stemv", "strikethrough-position", "strikethrough-thickness", "systemLanguage", "systemlanguage", 
     "target", "to", "underline-position", "underline-thickness", "unicode-range", "units-per-em", "version", 
     "viewBox", "widths", "x-height", "xlink:actuate", "xlink:arcrole", "xlink:href", "xlink:role", 
     "xlink:show", "xlink:title", "xlink:type", "xml:base", "xml:lang", "xml:space", "xmlns:xlink", "zoomAndPan"]
  additions (70):
    ["alignment-baseline", "azimuth", "basefrequency", "baseline-shift", "bias", "clip", "color-interpolation", 
    "color-profile", "diffuseconstant", "direction", "divisor", "edgemode", "elevation", "filter", 
    "flood-color", "flood-opacity", "font-size-adjust", "glyphref", "gradienttransform", "href", 
    "image-rendering", "in", "in2", "k1", "k2", "k3", "k4", "kernelmatrix", "kernelunitlength", "kerning", 
    "lengthadjust", "letter-spacing", "lighting-color", "local", "mask", "media", "mode", "numoctaves", 
    "operator", "order", "orientation", "overflow", "paint-order", "preservealpha", "radius", "result", "scale", 
    "seed", "shape-rendering", "specularconstant", "specularexponent", "spreadmethod", "stitchtiles", "style", 
    "surfacescale", "tabindex", "targetx", "targety", "text-decoration", "text-rendering", "textlength", 
    "vert-adv-y", "vert-origin-x", "vert-origin-y", "word-spacing", "wrap", "writing-mode", "xchannelselector", 
    "ychannelselector", "z"]

flavorjones commented 5 years ago

This is a pretty big changeset. I'm curious if anybody has objections to moving to it?

flavorjones commented 4 years ago

Notable conflicts:

html attr contenteditable added by https://github.com/flavorjones/loofah/pull/167 would be removed
svg attrs by and values would be re-added, re-introducing CVE-2018-16468 (#137) and CVE-2019-15587 (#154)

trevorrjohn commented 5 months ago

DomPurify does not allow contenteditable https://github.com/cure53/DOMPurify/issues/229

flavorjones commented 5 months ago

The little work I did on this is on branch https://github.com/flavorjones/loofah/tree/155-use-dompurify-safelists

trevorrjohn commented 5 months ago

We looked at this but the branch no longer works because node is a bit strange. Cannot load the attrs.js nor tags.js because it is using an import statement. Discussed the best option forward would be to ask DOMPurify to move those constants into a separate text file so that we could require and load it more easily. 🤷

flavorjones / loofah

explore using DOMPurify's allowlists #155