Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request parameterizes the Entra IDP URL, allowing it to be configured instead of being hardcoded. However, the current implementation introduces significant security risks, including the use of unencrypted HTTP which can lead to sensitive bearer token leakage, and vulnerability to OData injection due to improper escaping of user-supplied input in filter constructions. Additionally, a critical type safety issue exists where a constant string is assigned to a non-constant pointer, potentially causing a crash. These issues should be addressed by enforcing HTTPS, implementing proper sanitization for OData filters, and safely duplicating strings to resolve the type safety problem.
src/oidc_child/oidc_child_id.c
Outdated
| base_url = default_base_url; | ||
| } |
There was a problem hiding this comment.
Assigning a const char * (string literal) to a char * is a C language violation that can lead to undefined behavior, including crashes, if the string is later modified. To ensure type safety, you should duplicate the constant string into a mutable buffer and check for allocation failure.
base_url = talloc_strdup(mem_ctx, default_base_url);
if (base_url == NULL) {
return ENOMEM;
}
}| if (*base_url == '\0' || strncasecmp(base_url, "http", 4) != 0) { | ||
| DEBUG(SSSDBG_OP_FAILURE, "Colon supplied in %s but no url supplied.\n", | ||
| idp_type); | ||
| return EINVAL; | ||
| } |
There was a problem hiding this comment.
The base_url override allows the use of unencrypted HTTP (e.g., entra_id:http://...). When an HTTP URL is provided, the bearer_token (a sensitive credential) is transmitted in cleartext over the network. For a cloud service like Entra ID (Microsoft Graph), there is rarely a legitimate reason to use plain HTTP, and allowing it exposes the system to credential theft via man-in-the-middle (MITM) attacks. The code should enforce the use of HTTPS for the base URL.
| case GET_USER: | ||
| case GET_USER_GROUPS: | ||
| uri = talloc_asprintf(rest_ctx, "https://graph.microsoft.com/v1.0/users?$filter=%s", filter_enc); | ||
| uri = talloc_asprintf(rest_ctx, "%s/users?$filter=%s", base_url, filter_enc); |
There was a problem hiding this comment.
The input variable, which contains user-supplied data (such as a username or group name), is used to construct OData filter strings (e.g., at lines 84, 87, 95, 102, 105) without any sanitization or escaping of single quotes. Although the resulting filter is later URL-encoded, the OData logic itself remains vulnerable. An attacker can provide a crafted input containing single quotes (e.g., user') or (1 eq 1) to break out of the filter's quoting and inject arbitrary OData expressions. This could allow an attacker to manipulate identity lookups, potentially leading to unauthorized access or privilege escalation if the system relies on the lookup results for authorization decisions.
|
Is this ready for review or do you plan to work more on this? |
|
pretty much ready besides some docs additions, pending CI fixes |
alexey-tikhonov
added
Waiting for review
no-backport
pbrezina
left a comment
pbrezina
left a comment
There was a problem hiding this comment.
Hi, would you mind refactoring the code a bit? The keycloak and entraid functions currently take idp_type parameter that was used only in keycloak to get the base url, the same you do in entraid_lookup now.
It would be nice if you could write a separate function to parse the idp_type into base_url, call this function from oidc_get_id and provide base_url as parameter to the lookup functions (NULL means use default value (entryid) or error out (keycloak)).
| "https://graph.microsoft.com/v1.0/users/%s/getMemberGroups", | ||
| obj_id); | ||
| uri = talloc_asprintf(rest_ctx, "%s/users/%s/getMemberGroups", | ||
| base_url, obj_id); |
4 participants
fixes #8446