Merge master brunch

Author: pitbulk

README.md

@@ -1,5 +1,9 @@
 # Ruby SAML [![Build Status](https://secure.travis-ci.org/onelogin/ruby-saml.png)](http://travis-ci.org/onelogin/ruby-saml) [![Coverage Status](https://coveralls.io/repos/onelogin/ruby-saml/badge.svg?branch=master%0A)](https://coveralls.io/r/onelogin/ruby-saml?branch=master%0A) [![Gem Version](https://badge.fury.io/rb/ruby-saml.svg)](http://badge.fury.io/rb/ruby-saml)
 
+## Updating from 1.3.x to 1.4.X
+
+Version `1.4.0` is a recommended update for all Ruby SAML users as it includes security improvements.
+
 ## Updating from 1.2.x to 1.3.X
 
 Version `1.3.0` is a recommended update for all Ruby SAML users as it includes security fixes. It  adds security improvements in order to prevent Signature wrapping attacks. [CVE-2016-5697](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-5697)

changelog.md

@@ -1,5 +1,23 @@
 # RubySaml Changelog
 
+### 1.4.0 (October 13, 2016)
+* Several security improvements:
+  * Conditions element required and unique.
+  * AuthnStatement element required and unique.
+  * SPNameQualifier must math the SP EntityID
+  * Reject saml:Attribute element with same “Name” attribute
+  * Reject empty nameID
+  * Require Issuer element. (Must match IdP EntityID).
+  * Destination value can't be blank (if present must match ACS URL).
+  * Check that the EncryptedAssertion element only contains 1 Assertion element.
+
+* [#335](https://github.com/onelogin/ruby-saml/pull/335) Explicitly parse as XML and fix setting of Nokogiri options.
+* [#345](https://github.com/onelogin/ruby-saml/pull/345)Support multiple settings.auth_context
+* More tests to prevent XML Signature Wrapping
+* [#342](https://github.com/onelogin/ruby-saml/pull/342) Correct the usage of Mutex
+* [352](https://github.com/onelogin/ruby-saml/pull/352) Support multiple AttributeStatement tags
+
+
 ### 1.3.1 (July 10, 2016)
 * Fix response_test.rb of gem 1.3.0
 * Add reference to Security Guidelines

lib/onelogin/ruby-saml/response.rb

@@ -88,6 +88,23 @@ def name_id_format
 
       alias_method :nameid_format, :name_id_format
 
+      # @return [String] the NameID SPNameQualifier provided by the SAML response from the IdP.
+      #
+      def name_id_spnamequalifier
+        @name_id_spnamequalifier ||=
+          if name_id_node && name_id_node.attribute("SPNameQualifier")
+            name_id_node.attribute("SPNameQualifier").value
+          end
+      end
+
+      # @return [String] the NameID NameQualifier provided by the SAML response from the IdP.
+      #
+      def name_id_namequalifier
+        @name_id_namequalifier ||=
+          if name_id_node && name_id_node.attribute("NameQualifier")
+            name_id_node.attribute("NameQualifier").value
+          end
+      end
 
       # Gets the SessionIndex from the AuthnStatement.
       # Could be used to be stored in the local session in order
@@ -115,7 +132,8 @@ def sessionindex
       #    attributes['name']
       #
       # @return [Attributes] OneLogin::RubySaml::Attributes enumerable collection.
-      #      
+      # @raise [ValidationError] if there are 2+ Attribute with the same Name
+      #
       def attributes
         @attr_statements ||= begin
           attributes = Attributes.new
@@ -130,6 +148,11 @@ def attributes
               end
 
               name  = node.attributes["Name"]
+
+              if options[:check_duplicated_attributes] && attributes.include?(name)
+                raise ValidationError.new("Found an Attribute element with duplicated Name")
+              end
+
               values = node.elements.collect{|e|
                 if (e.elements.nil? || e.elements.size == 0)
                   # SAMLCore requires that nil AttributeValues MUST contain xsi:nil XML attribute set to "true" or "1"
@@ -175,25 +198,31 @@ def success?
       #
       def status_code
         @status_code ||= begin
-          node = REXML::XPath.first(
+          nodes = REXML::XPath.match(
             document,
             "/p:Response/p:Status/p:StatusCode",
             { "p" => PROTOCOL }
           )
-          node.attributes["Value"] if node && node.attributes
+          if nodes.size == 1
+            node = nodes[0]
+            node.attributes["Value"] if node && node.attributes
+          end
         end
       end
 
       # @return [String] the StatusMessage value from a SAML Response.
       #
       def status_message
         @status_message ||= begin
-          node = REXML::XPath.first(
+          nodes = REXML::XPath.match(
             document,
             "/p:Response/p:Status/p:StatusMessage",
             { "p" => PROTOCOL }
           )
-          node.text if node
+          if nodes.size == 1
+            node = nodes[0]
+            node.text if node
+          end
         end
       end
 
@@ -219,26 +248,6 @@ def not_on_or_after
         @not_on_or_after ||= parse_time(conditions, "NotOnOrAfter")
       end
 
-      # Gets the Issuers (from Response and Assertion).
-      # (returns the first node that matches the supplied xpath from the Response and from the Assertion)
-      # @return [Array] Array with the Issuers (REXML::Element)
-      #
-      def issuers
-        @issuers ||= begin
-          issuers = []
-          nodes = REXML::XPath.match(
-            document,
-            "/p:Response/a:Issuer",
-            { "p" => PROTOCOL, "a" => ASSERTION }
-          )
-          nodes += xpath_from_signed_assertion("/a:Issuer")
-          nodes.each do |node|
-            issuers << node.text if node.text
-          end
-          issuers.uniq
-        end
-      end
-
       # @return [String|nil] The InResponseTo attribute from the SAML Response.
       #
       def in_response_to
@@ -303,15 +312,19 @@ def validate(collect_errors = false)
           :validate_id,
           :validate_success_status,
           :validate_num_assertion,
+          :validate_no_duplicated_attributes,
           :validate_signed_elements,
           :validate_structure,
           :validate_in_response_to,
+          :validate_one_conditions,
           :validate_conditions,
+          :validate_one_authnstatement,
           :validate_audience,
           :validate_destination,
           :validate_issuer,
           :validate_session_expiration,
           :validate_subject_confirmation,
+          :validate_name_id,
           :validate_signature
         ]
 
@@ -400,6 +413,7 @@ def validate_version
       # @return [Boolean] True if the SAML Response contains one unique Assertion, otherwise False
       #
       def validate_num_assertion
+        error_msg = "SAML Response must contain 1 assertion"
         assertions = REXML::XPath.match(
           document,
           "//a:Assertion",
@@ -412,7 +426,35 @@ def validate_num_assertion
         )
 
         unless assertions.size + encrypted_assertions.size == 1
-          return append_error("SAML Response must contain 1 assertion")
+          return append_error(error_msg)
+        end
+
+        unless decrypted_document.nil?
+          assertions = REXML::XPath.match(
+            decrypted_document,
+            "//a:Assertion",
+            { "a" => ASSERTION }
+          )
+          unless assertions.size == 1
+            return append_error(error_msg)
+          end
+        end
+
+        true
+      end
+
+      # Validates that there are not duplicated attributes
+      # If fails, the error is added to the errors array
+      # @return [Boolean] True if there are no duplicated attribute elements, otherwise False if soft=True
+      # @raise [ValidationError] if soft == false and validation fails
+      #
+      def validate_no_duplicated_attributes
+        if options[:check_duplicated_attributes]
+          begin
+            attributes
+          rescue ValidationError => e
+            return append_error(e.message)
+          end
         end
 
         true
@@ -516,7 +558,14 @@ def validate_audience
       # @return [Boolean] True if there is a Destination element that matches the Consumer Service URL, otherwise False
       #
       def validate_destination
-        return true if destination.nil? || destination.empty? || settings.assertion_consumer_service_url.nil? || settings.assertion_consumer_service_url.empty?
+        return true if destination.nil?
+
+        if destination.empty?
+          error_msg = "The response has an empty Destination value"
+          return append_error(error_msg)
+        end
+
+        return true if settings.assertion_consumer_service_url.nil? || settings.assertion_consumer_service_url.empty?
 
         unless destination == settings.assertion_consumer_service_url
           error_msg = "The response was received at #{destination} instead of #{settings.assertion_consumer_service_url}"
@@ -526,6 +575,34 @@ def validate_destination
         true
       end
 
+      # Checks that the samlp:Response/saml:Assertion/saml:Conditions element exists and is unique.
+      # If fails, the error is added to the errors array
+      # @return [Boolean] True if there is a conditions element and is unique
+      #
+      def validate_one_conditions
+        conditions_nodes = xpath_from_signed_assertion('/a:Conditions')
+        unless conditions_nodes.size == 1
+          error_msg = "The Assertion must include one Conditions element"
+          return append_error(error_msg)
+        end
+
+        true
+      end
+
+      # Checks that the samlp:Response/saml:Assertion/saml:AuthnStatement element exists and is unique.
+      # If fails, the error is added to the errors array
+      # @return [Boolean] True if there is a authnstatement element and is unique
+      #
+      def validate_one_authnstatement
+        authnstatement_nodes = xpath_from_signed_assertion('/a:AuthnStatement')
+        unless authnstatement_nodes.size == 1
+          error_msg = "The Assertion must include one AuthnStatement element"
+          return append_error(error_msg)
+        end
+
+        true
+      end
+
       # Validates the Conditions. (If the response was initialized with the :skip_conditions option, this validation is skipped,
       # If the response was initialized with the :allowed_clock_drift option, the timing validations are relaxed by the allowed_clock_drift value)
       # @return [Boolean] True if satisfies the conditions, otherwise False if soft=True
@@ -558,6 +635,31 @@ def validate_conditions
       def validate_issuer
         return true if settings.idp_entity_id.nil?
 
+        issuers = []
+        issuer_response_nodes = REXML::XPath.match(
+          document,
+          "/p:Response/a:Issuer",
+          { "p" => PROTOCOL, "a" => ASSERTION }
+        )
+
+        unless issuer_response_nodes.size == 1
+          error_msg = "Issuer of the Response not found or multiple."
+          return append_error(error_msg)
+        end
+
+        doc = decrypted_document.nil? ? document : decrypted_document
+        issuer_assertion_nodes = xpath_from_signed_assertion("/a:Issuer")
+        unless issuer_assertion_nodes.size == 1
+          error_msg = "Issuer of the Assertion not found or multiple."
+          return append_error(error_msg)
+        end
+
+        nodes = issuer_response_nodes + issuer_assertion_nodes
+        nodes.each do |node|
+          issuers << node.text if node.text
+        end
+        issuers.uniq
+
         issuers.each do |issuer|
           unless URI.parse(issuer) == URI.parse(settings.idp_entity_id)
             error_msg = "Doesn't match the issuer, expected: <#{settings.idp_entity_id}>, but was: <#{issuer}>"
@@ -631,6 +733,27 @@ def validate_subject_confirmation
         true
       end
 
+      # Validates the NameID element
+      def validate_name_id
+        if name_id_node.nil?
+          if settings.security[:want_name_id]
+            return append_error("No NameID element found in the assertion of the Response")
+          end
+        else
+          if name_id.nil? || name_id.empty?
+            return append_error("An empty NameID value found")
+          end
+
+          unless settings.issuer.nil? || settings.issuer.empty? || name_id_spnamequalifier.nil? || name_id_spnamequalifier.empty?
+            if name_id_spnamequalifier != settings.issuer
+              return append_error("The SPNameQualifier value mistmatch the SP entityID value.")
+            end
+          end
+        end
+
+        true
+      end
+
       # Validates the Signature
       # @return [Boolean] True if not contains a Signature or if the Signature is valid, otherwise False if soft=True
       # @raise [ValidationError] if soft == false and validation fails

lib/onelogin/ruby-saml/settings.rb

@@ -155,6 +155,7 @@ def get_sp_key
           :logout_requests_signed   => false,
           :logout_responses_signed  => false,
           :want_assertions_signed   => false,
+          :want_name_id             => false,
           :metadata_signed          => false,
           :embed_sign               => false,
           :digest_method            => XMLSecurity::Document::SHA1,

lib/onelogin/ruby-saml/version.rb

@@ -1,5 +1,5 @@
 module OneLogin
   module RubySaml
-    VERSION = '1.3.1'
+    VERSION = '1.4.0'
   end
 end