SAML request encoding: on redirect binding, base64 encoded message contains CRLF

[junytse]

Describe the bug

When SAML SP generate a SAML request to redirect user to IDP, the base64 encoded message has \r\n inserted every 76 character. When redirect binding is chosen, this will result in %0A appear in the url. The below issue may occur:

1. Some IDP implementation cannot parse line feeds in the encoded message, and it will result in failure on validation;
2. For security reasons, some web server or firewall implementation will have CRLF filter to also filter out %0A in URL. When this happen, the message sent out is modified and signature validation will fail;
3. It violates SAML spec "http://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf" chapter 3.4.4.1 about redirect binding:

   3. The compressed data is subsequently base64-encoded according to the rules specified in IETF
      RFC 2045 [RFC2045]. Linefeeds or other whitespace MUST be removed from the result.

To Reproduce

1. Config your SAML IDP to support redirect binding and let SAML SP (using Spring Security SAML SP) to send using redirect binding;
2. Trigger authentication request from SAML SP in a browser with developer mode on;
3. Check the request that redirect from SP to IDP, url params contains SAML message, which has %0A.

Expected behavior
The BASE64 encoded message should not have line feeds for redirect binding.

Sample

https://www.example.org/app/logout?SAMLResponse=jVLBaoNAEP0V2buuq6vZDFEwsYFAemlCDr2Ejdk2lnVXnBXy%2BVVDaQol7WVgmPdm3nvMAmWjW9ja%0Ad9u7F4WtNai8a6MNwjTKSN8ZsBJrBCMbheAq2BXPW4iCENrOOltZTW6Ux2CJqDpXW0O8UqGrjRyb%0AjFycaxEo%2FTgrY2r03TAM1FU2rVZBZRvaKK2toXoSSbxNmZFNeYyEFGnIKp%2FP50ORs9gX8sR9Nj%2Bf%0A3lgaztK5GMDmy9XeZuTIZyVLlglbx0tWPBWcRatiWYikTNcFL8ORgNirjUEnjctIFDLhs9BnyT4K%0AIUkgTgMu0lfiHVSHk%2FzBGskXY1gwcbu7%2BP4bSP5XBiA4j6ns3YV2SuoG6YhY0LuzNw0t7Jx0Pf7s%0AVvasvIPUvXqsCCc07PqqUoiE5rcL30vpb%2F%2BSfwI%3D&RelayState=https%3A%2F%2Fjdennis-test.example.com%2Flogged-out.html&SigAlg=http%3A%2F%2Fwww.w3.org%2F2001%2F04%2Fxmldsig-more%23rsa-sha256&Signature=fhk%2BaH4gkG0qeTxHga42mtCMrA12ShVLLhmth%2Bk7d5vVkqsA0L2mAJ9wc8j8STqedrhQgGxzIfa1CpIjVK3vf27%2B06HvZWsuM9RLY3Q%2BhqtBKC3eTbvFD88uhYkKfnp1ws79psdzwo5%2FVY50MIoaersS5ac%2FX9AJm9cCRawuPpfpOXul13kwPEv0IQFrKkK7%2FOYWYcWMuMrY4aGwgmRmTIYubmUiZxkTmfw4Nc0hqe76%2BrJmLjPKIACJZZsvd4i2757yGNrREdE%2BfkEM5EeUcshRYOhTQn4uQY1EeA4Xn%2F2F%2BVjdqsPewNHAfA5QUC9mBkTQwIKUmiYs928sYkJUjA%3D%3D

Message encoding code was changed in this commit: 0b59e77
It fixed the base64 decoding issue. At the same time, however, it also change to use RFC2045 MIME encoder to encode outward messages.

// Saml2Utils.java

	static String samlEncode(byte[] b) {
		return Base64.getMimeEncoder().encodeToString(b);
	}

[jzheaux]

Thanks for the report, @junytse.

I think this can be corrected in OpenSamlAuthenticationRequestResolver and should likely also be corrected in OpenSamlLogoutRequestResolver and OpenSamlLogoutResponseResolver.

Are you able to provide a PR that removes the whitespace in accordance with the spec instructions?

[jzheaux]

In the meantime, you should be able to address the issue by providing your own Saml2AuthenticationRequestFactory, like so:

