Add extra validations to prevent Signature wrapping attacks

There was a bug on the toolkit that made it vulnerable to a Signature wrapping attacks in the specific scenario where there was a Signature that referenced at the same time 2 elements (but past the scheme validator process since 1 of the element was inside the encrypted assertion. On this commit we added 3 new validators in order to avoid Signature wrapping attacks: - Extra validations at the validate_signed_elements method to check that the ref URIs and IDs are unique and consistent. - Validate the document (encrypted and decrypted version) against the scheme. - Use at validate_signature method the same logic than in xpath_from_signed_assertion (adding the 'id' => doc.signed_element_id condition).
SAML-Toolkits · Jun 16, 2016 · a571f52 · a571f52
1 parent 8453f12
commit a571f52
Showing 1 changed file with 47 additions and 5 deletions.
diff --git a/lib/onelogin/ruby-saml/response.rb b/lib/onelogin/ruby-saml/response.rb
@@ -337,8 +337,15 @@ def validate_success_status
       # @raise [ValidationError] if soft == false and validation fails 
       #
       def validate_structure
+        structure_error_msg = "Invalid SAML Response. Not match the saml-schema-protocol-2.0.xsd"
         unless valid_saml?(document, soft)
-          return append_error("Invalid SAML Response. Not match the saml-schema-protocol-2.0.xsd")
+          return append_error(structure_error_msg)
+        end
+
+        unless decrypted_document.nil?
+          unless valid_saml?(decrypted_document, soft)
+            return append_error(structure_error_msg)
+          end
         end
 
         true
@@ -434,11 +441,44 @@ def validate_signed_elements
           {"ds"=>DSIG}
         )
         signed_elements = []
+        seis = []
+        ids = []
         signature_nodes.each do |signature_node|
           signed_element = signature_node.parent.name
           if signed_element != 'Response' && signed_element != 'Assertion'
             return append_error("Found an unexpected Signature Element. SAML Response rejected")
           end
+
+          if signature_node.parent.attributes['ID'].nil?
+            return append_error("Signed Element must contain ID. SAML Response rejected")
+          end
+
+          id = signature_node.parent.attributes.get_attribute("ID").value
+          if ids.include?(id)
+            return append_error("Duplicated ID. SAML Response rejected")
+          end
+          ids.push(id)
+
+          # Check that reference URI matches the parent ID and no duplicate References or IDs
+          ref = REXML::XPath.first(signature_node, ".//ds:Reference", {"ds"=>DSIG})
+          if ref
+            uri = ref.attributes.get_attribute("URI")
+            if uri && !uri.value.empty?
+              sei = uri.value[1..-1]
+              id = signature_node.parent.attributes.get_attribute("ID").value
+
+              unless sei == id
+                return append_error("Found an invalid Signed Element. SAML Response rejected")
+              end
+
+              if seis.include?(sei)
+                return append_error("Duplicated Reference URI. SAML Response rejected")
+              end
+
+              seis.push(sei)
+            end
+          end
+
           signed_elements << signed_element
         end
 
@@ -614,8 +654,9 @@ def validate_signature
         # otherwise, review if the decrypted assertion contains a signature
         sig_elements = REXML::XPath.match(
           document,
-          "/p:Response/ds:Signature]",
-          { "p" => PROTOCOL, "ds" => DSIG }
+          "/p:Response[@ID=$id]/ds:Signature]",
+          { "p" => PROTOCOL, "ds" => DSIG },
+          { 'id' => document.signed_element_id }
         )
 
         use_original = sig_elements.size == 1 || decrypted_document.nil?
@@ -625,8 +666,9 @@ def validate_signature
         if sig_elements.nil? || sig_elements.size == 0
           sig_elements = REXML::XPath.match(
             doc,
-            "/p:Response/a:Assertion/ds:Signature",
-            {"p" => PROTOCOL, "a" => ASSERTION, "ds"=>DSIG}
+            "/p:Response/a:Assertion[@ID=$id]/ds:Signature",
+            {"p" => PROTOCOL, "a" => ASSERTION, "ds"=>DSIG},
+            { 'id' => doc.signed_element_id }
           )
         end