Commit 13791c67 authored by Gabriel Mazetto's avatar Gabriel Mazetto Committed by Robert Speicher

Refactor SVG sanitizer and prevent `xlink:href` to refer to external resources

parent 02b88241
...@@ -10,13 +10,19 @@ module Gitlab ...@@ -10,13 +10,19 @@ module Gitlab
DATA_ATTR_PATTERN = /\Adata-(?!xml)[a-z_][\w.\u00E0-\u00F6\u00F8-\u017F\u01DD-\u02AF-]*\z/u DATA_ATTR_PATTERN = /\Adata-(?!xml)[a-z_][\w.\u00E0-\u00F6\u00F8-\u017F\u01DD-\u02AF-]*\z/u
def scrub(node) def scrub(node)
unless Whitelist::ALLOWED_ELEMENTS.include?(node.name) if Whitelist::ALLOWED_ELEMENTS.include?(node.name)
node.unlink
else
valid_attributes = Whitelist::ALLOWED_ATTRIBUTES[node.name] valid_attributes = Whitelist::ALLOWED_ATTRIBUTES[node.name]
return unless valid_attributes
node.attribute_nodes.each do |attr| node.attribute_nodes.each do |attr|
unless valid_attributes && valid_attributes.include?(attribute_name_with_namespace(attr)) attr_name = attribute_name_with_namespace(attr)
if valid_attributes.include?(attr_name)
# xlink:href is on the whitelist but we should deny any reference other than internal ids
if attr_name == 'xlink:href' && unsafe_href?(attr)
attr.unlink
end
else
if Whitelist::ALLOWED_DATA_ATTRIBUTES_IN_ELEMENTS.include?(node.name) && data_attribute?(attr) if Whitelist::ALLOWED_DATA_ATTRIBUTES_IN_ELEMENTS.include?(node.name) && data_attribute?(attr)
# Arbitrary data attributes are allowed. Verify that the attribute # Arbitrary data attributes are allowed. Verify that the attribute
# is a valid data attribute. # is a valid data attribute.
...@@ -26,6 +32,8 @@ module Gitlab ...@@ -26,6 +32,8 @@ module Gitlab
end end
end end
end end
else
node.unlink
end end
end end
...@@ -37,7 +45,9 @@ module Gitlab ...@@ -37,7 +45,9 @@ module Gitlab
end end
end end
private def unsafe_href?(attr)
!attr.value.start_with?('#')
end
def data_attribute?(attr) def data_attribute?(attr)
attr.name.start_with?('data-') attr.name.start_with?('data-')
......
...@@ -2,12 +2,12 @@ require 'spec_helper' ...@@ -2,12 +2,12 @@ require 'spec_helper'
describe Gitlab::Sanitizers::SVG do describe Gitlab::Sanitizers::SVG do
let(:scrubber) { Gitlab::Sanitizers::SVG::Scrubber.new } let(:scrubber) { Gitlab::Sanitizers::SVG::Scrubber.new }
let(:namespace) { double(Nokogiri::XML::Namespace, prefix: 'xlink') } let(:namespace) { double(Nokogiri::XML::Namespace, prefix: 'xlink', href: 'http://www.w3.org/1999/xlink') }
let(:namespaced_attr) { double(Nokogiri::XML::Attr, name: 'href', namespace: namespace) } let(:namespaced_attr) { double(Nokogiri::XML::Attr, name: 'href', namespace: namespace, value: '#awesome_id') }
context 'scrubber' do context 'scrubber' do
describe '#scrub' do describe '#scrub' do
let(:invalid_element) { double(Nokogiri::XML::Node, name: 'invalid') } let(:invalid_element) { double(Nokogiri::XML::Node, name: 'invalid', value: 'invalid') }
let(:invalid_attribute) { double(Nokogiri::XML::Attr, name: 'invalid', namespace: nil) } let(:invalid_attribute) { double(Nokogiri::XML::Attr, name: 'invalid', namespace: nil) }
let(:valid_element) { double(Nokogiri::XML::Node, name: 'use') } let(:valid_element) { double(Nokogiri::XML::Node, name: 'use') }
...@@ -44,5 +44,17 @@ describe Gitlab::Sanitizers::SVG do ...@@ -44,5 +44,17 @@ describe Gitlab::Sanitizers::SVG do
expect(scrubber.attribute_name_with_namespace(namespaced_attr)).to eq('xlink:href') expect(scrubber.attribute_name_with_namespace(namespaced_attr)).to eq('xlink:href')
end end
end end
describe '#unsafe_href?' do
let(:unsafe_attr) { double(Nokogiri::XML::Attr, name: 'href', namespace: namespace, value: 'http://evilsite.example.com/random.svg') }
it 'returns true if href attribute is an external url' do
expect(scrubber.unsafe_href?(unsafe_attr)).to be_truthy
end
it 'returns false if href atttribute is an internal reference' do
expect(scrubber.unsafe_href?(namespaced_attr)).to be_falsey
end
end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment