There is a serious security issue with the Drupal integration in simplesamlphp

[GoogleCodeExporter]

There is a serious security issue with the Drupal integration in simplesamlphp.
The function "getUser()" which returns the attributes for the current user 
picks the Drupal User ID "uid" from a Cookie which obviously passes through the 
client machine. There is nothing stopping a suitable engineered client from 
changing the uid in the Cookie to be any user. Thus the client could acquire 
single sign on credentials for any Drupal user whatsoever by manipulating the 
Cookie so that when a single sign on transaction is started the client machine 
gets logged in to the requesting server as that other user rather that as the 
currently logged in Drupal user.

There is a very straightforward alternative which is to fetch the credentials 
for the currently logged in Drupal user in 
simplesamlphp/modules/drupalauth/lib/Auth/Source/External.php ...
Pseudo code...
global $user;
...
$drupaluid = $user->uid;
...
$drupaluser = user_load($drupaluid);

I have also added "chdir(DRUPAL_ROOT)" in two places as, at least in Drupal 6, 
Drupal generates errors if it is called with the wrong working directory. 
"chdir($a)" changes back the working directory after the call.

The code to fix the above issues is in the attached new version of 
simplesamlphp/modules/drupalauth/lib/Auth/Source/External.php and, as an 
alternative, a patch "External.php.patch" to apply the change. 

Alan Barrett

Original issue reported on code.google.com by alanabar...@gmail.com on 4 Nov 2013 at 4:35

Attachments:

• External.php
• External.php.patch

[GoogleCodeExporter]

I will review this and get this corrected on or after Dec 11th. Unfortunately 
that's the fasted I can get to this.

In terms of scope this vulnerability seems to be limited to installations that 
use the drupalauth:External authsource. Installations that use the 
drupalauth:UserPass do not appear to be affected.

Original comment by smoit...@gmail.com on 4 Dec 2013 at 1:16

• Changed state: Accepted
• Added labels: Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Alan,

Thank you for identifying the issue and for submitting your patch. I have 
reviewed and tested your changes. Unfortunately, they do not work in all 
situations. So, I have incorporated the aspects that I could and resolved the 
cookie manipulation vector by including the uid along with the salt before 
generating the hash. This ensures that no one can manipulate the hash or the 
uid.

Unfortunately, this approach will require people to update both External.php 
and drupal4ssp.module.

I have uploaded a new release for download version 1.2.2.

Original comment by smoit...@gmail.com on 10 Dec 2013 at 5:58

• Changed state: Fixed