Uploaded image for project: 'SX Spring Security Extension'
  1. SX Spring Security Extension
  2. SES-17

Error in subject validation in WebSSOProfileConsumerImpl

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Invalid
    • Affects Version/s: saml-1.0.0
    • Fix Version/s: saml-1.0.0.RC1
    • Component/s: saml
    • Labels:
      None

      Description

      On line 269 in WebSSOProfileConsumerImpl the Recipient of the Subject element in the message is supposed to be checked. However as well as checking for a correct Recipient value the binding used is also checked to be correct. This check is wrong as it checks the communicationProfileId which is never set in the message context and therefore we get a NullPointerException.

      From what I can deduce from the code and the SAML standard this binding check has nothing to do with the Subject in question and it is not required to be done, there only needs to be a check that the Recipient is correct. If it however is to be done shouldn't it use the inboundSAMLProtocol field (information found in the message) instead of the communicationProfileId?

      Expected code from revision 54:

      if (context.getInboundSAMLProtocol().equals(service.getBinding()) && service.getLocation().equals(data.getRecipient())) {

        Activity

        Hide
        vsch Vladimir Schäfer added a comment -

        Hi Mandus, it seems that when the bug was reported the revision 56 was already available and probably fixes the issue, but could you please verify that it really is so?

        The check for binding is "service.getBinding().equals(context.getCommunicationProfileId()",
        where service.getBinding() can never return null (in case metadata is correct), so it doesn't seem that this code could result in NullPointerException (rev. 56).

        This check is not explicitly required by specification, but in this module messages sent using both HTTP-POST and HTTP-Redirect bindings are consumed at the same location. In case one of the bindings would be disabled in metadata, without this check messages sent using either of the binding would still be consumed.

        The communicationProfileId is set during parsing of the SAML message from input in SAMLProcessorImpl, lines 131 and 134. Property inboundSAMLProtocol is used internally by OpenSAML for trust verifications and needs to be set to value "urn:oasis:names:tc:SAML:2.0:protocol".

        Show
        vsch Vladimir Schäfer added a comment - Hi Mandus, it seems that when the bug was reported the revision 56 was already available and probably fixes the issue, but could you please verify that it really is so? The check for binding is "service.getBinding().equals(context.getCommunicationProfileId()", where service.getBinding() can never return null (in case metadata is correct), so it doesn't seem that this code could result in NullPointerException (rev. 56). This check is not explicitly required by specification, but in this module messages sent using both HTTP-POST and HTTP-Redirect bindings are consumed at the same location. In case one of the bindings would be disabled in metadata, without this check messages sent using either of the binding would still be consumed. The communicationProfileId is set during parsing of the SAML message from input in SAMLProcessorImpl, lines 131 and 134. Property inboundSAMLProtocol is used internally by OpenSAML for trust verifications and needs to be set to value "urn:oasis:names:tc:SAML:2.0:protocol".
        Hide
        mel Mandus Elfving added a comment -

        Sorry, my bad, we have actually extended the OpenSAML library to support Artifact binding (deconding) and for doing so we had to rewrite some parts of the code for this module. In doing so we forgot to update SAMLProcessorImpl (still setting inboundSAMLProtocol). This issue may therefore be closed.

        By the way thanks for the great explanation!

        Show
        mel Mandus Elfving added a comment - Sorry, my bad, we have actually extended the OpenSAML library to support Artifact binding (deconding) and for doing so we had to rewrite some parts of the code for this module. In doing so we forgot to update SAMLProcessorImpl (still setting inboundSAMLProtocol). This issue may therefore be closed. By the way thanks for the great explanation!

          People

          • Assignee:
            vsch Vladimir Schäfer
            Reporter:
            mel Mandus Elfving
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development