Problem/Motivation

Checkmarx flagged unsafe reflection in the SAML SP module. User input is used directly to call callbacks, allowing attackers to inject arbitrary parameters into reflection methods. This could lead to critical security vulnerabilities.

Steps to reproduce

  • Send a request to the SAML SP endpoint with a callback parameter in the request.
  • The code executes $callback($is_valid, $saml_response, $idp); directly.
  • Any callable passed by the user is executed, demonstrating unsafe reflection.

Vulnerable code reference:

$callback = $request['callback'];
$result = $callback($is_valid, $saml_response, $idp);

Proposed resolution

  • Implement a whitelist of allowed callbacks to ensure only safe, predefined functions are executed.
  • Alternatively, remove user-controlled callbacks entirely if not necessary.

Remaining tasks

  • Review all callback usages in the module.
  • Add automated tests to prevent injection of arbitrary callables.
  • Update documentation if callbacks are restricted.

User interface changes

None. This is a backend security fix.

API changes

Potential change in the behavior of callback handling; any code relying on dynamic callbacks may need adjustment.

Data model changes

None.

Comments

fazni created an issue. See original summary.

manarth’s picture

manarth commented
Version: 4.3.4 » 8.x-3.x-dev

The relevant code is present in the 8.x-3.x branch.

There is a significant degree of behaviour indirection in that code, and the variable names may have triggered the Checkmarx threshold.

Although the code refers to $request['callback'], the callback parameter is not provided by an external/untrusted source as a request parameter - it's a property of a tracked request, which is predefined in the module.

For example:

  public function initiate(Idp $idp) {
    // …
    $callback = 'saml_sp_drupal_login__saml_authenticate';
    // …
    $return = saml_sp_start($idp, $callback, $forceAuthn);

On the face of it, this doesn't immediately appear to be a security issue, although potentially an area for improvement to avoid reports from Checkmarx and improve the developer experience/understanding.

jproctor’s picture

jproctor
they/them
Primary language English
commented
Version: 8.x-3.x-dev » 4.x-dev
Category: Support request » Task
Status: Active » Closed (works as designed)

The code is still present in 4.x but, as @manarth notes, does not actually expose the parameter. I don’t see a good way to simplify without a significant overhaul of the structure of the module.

In the future, if you have a security concern about this or another module, please follow the procedure for reporting security issues.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.