Merge branch 'security-dos-via-asciidoc-includes' into 'master'

#40 - Denial of service via AsciiDoc include:: overuse

See merge request gitlab-org/security/gitlab!133

changelogs/unreleased/security-dos-via-asciidoc-includes.yml

+---
+title: Limit number of AsciiDoc includes per document
+merge_request:
+author:
+type: security

doc/user/asciidoc.md

@@ -221,6 +221,11 @@ include::basics.adoc[]
 include::https://example.org/installation.adoc[]
 ```
 
+To guarantee good system performance and prevent malicious documents causing
+problems, GitLab enforces a **maximum limit** on the number of include directives
+processed in any one document. Currently a total of 32 documents can be
+included, a number that is inclusive of transitive dependencies.
+
 ### Blocks
 
 ```asciidoc

lib/gitlab/asciidoc.rb

@@ -11,6 +11,7 @@ module Gitlab
   # the resulting HTML through HTML pipeline filters.
   module Asciidoc
     MAX_INCLUDE_DEPTH = 5
+    MAX_INCLUDES = 32
     DEFAULT_ADOC_ATTRS = {
         'showtitle' => true,
         'sectanchors' => true,
@@ -40,6 +41,7 @@ module Gitlab
                         extensions: extensions }
 
       context[:pipeline] = :ascii_doc
+      context[:max_includes] = [MAX_INCLUDES, context[:max_includes]].compact.min
 
       plantuml_setup
 

lib/gitlab/asciidoc/include_processor.rb

@@ -14,6 +14,8 @@ module Gitlab
 
         @context = context
         @repository = context[:repository] || context[:project].try(:repository)
+        @max_includes = context[:max_includes].to_i
+        @included = []
 
         # Note: Asciidoctor calls #freeze on extensions, so we can't set new
         # instance variables after initialization.
@@ -28,8 +30,11 @@ module Gitlab
       def include_allowed?(target, reader)
         doc = reader.document
 
-        return false if doc.attributes.fetch('max-include-depth').to_i < 1
+        max_include_depth = doc.attributes.fetch('max-include-depth').to_i
+
+        return false if max_include_depth < 1
         return false if target_uri?(target)
+        return false if included.size >= max_includes
 
         true
       end
@@ -62,7 +67,7 @@ module Gitlab
 
       private
 
-      attr_accessor :context, :repository, :cache
+      attr_reader :context, :repository, :cache, :max_includes, :included
 
       # Gets a Blob at a path for a specific revision.
       # This method will check that the Blob exists and contains readable text.
@@ -77,6 +82,8 @@ module Gitlab
         raise 'Blob not found' unless blob
         raise 'File is not readable' unless blob.readable_text?
 
+        included << filename
+
         blob
       end
 

spec/lib/gitlab/asciidoc/include_processor_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'nokogiri'
+
+describe Gitlab::Asciidoc::IncludeProcessor do
+  let_it_be(:project) { create(:project, :repository) }
+
+  let(:processor_context) do
+    {
+      project: project,
+      max_includes: max_includes,
+      ref: ref
+    }
+  end
+  let(:ref) { project.repository.root_ref }
+  let(:max_includes) { 10 }
+
+  let(:reader) { Asciidoctor::PreprocessorReader.new(document, lines, 'file.adoc') }
+  let(:document) { Asciidoctor::Document.new(lines) }
+
+  subject(:processor) { described_class.new(processor_context) }
+
+  let(:a_blob) { double(:Blob, readable_text?: true, data: a_data) }
+  let(:a_data) { StringIO.new('include::b.adoc[]') }
+
+  let(:lines) { [':max-include-depth: 1000'] + Array.new(10, 'include::a.adoc[]') }
+
+  before do
+    allow(project.repository).to receive(:blob_at).with(ref, 'a.adoc').and_return(a_blob)
+  end
+
+  describe '#include_allowed?' do
+    it 'allows the first include' do
+      expect(processor.send(:include_allowed?, 'foo.adoc', reader)).to be_truthy
+    end
+
+    it 'allows the Nth + 1 include' do
+      (max_includes - 1).times { processor.send(:read_blob, ref, 'a.adoc') }
+
+      expect(processor.send(:include_allowed?, 'foo.adoc', reader)).to be_truthy
+    end
+
+    it 'disallows the Nth + 1 include' do
+      max_includes.times { processor.send(:read_blob, ref, 'a.adoc') }
+
+      expect(processor.send(:include_allowed?, 'foo.adoc', reader)).to be_falsey
+    end
+  end
+end

spec/lib/gitlab/asciidoc_spec.rb

@@ -425,6 +425,24 @@ module Gitlab
           create_file(current_file, "= AsciiDoc\n")
         end
 
+        def many_includes(target)
+          Array.new(10, "include::#{target}[]").join("\n")
+        end
+
+        context 'cyclic imports' do
+          before do
+            create_file('doc/api/a.adoc', many_includes('b.adoc'))
+            create_file('doc/api/b.adoc', many_includes('a.adoc'))
+          end
+
+          let(:include_path) { 'a.adoc' }
+          let(:requested_path) { 'doc/api/README.md' }
+
+          it 'completes successfully' do
+            is_expected.to include('<p>Include this:</p>')
+          end
+        end
+
         context 'with path to non-existing file' do
           let(:include_path) { 'not-exists.adoc' }