From ee48121752eb99594065f9a44fa537eada18c4d3 Mon Sep 17 00:00:00 2001
From: NAITOH Jun <naitoh@gmail.com>
Date: Sun, 28 Jul 2024 16:38:38 +0900
Subject: [PATCH] Fix to not allow Parameter Entity References at internal
 subsets.

## Why?
In the internal subset of DTD, references to parameter entities are not allowed within markup declarations.

See: https://www.w3.org/TR/xml/#wfc-PEinInternalSubset

> Well-formedness constraint: PEs in Internal Subset

> In the internal DTD subset, parameter-entity references MUST NOT occur within markup declarations; they may occur where markup declarations can occur. (This does not apply to references that occur in external parameter entities or to the external subset.)
---
 lib/rexml/entity.rb   | 37 ++++----------------------------
 test/test_document.rb | 50 -------------------------------------------
 test/test_entity.rb   | 32 ++++++++++-----------------
 3 files changed, 15 insertions(+), 104 deletions(-)

diff --git a/lib/rexml/entity.rb b/lib/rexml/entity.rb
index 573db691..d7983654 100644
--- a/lib/rexml/entity.rb
+++ b/lib/rexml/entity.rb
@@ -123,43 +123,14 @@ def to_s
     end
 
     PEREFERENCE_RE = /#{PEREFERENCE}/um
-    # Returns the value of this entity.  At the moment, only internal entities
-    # are processed.  If the value contains internal references (IE,
-    # %blah;), those are replaced with their values.  IE, if the doctype
-    # contains:
-    #  <!ENTITY % foo "bar">
-    #  <!ENTITY yada "nanoo %foo; nanoo>
-    # then:
-    #  doctype.entity('yada').value   #-> "nanoo bar nanoo"
     def value
-      @resolved_value ||= resolve_value
-    end
-
-    def parent=(other)
-      @resolved_value = nil
-      super
-    end
-
-    private
-    def resolve_value
       return nil if @value.nil?
-      return @value unless @value.match?(PEREFERENCE_RE)
 
