Reject slug+uri concat if slug is deemed unsafe
First reported: https://gitlab.com/gitlab-org/gitlab-ce/issues/60143 When the page slug is "javascript:" and we attempt to link to a relative path (using `.` or `..`) the code will concatenate the slug and the uri. This MR adds a guard to that concat step that will return `nil` if the incoming slug matches against any of the "unsafe" slug regexes; currently this is only for the slug "javascript:" but can be extended if needed. Manually tested against a non-exhaustive list from OWASP of common javascript XSS exploits that have to to with mangling the "javascript:" method, and all are caught by this change or by existing code that ingests the user-specified slug.
This commit is contained in:
parent
a600c0a78d
commit
a76fdcb7a3
3 changed files with 55 additions and 0 deletions
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Filter relative links in wiki for XSS
|
||||
merge_request:
|
||||
author:
|
||||
type: security
|
|
@ -4,6 +4,8 @@ module Banzai
|
|||
module Filter
|
||||
class WikiLinkFilter < HTML::Pipeline::Filter
|
||||
class Rewriter
|
||||
UNSAFE_SLUG_REGEXES = [/\Ajavascript:/i].freeze
|
||||
|
||||
def initialize(link_string, wiki:, slug:)
|
||||
@uri = Addressable::URI.parse(link_string)
|
||||
@wiki_base_path = wiki && wiki.wiki_base_path
|
||||
|
@ -35,6 +37,8 @@ module Banzai
|
|||
|
||||
# Of the form `./link`, `../link`, or similar
|
||||
def apply_hierarchical_link_rules!
|
||||
return if slug_considered_unsafe?
|
||||
|
||||
@uri = Addressable::URI.join(@slug, @uri) if @uri.to_s[0] == '.'
|
||||
end
|
||||
|
||||
|
@ -54,6 +58,10 @@ module Banzai
|
|||
def repository_upload?
|
||||
@uri.relative? && @uri.path.starts_with?(Wikis::CreateAttachmentService::ATTACHMENT_PATH)
|
||||
end
|
||||
|
||||
def slug_considered_unsafe?
|
||||
UNSAFE_SLUG_REGEXES.any? { |r| r.match?(@slug) }
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -70,5 +70,47 @@ describe Banzai::Filter::WikiLinkFilter do
|
|||
expect(filtered_link.attribute('href').value).to eq(invalid_link)
|
||||
end
|
||||
end
|
||||
|
||||
context "when the slug is deemed unsafe or invalid" do
|
||||
let(:link) { "alert(1);" }
|
||||
|
||||
invalid_slugs = [
|
||||
"javascript:",
|
||||
"JaVaScRiPt:",
|
||||
"\u0001java\u0003script:",
|
||||
"javascript :",
|
||||
"javascript: ",
|
||||
"javascript : ",
|
||||
":javascript:",
|
||||
"javascript:",
|
||||
"javascript:",
|
||||
"javascript:",
|
||||
"javascript:",
|
||||
"java\0script:",
|
||||
"  javascript:"
|
||||
]
|
||||
|
||||
invalid_slugs.each do |slug|
|
||||
context "with the slug #{slug}" do
|
||||
it "doesn't rewrite a (.) relative link" do
|
||||
filtered_link = filter(
|
||||
"<a href='.#{link}'>Link</a>",
|
||||
project_wiki: wiki,
|
||||
page_slug: slug).children[0]
|
||||
|
||||
expect(filtered_link.attribute('href').value).not_to include(slug)
|
||||
end
|
||||
|
||||
it "doesn't rewrite a (..) relative link" do
|
||||
filtered_link = filter(
|
||||
"<a href='..#{link}'>Link</a>",
|
||||
project_wiki: wiki,
|
||||
page_slug: slug).children[0]
|
||||
|
||||
expect(filtered_link.attribute('href').value).not_to include(slug)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
Loading…
Reference in a new issue