This module has an open redirect vulnerability in the 7.x branch:

You can see this vulnerability by:
1. Enabling the module
2. As anonymous user go to /saml/sls?RelayState=http%3A%2F%2Fexample.com
3. You are redirected to an attacker supplied site, example.com here

The 7.x branch does not have a stable release, so we can fix it publicly. The 8.x versions are not affected.

CommentFileSizeAuthor
#8 interdiff-3065889-5-7.txt1.07 KBroderik
#5 samlauth-fix_open_redirect-170383-5.patch1.44 KBnironan
#2 170383-2.patch1.23 KBklausi

Comments

klausi created an issue. See original summary.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Active » Needs review
StatusFileSize
new 170383-2.patch1.23 KB

Uploading the latest patch we discussed in the private security issue.

Author of the patch: smfsh

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work 
  1. +++ b/samlauth.module
@@ -239,13 +239,31 @@ function samlauth_logout() {
+    $url = samlauth_get_saml2_auth()->processSLO(FALSE, NULL, FALSE, NULL, TRUE);
+    // Usually we don't get any URL returned. The case in which we do, seems to
+    // be something like IDP-initiated logout. Therefore we won't do further
+    // processing.
+    if (!$url) {

    This is not enough - we need to check if there were any errors before we trust the return value. 

  2. +++ b/samlauth.module
@@ -239,13 +239,31 @@ function samlauth_logout() {
+        $url = ('<front>');

    parenthesis should be removed. 

  3. +++ b/samlauth.module
@@ -239,13 +239,31 @@ function samlauth_logout() {
+  if (user_is_logged_in()) {
+    module_load_include('pages.inc', 'user');
+    user_logout_current_user();
   }

    this is not ideal because then this is vulnerable to logout CSRF attacks. We should only log out the user if the processed SAML request was valid.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

Oh right, I failed to spot that in first review. To say this in terms of comparing to the 8.x-2/3.x version: that first does Auth::processSLO() and

* if the function returns normally: calls Auth::getErrors()

* if not: catches the exception

And if either an exception is caught or there are errors: does not log out.

7.x should do the same.

nironan’s picture

nironan commented
Status: Needs work » Needs review
StatusFileSize
new samlauth-fix_open_redirect-170383-5.patch1.44 KB

Here's an attempt to fix the issue as suggested

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

This looks good to me. (I haven't tested, myself: I've never run D7 samlauth. But from reading the patch, it's clear that the only difference between the first and second patch is "being conservative with logging out if errors were encountered / reported".)

  • roderik committed 90ff41e on 7.x-1.x authored by nironan 
    Issue #3065889 by smfsh, nironan, klausi, roderik: Actually implement...
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Fixed
StatusFileSize
new interdiff-3065889-5-7.txt1.07 KB

It seems I'm the 7.x maintainer too now. Oh well... :)

Thanks again for this.

I did make one change, because in the meantime I learnt how SLS actually works (#3043704-9: Make user logout more robust), which means the returned $url can be empty. So I added a " ?: '' ". And changed some comments.

  • roderik committed 6e1d6e2 on 7.x-1.x 
    #3065889 fix a mistake I introduced.
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

Oh, no. That wasn't as intended. $url needs to stay empty there.

But if there was an error and/or no RelayState was set, then it needs to be set to <front>

I unfixed my 'fix' and refixed it in another way... hoping it's covered now...

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

Oh.

'' means the same as '<front>' in drupal_goto(). Which means the patch in #5 is good. (We'd just pass either value into drupal_goto(), depending on whether there were any errors.)

Oh well. Now we're passing a consistent value ¯\_(ツ)_/¯

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.