@Bean 
Saml2AuthenticationRequestFactory removesWhitespace() {
    Saml2AuthenticationRequestFactory delegate = new OpenSaml4AuthenticationRequestFactory();
    return new Saml2AuthenticationRequestFactory() {
        @Override
        public String createAuthenticationRequest(Saml2AuthenticationRequest request) {
             return delegate.createAuthenticationRequest(request);
        }

        @Override 
        public Saml2RedirectAuthenticationRequest createRedirectAuthenticationRequest(Saml2AuthenticationRequestContext context) {
            Saml2RedirectAuthenticationRequest request = delegate.createRedirectAuthenticationRequest(context);
            String withWhitespace = request.getSamlRequest();
            String withoutWhitespace = // ... remove whitespace ...
            return Saml2RedirectAuthenticationRequest.withAuthenticationRequestContext(context)
                .samlRequest(withoutWhitespace)
                .relayState(authentication.getRelayState())
                .sigAlg(authentication.getSigAlg())
                .signature(authentication.getSignature())
                .build();
        }
    }
}

[junytse]

Hi @jzheaux , the workaround using Saml2AuthenticationRequestFactory substitution could work on SSO. For SLO, the base64 encoding happens in the final class OpenSamlLogoutRequestResolver and cannot be modified in a usual way.

About fixing the issue, although we could use string replacements after Saml2Utils.samlEncode(...) in where we construct redirect binding messages, I think we could just make Saml2Utils.samlEncode(...) return strings without CRLF:

1. in redirect binding, encoded messages and signature should have no CRLF;
2. in POST binding, CRLF is optional for message encoding (saml-bindings-2.0-os.pdf 3.5.4):

   Messages are encoded for use with this binding by encoding the XML into an HTML form control and are
   transmitted using the HTTP POST method. A SAML protocol message is form-encoded by applying the
   base-64 encoding rules to the XML representation of the message and placing the result in a hidden form
   control within a form as defined by [HTML401] Section 17. The HTML document MUST adhere to the
   XHTML specification, [XHTML]. The base64-encoded value MAY be line-wrapped at a reasonable length
   in accordance with common practice.

Therefore, for SAML message encoding (POST, REDIRECT), it would be ok to always use RFC2045 base64 encoding without line feeds.

In Java implementation, Encoder.RFC4648 use the same alphabet table as Encoder.RFC2045 but encode without line feeds (in contrast, Encoder.RFC4648_URLSAFE use a slightly different alphabet table). Therefore, I'll recommend rolling back Saml2Utils.samlEncode(...) to use Base64.getEncoder() again, which returns Encoder.RFC4648.

In addition, if there are some other places need to use Encoder.RFC2045 we could add method to Saml2Utils to support both cases.

[jzheaux]

Okay, @junytse, I'm understanding better now. You are recommending that samlEncode change back to getEncoder(), but decoding continue doing getMimeEncoder(), is that right?

As for working around the other components, you should be able to use the same delegation strategy to create a new Saml2LogoutRequest/Saml2LogoutResponse:

@Bean 
Saml2LogoutRequestResolver removesWhitespaceFromLogoutRequests(RelyingPartyRegistrationResolver registrations) {
    Saml2LogoutRequestResolver delegate = new OpenSaml4LogoutRequestResolver(registrations);
    return (request) -> {
        Saml2LogoutRequest logoutRequest = delegate.resolve(request);
        if (logoutRequest == null) {
            return null;
        }
        RelyingPartyRegistration registration = registrations.resolve(request, logoutRequest.getRelyingPartyRegistrationId());
        String withWhitespace = logoutRequest.getSamlRequest();
        String withoutWhitespace = // ... remove whitespace
        return Saml2LogoutRequest.withRelyingPartyRegistration(registration)
                .samlRequest(withoutWhitespace)
                 // ...
                 .build();
    };
}

[junytse]

@jzheaux yes, no change is required for decoding, as Base64.getMimeDecoder() could decode BASE64 text with or without \r\n. I've submitted a PR about this change #11270.

Replacing OpenSaml4LogoutRequestResolver with a custom implementation should be an workaround on SLO. Thanks for you advice.