diff options
author | Steve Azzopardi <sazzopardi@gitlab.com> | 2018-11-23 17:47:56 +0300 |
---|---|---|
committer | Steve Azzopardi <sazzopardi@gitlab.com> | 2018-11-23 17:47:56 +0300 |
commit | 5a26a1eae8f3ef10fb43b8d6be65ae9a36da2347 (patch) | |
tree | fe07af0fbd3a95229f3f87249f53601da08c1e73 | |
parent | d0dadd3bb3d3d3caa8d7795a2ca90b41c6f85961 (diff) | |
parent | 918a742449036c4a440881fce3c7e9bc6d9cfea4 (diff) |
Merge branch 'security-11-3-xss-in-markdown-following-unrecognized-html-element' into 'security-11-3'
[11.3] XSS in markdown following unrecognized HTML element
See merge request gitlab/gitlabhq!2633
5 files changed, 25 insertions, 2 deletions
diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 6e2adc76ec6..a8c9e54f00c 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -15,7 +15,7 @@ module CacheMarkdownField # Increment this number every time the renderer changes its output CACHE_REDCARPET_VERSION = 3 CACHE_COMMONMARK_VERSION_START = 10 - CACHE_COMMONMARK_VERSION = 11 + CACHE_COMMONMARK_VERSION = 12 # changes to these attributes cause the cache to be invalidates INVALIDATED_BY = %w[author project].freeze diff --git a/changelogs/unreleased/security-11-3-xss-in-markdown-following-unrecognized-html-element.yml b/changelogs/unreleased/security-11-3-xss-in-markdown-following-unrecognized-html-element.yml new file mode 100644 index 00000000000..16c4474aadd --- /dev/null +++ b/changelogs/unreleased/security-11-3-xss-in-markdown-following-unrecognized-html-element.yml @@ -0,0 +1,5 @@ +--- +title: Fix possible XSS attack in Markdown urls with spaces +merge_request: +author: +type: security diff --git a/lib/banzai/filter/spaced_link_filter.rb b/lib/banzai/filter/spaced_link_filter.rb index a27f1d46863..c6a3a763c23 100644 --- a/lib/banzai/filter/spaced_link_filter.rb +++ b/lib/banzai/filter/spaced_link_filter.rb @@ -17,6 +17,9 @@ module Banzai # This is a small extension to the CommonMark spec. If they start allowing # spaces in urls, we could then remove this filter. # + # Note: Filter::SanitizationFilter should always be run sometime after this filter + # to prevent XSS attacks + # class SpacedLinkFilter < HTML::Pipeline::Filter include ActionView::Helpers::TagHelper diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb index bd34614f149..227b6c8d0b5 100644 --- a/lib/banzai/pipeline/gfm_pipeline.rb +++ b/lib/banzai/pipeline/gfm_pipeline.rb @@ -10,13 +10,16 @@ module Banzai def self.filters @filters ||= FilterArray[ Filter::PlantumlFilter, + + # Must always be before the SanitizationFilter to prevent XSS attacks + Filter::SpacedLinkFilter, + Filter::SanitizationFilter, Filter::SyntaxHighlightFilter, Filter::MathFilter, Filter::ColorFilter, Filter::MermaidFilter, - Filter::SpacedLinkFilter, Filter::VideoLinkFilter, Filter::ImageLazyLoadFilter, Filter::ImageLinkFilter, diff --git a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb index df24cef0b8b..91b0499375d 100644 --- a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb @@ -104,5 +104,17 @@ describe Banzai::Pipeline::GfmPipeline do expect(output).to include("src=\"test%20image.png\"") end + + it 'sanitizes the fixed link' do + markdown_xss = "[xss](javascript: alert%28document.domain%29)" + output = described_class.to_html(markdown_xss, project: project) + + expect(output).not_to include("javascript") + + markdown_xss = "<invalidtag>\n[xss](javascript:alert%28document.domain%29)" + output = described_class.to_html(markdown_xss, project: project) + + expect(output).not_to include("javascript") + end end end |