spring-projects / spring-projects/spring-security

Invalid order of query paramerter in SAMLResponse for an AP-Initiated SLO

Open
#18,324 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

status: waiting-for-triage type: bug
Dominant language
Java
Stars
9.6k
Forks
6.3k
Avg merge
2d 11h
Merged PRs (30d)
52

Description

Describe the bug
In an AP-Initiated SLO, the signed response for the HTTP redirect binding with relaystate is incorrect. The AP fails to validate the signature.

The relying party, after validating the logout request in the Saml2LogoutRequestFilter, delegates the task of resolving the response to BaseOpenSamlLogoutResponseResolver.

Here, the response XML is constructed. If the binging is an HTTP POST, the LogoutResponse is signed; otherwise, the XML is deflated-encoded, and the signing parameters (HashMap) are signed. If the LogoutRequest has a relay state, the map of signing parameters includes it.

OpenSaml4Template.OpenSaml4SignatureConfigurer.sign takes a Map and then constructs the query string, and then signs it. Here, the actual parameter order that Spring Security uses for HTTP-Redirect binding is incorrect. (basically loses the order because of HashMap).

To Reproduce
Run the below sample code with jbang

jbang run TestSigningMethod.java

Expected behavior
A signed SLO SAML Response should have the query parameter in the correct order.

Sample


///usr/bin/env jbang "$0" "$@" ; exit $?

//DEPS org.springframework:spring-web:6.0.10


import org.springframework.web.util.UriComponentsBuilder;
import org.springframework.web.util.UriUtils;

import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.Map;

public class TestSigningMethod {

	Map<String, String> components = new LinkedHashMap<>();

	public static void main(String[] args) {
                // Simulating the BaseOpenSamlLogoutResponseResolver.resolve()
		Map<String, String> params = new HashMap<>();
		TestSigningMethod tsm = new TestSigningMethod();
		params.put("SAMLResponse", "value1");
		params.put("RelayState", "value2");
		tsm.sign(params);

	}
        
        // simulating OpenSaml4Template.OpenSaml4SignatureConfigurer.sign()
	public void sign(Map<String, String> params) {
		components.putAll(params);
		UriComponentsBuilder builder = UriComponentsBuilder.newInstance();
		for (Map.Entry<String, String> component : this.components.entrySet()) {
			builder.queryParam(component.getKey(),
					UriUtils.encode(component.getValue(), StandardCharsets.ISO_8859_1));
		}
		String queryString = builder.build(true).toString().substring(1);
		System.out.println(queryString);
	}
}

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with BaseOpenSamlLogoutResponseResolver and OpenSaml4Template.OpenSaml4SignatureConfigurer.sign, then reproduce the ordering problem with TestSigningMethod.java using jbang. Trace how SAMLResponse and RelayState are collected for the HTTP-Redirect binding. Done when the signed SLO response preserves the required query-parameter order and the AP can validate its signature.

Written by the indexing model from the issue text.

Assessment

Tech stack
java, spring
Domain
security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.