-      matches = @value.scan(PEREFERENCE_RE)
-      rv = @value.clone
-      if @parent
-        sum = 0
-        matches.each do |entity_reference|
-          entity_value = @parent.entity( entity_reference[0] )
-          if sum + entity_value.bytesize > Security.entity_expansion_text_limit
-            raise "entity expansion has grown too large"
-          else
-            sum += entity_value.bytesize
-          end
-          rv.gsub!( /%#{entity_reference.join};/um, entity_value )
-        end
+      if @value.match?(PEREFERENCE_RE)
+        raise REXML::ParseException.new("Parameter Entity References forbidden in internal subset :#{@value}")
       end
-      rv
+
+      @value
     end
   end
 
diff --git a/test/test_document.rb b/test/test_document.rb
index 0764631d..6c653fa1 100644
--- a/test/test_document.rb
+++ b/test/test_document.rb
@@ -127,56 +127,6 @@ def test_with_default_entity
           end
         end
       end
-
-      class ParameterEntityTest < self
-        def test_have_value
-          xml = <<EOF
-<!DOCTYPE root [
-  <!ENTITY % a "BOOM.BOOM.BOOM.BOOM.BOOM.BOOM.BOOM.BOOM.BOOM.">
-  <!ENTITY % b "%a;%a;%a;%a;%a;%a;%a;%a;%a;%a;%a;%a;%a;%a;%a;">
-  <!ENTITY % c "%b;%b;%b;%b;%b;%b;%b;%b;%b;%b;%b;%b;%b;%b;%b;">
-  <!ENTITY % d "%c;%c;%c;%c;%c;%c;%c;%c;%c;%c;%c;%c;%c;%c;%c;">
-  <!ENTITY % e "%d;%d;%d;%d;%d;%d;%d;%d;%d;%d;%d;%d;%d;%d;%d;">
-  <!ENTITY % f "%e;%e;%e;%e;%e;%e;%e;%e;%e;%e;%e;%e;%e;%e;%e;">
-  <!ENTITY % g "%f;%f;%f;%f;%f;%f;%f;%f;%f;%f;%f;%f;%f;%f;%f;">
-  <!ENTITY test "test %g;">
-]>
-<cd></cd>
-EOF
-
-          assert_raise(REXML::ParseException) do
-            REXML::Document.new(xml)
-          end
-          REXML::Security.entity_expansion_limit = 100
-          assert_equal(100, REXML::Security.entity_expansion_limit)
-          assert_raise(REXML::ParseException) do
-            REXML::Document.new(xml)
-          end
-        end
-
-        def test_empty_value
-          xml = <<EOF
-<!DOCTYPE root [
-  <!ENTITY % a "">
-  <!ENTITY % b "%a;%a;%a;%a;%a;%a;%a;%a;%a;%a;%a;%a;%a;%a;%a;">
-  <!ENTITY % c "%b;%b;%b;%b;%b;%b;%b;%b;%b;%b;%b;%b;%b;%b;%b;">
-  <!ENTITY % d "%c;%c;%c;%c;%c;%c;%c;%c;%c;%c;%c;%c;%c;%c;%c;">
-  <!ENTITY % e "%d;%d;%d;%d;%d;%d;%d;%d;%d;%d;%d;%d;%d;%d;%d;">
-  <!ENTITY % f "%e;%e;%e;%e;%e;%e;%e;%e;%e;%e;%e;%e;%e;%e;%e;">
-  <!ENTITY % g "%f;%f;%f;%f;%f;%f;%f;%f;%f;%f;%f;%f;%f;%f;%f;">
-  <!ENTITY test "test %g;">
-]>
-<cd></cd>
-EOF
-
-          REXML::Document.new(xml)
-          REXML::Security.entity_expansion_limit = 90
-          assert_equal(90, REXML::Security.entity_expansion_limit)
-          assert_raise(REXML::ParseException) do
-            REXML::Document.new(xml)
-          end
-        end
-      end
     end
 
     def test_tag_in_cdata_with_not_ascii_only_but_ascii8bit_encoding_source
diff --git a/test/test_entity.rb b/test/test_entity.rb
index a2b262f7..afda6542 100644
--- a/test/test_entity.rb
+++ b/test/test_entity.rb
@@ -59,8 +59,7 @@ def test_parse_entity
 
     def test_constructor
       one = [ %q{<!ENTITY % YN '"Yes"'>},
-        %q{<!ENTITY % YN2 "Yes">},
-        %q{<!ENTITY WhatHeSaid "He said %YN;">},
+        %q{<!ENTITY WhatHeSaid 'He said "Yes"'>},
         '<!ENTITY open-hatch
                 SYSTEM "http://www.textuality.com/boilerplate/OpenHatch.xml">',
         '<!ENTITY open-hatch2
@@ -71,8 +70,7 @@ def test_constructor
                 NDATA gif>' ]
       source = %q{<!DOCTYPE foo [
         <!ENTITY % YN '"Yes"'>
-        <!ENTITY % YN2 "Yes">
-        <!ENTITY WhatHeSaid "He said %YN;">
+        <!ENTITY WhatHeSaid 'He said "Yes"'>
         <!ENTITY open-hatch
                 SYSTEM "http://www.textuality.com/boilerplate/OpenHatch.xml">
         <!ENTITY open-hatch2
@@ -104,6 +102,14 @@ def test_replace_entities
       assert_equal source, out
     end
 
+    def test_parameter_entity_reference_forbidden_in_internal_subset
+      source = '<!DOCTYPE root [ <!ENTITY % a "B" > <!ENTITY c "%a;" > ]><root/>'
+      exception = assert_raise(REXML::ParseException) do
+        REXML::Document.new(source)
+      end
+      assert_equal "Parameter Entity References forbidden in internal subset :%a;", exception.message
+    end
+
     def test_entity_string_limit
       template = '<!DOCTYPE bomb [ <!ENTITY a "^" > ]> <bomb>$</bomb>'
       len      = 5120 # 5k per entity
@@ -122,22 +128,6 @@ def test_entity_string_limit
       end
     end
 
-    def test_entity_string_limit_for_parameter_entity
-      template = '<!DOCTYPE bomb [ <!ENTITY % a "^" > <!ENTITY bomb "$" > ]><root/>'
-      len      = 5120 # 5k per entity
-      template.sub!(/\^/, "B" * len)
-
-      # 10k is OK
-      entities = '%a;' * 2 # 5k entity * 2 = 10k
-      REXML::Document.new(template.sub(/\$/, entities))
-
-      # above 10k explodes
-      entities = '%a;' * 3 # 5k entity * 2 = 15k
-      assert_raise(REXML::ParseException) do
-        REXML::Document.new(template.sub(/\$/, entities))
-      end
-    end
-
     def test_raw
       source = '<!DOCTYPE foo [
 <!ENTITY ent "replace">
@@ -161,7 +151,7 @@ def test_lazy_evaluation
     def test_entity_replacement
       source = %q{<!DOCTYPE foo [
       <!ENTITY % YN '"Yes"'>
-      <!ENTITY WhatHeSaid "He said %YN;">]>
+      <!ENTITY WhatHeSaid 'He said "Yes"'>]>
       <a>&WhatHeSaid;</a>}
 
       d = REXML::Document.new( source )