Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent colon character in group names for CustomAWSSAMLGroupMapper #202

Closed
bburky opened this issue Aug 26, 2024 · 0 comments · Fixed by #205
Closed

Prevent colon character in group names for CustomAWSSAMLGroupMapper #202

bburky opened this issue Aug 26, 2024 · 0 comments · Fixed by #205
Assignees
Labels
possible-bug Something may not be working

Comments

@bburky
Copy link
Member

bburky commented Aug 26, 2024

#185 added support for a custom mapper supporting AWS. This mapper joins group names with a colon, :, character.

This code is potentially vulnerable to group name injection if any groups contain a : in their name.

To fix the mapper, throw an error or drop group names containing a :

if (!groups.isEmpty()) {
// Use Keycloak's ModelToRepresentation utility to get the full group paths
List<String> groupPaths = groups.stream()
.map(ModelToRepresentation::buildGroupPath)
.collect(Collectors.toList());
// Concatenate the group paths into a single string separated by colons
String groupsString = String.join(":", groupPaths);
// Create a new SAML attribute
AttributeType attribute = new AttributeType(mappingModel.getConfig().get(AttributeStatementHelper.SAML_ATTRIBUTE_NAME));
attribute.setNameFormat(JBossSAMLURIConstants.ATTRIBUTE_FORMAT_BASIC.get());
attribute.addAttributeValue(groupsString);
attributeStatement.addAttribute(new ASTChoiceType(attribute));
}

@bburky bburky added the possible-bug Something may not be working label Aug 26, 2024
@UnicornChance UnicornChance linked a pull request Aug 28, 2024 that will close this issue
5 tasks
@UnicornChance UnicornChance self-assigned this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
possible-bug Something may not be working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@bburky @UnicornChance and others