Fix vulnerability CVE-2017-11428. Process text of nodes properly, ignoring comments

Author: pitbulk

lib/onelogin/ruby-saml/logoutresponse.rb

@@ -62,7 +62,7 @@ def issuer
         @issuer ||= begin
           node = REXML::XPath.first(document, "/p:LogoutResponse/a:Issuer", { "p" => PROTOCOL, "a" => ASSERTION })
           node ||= REXML::XPath.first(document, "/p:LogoutResponse/a:Assertion/a:Issuer", { "p" => PROTOCOL, "a" => ASSERTION })
-          node.nil? ? nil : node.text
+          Utils.element_text(node)
         end
       end
 

lib/onelogin/ruby-saml/response.rb

@@ -37,7 +37,7 @@ def validate!
       def name_id
         @name_id ||= begin
           node = xpath_first_from_signed_assertion('/a:Subject/a:NameID')
-          node.nil? ? nil : node.text
+          Utils.element_text(node)
         end
       end
 
@@ -58,7 +58,7 @@ def attributes
 
           stmt_element.elements.each do |attr_element|
             name  = attr_element.attributes["Name"]
-            value = attr_element.elements.first.text
+            value = Utils.element_text(attr_element.elements.first)
 
             result[name] = value
           end
@@ -104,7 +104,7 @@ def issuer
         @issuer ||= begin
           node = REXML::XPath.first(document, "/p:Response/a:Issuer", { "p" => PROTOCOL, "a" => ASSERTION })
           node ||= xpath_first_from_signed_assertion('/a:Issuer')
-          node.nil? ? nil : node.text
+          Utils.element_text(node)
         end
       end
 

lib/onelogin/ruby-saml/utils.rb

@@ -0,0 +1,15 @@
+module OneLogin
+  module RubySaml
+
+    # SAML2 Auxiliary class
+    #
+    class Utils
+      # Given a REXML::Element instance, return the concatenation of all child text nodes. Assumes
+      # that there all children other than text nodes can be ignored (e.g. comments). If nil is
+      # passed, nil will be returned.
+      def self.element_text(element)
+        element.texts.join if element
+      end
+    end
+  end
+end

lib/ruby-saml.rb

@@ -4,6 +4,7 @@
 require 'onelogin/ruby-saml/logoutresponse'
 require 'onelogin/ruby-saml/response'
 require 'onelogin/ruby-saml/settings'
+require 'onelogin/ruby-saml/utils'
 require 'onelogin/ruby-saml/validation_error'
 require 'onelogin/ruby-saml/metadata'
 require 'onelogin/ruby-saml/version'

lib/xml_security.rb

@@ -30,6 +30,7 @@
 require "digest/sha1"
 require "digest/sha2"
 require "onelogin/ruby-saml/validation_error"
+require "onelogin/ruby-saml/utils"
 
 module XMLSecurity
 
@@ -48,7 +49,7 @@ def validate_document(idp_cert_fingerprint, soft = true)
       # get cert from response
       cert_element = REXML::XPath.first(self, "//ds:X509Certificate", { "ds"=>DSIG })
       raise OneLogin::RubySaml::ValidationError.new("Certificate element missing in response (ds:X509Certificate)") unless cert_element
-      base64_cert  = cert_element.text
+      base64_cert  = OneLogin::RubySaml::Utils.element_text(cert_element)
       cert_text    = Base64.decode64(base64_cert)
       cert         = OpenSSL::X509::Certificate.new(cert_text)
 
@@ -99,14 +100,14 @@ def validate_signature(base64_cert, soft = true)
         digest_algorithm              = algorithm(REXML::XPath.first(ref, "//ds:DigestMethod", 'ds' => DSIG))
 
         hash                          = digest_algorithm.digest(canon_hashed_element)
-        digest_value                  = Base64.decode64(REXML::XPath.first(ref, "//ds:DigestValue", {"ds"=>DSIG}).text)
+        digest_value                  = Base64.decode64(OneLogin::RubySaml::Utils.element_text(REXML::XPath.first(ref, "//ds:DigestValue", {"ds"=>DSIG})))
 
         unless digests_match?(hash, digest_value)
           return soft ? false : (raise OneLogin::RubySaml::ValidationError.new("Digest mismatch"))
         end
       end
 
-      base64_signature        = REXML::XPath.first(@sig_element, "//ds:SignatureValue", {"ds"=>DSIG}).text
+      base64_signature        = OneLogin::RubySaml::Utils.element_text(REXML::XPath.first(@sig_element, "//ds:SignatureValue", {"ds"=>DSIG}))
       signature               = Base64.decode64(base64_signature)
 
       # get certificate object

test/response_test.rb

@@ -151,6 +151,14 @@ class RubySamlTest < Test::Unit::TestCase
         response.settings = settings
         assert_raises(OneLogin::RubySaml::ValidationError, 'Digest mismatch'){ response.validate! }
       end
+
+      should "Prevent node text with comment (VU#475445) attack" do
+        response_doc = File.read(File.join(File.dirname(__FILE__), "responses", 'response_node_text_attack.xml.base64'))
+        response = OneLogin::RubySaml::Response.new(response_doc)
+
+        assert_equal "[email protected]", response.name_id
+        assert_equal "smith", response.attributes["surname"]
+      end
     end
 
     context "#name_id" do

test/responses/response_node_text_attack.xml.base64

@@ -0,0 +1 @@
+PHNhbWxwOlJlc3BvbnNlIHhtbG5zOnNhbWw9InVybjpvYXNpczpuYW1lczp0YzpTQU1MOjIuMDphc3NlcnRpb24iIHhtbG5zOnNhbWxwPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6cHJvdG9jb2wiIElEPSJHT1NBTUxSMTI5MDExNzQ1NzE3OTQiIFZlcnNpb249IjIuMCIgSXNzdWVJbnN0YW50PSIyMDEwLTExLTE4VDIxOjU3OjM3WiIgRGVzdGluYXRpb249IntyZWNpcGllbnR9Ij4NCiAgPHNhbWxwOlN0YXR1cz4NCiAgICA8c2FtbHA6U3RhdHVzQ29kZSBWYWx1ZT0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOnN0YXR1czpTdWNjZXNzIi8+PC9zYW1scDpTdGF0dXM+DQogIDxzYW1sOkFzc2VydGlvbiB4bWxuczp4cz0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEiIHhtbG5zOnhzaT0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEtaW5zdGFuY2UiIFZlcnNpb249IjIuMCIgSUQ9InBmeGE0NjU3NGRmLWIzYjAtYTA2YS0yM2M4LTYzNjQxMzE5ODc3MiIgSXNzdWVJbnN0YW50PSIyMDEwLTExLTE4VDIxOjU3OjM3WiI+DQogICAgPHNhbWw6SXNzdWVyPmh0dHBzOi8vYXBwLm9uZWxvZ2luLmNvbS9zYW1sL21ldGFkYXRhLzEzNTkwPC9zYW1sOklzc3Vlcj4NCiAgICA8ZHM6U2lnbmF0dXJlIHhtbG5zOmRzPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwLzA5L3htbGRzaWcjIj4NCiAgICAgIDxkczpTaWduZWRJbmZvPg0KICAgICAgICA8ZHM6Q2Fub25pY2FsaXphdGlvbk1ldGhvZCBBbGdvcml0aG09Imh0dHA6Ly93d3cudzMub3JnLzIwMDEvMTAveG1sLWV4Yy1jMTRuIyIvPg0KICAgICAgICA8ZHM6U2lnbmF0dXJlTWV0aG9kIEFsZ29yaXRobT0iaHR0cDovL3d3dy53My5vcmcvMjAwMC8wOS94bWxkc2lnI3JzYS1zaGExIi8+DQogICAgICAgIDxkczpSZWZlcmVuY2UgVVJJPSIjcGZ4YTQ2NTc0ZGYtYjNiMC1hMDZhLTIzYzgtNjM2NDEzMTk4NzcyIj4NCiAgICAgICAgICA8ZHM6VHJhbnNmb3Jtcz4NCiAgICAgICAgICAgIDxkczpUcmFuc2Zvcm0gQWxnb3JpdGhtPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwLzA5L3htbGRzaWcjZW52ZWxvcGVkLXNpZ25hdHVyZSIvPg0KICAgICAgICAgICAgPGRzOlRyYW5zZm9ybSBBbGdvcml0aG09Imh0dHA6Ly93d3cudzMub3JnLzIwMDEvMTAveG1sLWV4Yy1jMTRuIyIvPg0KICAgICAgICAgIDwvZHM6VHJhbnNmb3Jtcz4NCiAgICAgICAgICA8ZHM6RGlnZXN0TWV0aG9kIEFsZ29yaXRobT0iaHR0cDovL3d3dy53My5vcmcvMjAwMC8wOS94bWxkc2lnI3NoYTEiLz4NCiAgICAgICAgICA8ZHM6RGlnZXN0VmFsdWU+cEpRN01TL2VrNEtSUldHbXYvSDQzUmVIWU1zPTwvZHM6RGlnZXN0VmFsdWU+DQogICAgICAgIDwvZHM6UmVmZXJlbmNlPg0KICAgICAgPC9kczpTaWduZWRJbmZvPg0KICAgICAgPGRzOlNpZ25hdHVyZVZhbHVlPnlpdmVLY1BkRHB1RE5qNnNoclEzQUJ3ci9jQTNDcnlEMnBoRy94TFpzektXeFU1L21sYUt0OGV3YlpPZEtLdnRPczJwSEJ5NUR1YTNrOTRBRnp4R3llbDVnT293bW95WEpyQU9ya1BPMHZsaTFWOG8zaFBQVVp3UmdTWDZROXBTMUNxUWdoS2lFYXNSeXlscXFKVWFQWXptT3pPRTgvWGxNa3dpV21PMD08L2RzOlNpZ25hdHVyZVZhbHVlPg0KICAgICAgPGRzOktleUluZm8+DQogICAgICAgIDxkczpYNTA5RGF0YT4NCiAgICAgICAgICA8ZHM6WDUwOUNlcnRpZmljYXRlPk1JSUJyVENDQWFHZ0F3SUJBZ0lCQVRBREJnRUFNR2N4Q3pBSkJnTlZCQVlUQWxWVE1STXdFUVlEVlFRSURBcERZV3hwWm05eWJtbGhNUlV3RXdZRFZRUUhEQXhUWVc1MFlTQk5iMjVwWTJFeEVUQVBCZ05WQkFvTUNFOXVaVXh2WjJsdU1Sa3dGd1lEVlFRRERCQmhjSEF1YjI1bGJHOW5hVzR1WTI5dE1CNFhEVEV3TURNd09UQTVOVGcwTlZvWERURTFNRE13T1RBNU5UZzBOVm93WnpFTE1Ba0dBMVVFQmhNQ1ZWTXhFekFSQmdOVkJBZ01Da05oYkdsbWIzSnVhV0V4RlRBVEJnTlZCQWNNREZOaGJuUmhJRTF2Ym1sallURVJNQThHQTFVRUNnd0lUMjVsVEc5bmFXNHhHVEFYQmdOVkJBTU1FR0Z3Y0M1dmJtVnNiMmRwYmk1amIyMHdnWjh3RFFZSktvWklodmNOQVFFQkJRQURnWTBBTUlHSkFvR0JBT2pTdTFmalB5OGQ1dzRReUwxemQ0aEl3MU1ra2ZmNFdZL1RMRzhPWmtVNVlUU1dtbUhQRDVrdllINXVvWFMvNnFRODFxWHBSMndWOENUb3daSlVMZzA5ZGRSZFJuOFFzcWoxRnlPQzVzbEUzeTJiWjJvRnVhNzJvZi80OWZwdWpuRlQ2S25RNjFDQk1xbERvVFFxT1Q2MnZHSjhuUDZNWld2QTZzeHF1ZDVBZ01CQUFFd0F3WUJBQU1CQUE9PTwvZHM6WDUwOUNlcnRpZmljYXRlPg0KICAgICAgICA8L2RzOlg1MDlEYXRhPg0KICAgICAgPC9kczpLZXlJbmZvPg0KICAgIDwvZHM6U2lnbmF0dXJlPg0KICAgIDxzYW1sOlN1YmplY3Q+DQogICAgICA8c2FtbDpOYW1lSUQgRm9ybWF0PSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoxLjE6bmFtZWlkLWZvcm1hdDplbWFpbEFkZHJlc3MiPnN1cHBvcnQ8IS0tIGF0dGFjayEgLS0+QG9uZWxvZ2luLmNvbTwvc2FtbDpOYW1lSUQ+DQogICAgICA8c2FtbDpTdWJqZWN0Q29uZmlybWF0aW9uIE1ldGhvZD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOmNtOmJlYXJlciI+DQogICAgICAgIDxzYW1sOlN1YmplY3RDb25maXJtYXRpb25EYXRhIE5vdE9uT3JBZnRlcj0iMjAxMC0xMS0xOFQyMjowMjozN1oiIFJlY2lwaWVudD0ie3JlY2lwaWVudH0iLz48L3NhbWw6U3ViamVjdENvbmZpcm1hdGlvbj4NCiAgICA8L3NhbWw6U3ViamVjdD4NCiAgICA8c2FtbDpDb25kaXRpb25zIE5vdEJlZm9yZT0iMjAxMC0xMS0xOFQyMTo1MjozN1oiIE5vdE9uT3JBZnRlcj0iMjAxMC0xMS0xOFQyMjowMjozN1oiPg0KICAgICAgPHNhbWw6QXVkaWVuY2VSZXN0cmljdGlvbj4NCiAgICAgICAgPHNhbWw6QXVkaWVuY2U+e2F1ZGllbmNlfTwvc2FtbDpBdWRpZW5jZT4NCiAgICAgIDwvc2FtbDpBdWRpZW5jZVJlc3RyaWN0aW9uPg0KICAgIDwvc2FtbDpDb25kaXRpb25zPg0KICAgIDxzYW1sOkF1dGhuU3RhdGVtZW50IEF1dGhuSW5zdGFudD0iMjAxMC0xMS0xOFQyMTo1NzozN1oiIFNlc3Npb25Ob3RPbk9yQWZ0ZXI9IjIwMTAtMTEtMTlUMjE6NTc6MzdaIiBTZXNzaW9uSW5kZXg9Il81MzFjMzJkMjgzYmRmZjdlMDRlNDg3YmNkYmM0ZGQ4ZCI+DQogICAgICA8c2FtbDpBdXRobkNvbnRleHQ+DQogICAgICAgIDxzYW1sOkF1dGhuQ29udGV4dENsYXNzUmVmPnVybjpvYXNpczpuYW1lczp0YzpTQU1MOjIuMDphYzpjbGFzc2VzOlBhc3N3b3JkPC9zYW1sOkF1dGhuQ29udGV4dENsYXNzUmVmPg0KICAgICAgPC9zYW1sOkF1dGhuQ29udGV4dD4NCiAgICA8L3NhbWw6QXV0aG5TdGF0ZW1lbnQ+DQogICAgPHNhbWw6QXR0cmlidXRlU3RhdGVtZW50Pg0KICAgICAgPHNhbWw6QXR0cmlidXRlIE5hbWU9InN1cm5hbWUiPg0KICAgICAgICA8c2FtbDpBdHRyaWJ1dGVWYWx1ZSB4bWxuczp4cz0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEiIHhtbG5zOnhzaT0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEtaW5zdGFuY2UiIHhzaTp0eXBlPSJ4czpzdHJpbmciPnM8IS0tIGF0dGFjayEgLS0+bWl0aDwvc2FtbDpBdHRyaWJ1dGVWYWx1ZT4NCiAgICAgIDwvc2FtbDpBdHRyaWJ1dGU+DQogICAgICA8c2FtbDpBdHRyaWJ1dGUgTmFtZT0iYW5vdGhlcl92YWx1ZSI+DQogICAgICAgIDxzYW1sOkF0dHJpYnV0ZVZhbHVlIHhtbG5zOnhzPSJodHRwOi8vd3d3LnczLm9yZy8yMDAxL1hNTFNjaGVtYSIgeG1sbnM6eHNpPSJodHRwOi8vd3d3LnczLm9yZy8yMDAxL1hNTFNjaGVtYS1pbnN0YW5jZSIgeHNpOnR5cGU9InhzOnN0cmluZyI+dmFsdWUxPC9zYW1sOkF0dHJpYnV0ZVZhbHVlPg0KICAgICAgICA8c2FtbDpBdHRyaWJ1dGVWYWx1ZSB4bWxuczp4cz0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEiIHhtbG5zOnhzaT0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEtaW5zdGFuY2UiIHhzaTp0eXBlPSJ4czpzdHJpbmciPnZhbHVlMjwvc2FtbDpBdHRyaWJ1dGVWYWx1ZT4NCiAgICAgIDwvc2FtbDpBdHRyaWJ1dGU+DQogICAgICA8c2FtbDpBdHRyaWJ1dGUgTmFtZT0icm9sZSI+DQogICAgICAgIDxzYW1sOkF0dHJpYnV0ZVZhbHVlIHhtbG5zOnhzPSJodHRwOi8vd3d3LnczLm9yZy8yMDAxL1hNTFNjaGVtYSIgeG1sbnM6eHNpPSJodHRwOi8vd3d3LnczLm9yZy8yMDAxL1hNTFNjaGVtYS1pbnN0YW5jZSIgeHNpOnR5cGU9InhzOnN0cmluZyI+cm9sZTE8L3NhbWw6QXR0cmlidXRlVmFsdWU+DQogICAgICA8L3NhbWw6QXR0cmlidXRlPg0KICAgIDwvc2FtbDpBdHRyaWJ1dGVTdGF0ZW1lbnQ+DQogICAgPHNhbWw6QXR0cmlidXRlU3RhdGVtZW50Pg0KICAgICAgPHNhbWw6QXR0cmlidXRlIE5hbWU9ImZpcnN0bmFtZSI+DQogICAgICAgIDxzYW1sOkF0dHJpYnV0ZVZhbHVlIHhtbG5zOnhzPSJodHRwOi8vd3d3LnczLm9yZy8yMDAxL1hNTFNjaGVtYSIgeG1sbnM6eHNpPSJodHRwOi8vd3d3LnczLm9yZy8yMDAxL1hNTFNjaGVtYS1pbnN0YW5jZSIgeHNpOnR5cGU9InhzOnN0cmluZyI+Ym9iPC9zYW1sOkF0dHJpYnV0ZVZhbHVlPg0KICAgICAgPC9zYW1sOkF0dHJpYnV0ZT4gIA0KICAgICAgPHNhbWw6QXR0cmlidXRlIE5hbWU9ImF0dHJpYnV0ZV93aXRoX25pbF92YWx1ZSI+DQogICAgICAgIDxzYW1sOkF0dHJpYnV0ZVZhbHVlIHhtbG5zOnhzPSJodHRwOi8vd3d3LnczLm9yZy8yMDAxL1hNTFNjaGVtYSIgeG1sbnM6eHNpPSJodHRwOi8vd3d3LnczLm9yZy8yMDAxL1hNTFNjaGVtYS1pbnN0YW5jZSIgeHNpOm5pbD0idHJ1ZSIvPg0KICAgICAgPC9zYW1sOkF0dHJpYnV0ZT4NCiAgICAgIDxzYW1sOkF0dHJpYnV0ZSBOYW1lPSJhdHRyaWJ1dGVfd2l0aF9uaWxzX2FuZF9lbXB0eV9zdHJpbmdzIj4NCiAgICAgICAgPHNhbWw6QXR0cmlidXRlVmFsdWUvPg0KICAgICAgICA8c2FtbDpBdHRyaWJ1dGVWYWx1ZT52YWx1ZVByZXNlbnQ8L3NhbWw6QXR0cmlidXRlVmFsdWU+DQogICAgICAgIDxzYW1sOkF0dHJpYnV0ZVZhbHVlIHhtbG5zOnhzPSJodHRwOi8vd3d3LnczLm9yZy8yMDAxL1hNTFNjaGVtYSIgeG1sbnM6eHNpPSJodHRwOi8vd3d3LnczLm9yZy8yMDAxL1hNTFNjaGVtYS1pbnN0YW5jZSIgeHNpOm5pbD0idHJ1ZSIvPg0KICAgICAgICA8c2FtbDpBdHRyaWJ1dGVWYWx1ZSB4bWxuczp4cz0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEiIHhtbG5zOnhzaT0iaHR0cDovL3d3dy53My5vcmcvMjAwMS9YTUxTY2hlbWEtaW5zdGFuY2UiIHhzaTpuaWw9IjEiLz4NCiAgICAgIDwvc2FtbDpBdHRyaWJ1dGU+DQogICAgPC9zYW1sOkF0dHJpYnV0ZVN0YXRlbWVudD4NCiAgPC9zYW1sOkFzc2VydGlvbj4NCjwvc2FtbHA6UmVzcG9uc2U+