r68288 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r68287‎ | r68288 | r68289 >
Date:20:57, 19 June 2010
Author:laner
Status:deferred (Comments)
Tags:
Comment:
Fix email me a password functionality when $wgLDAPMailPassword is set to true.
Modified paths:
  • /trunk/extensions/LdapAuthentication/LdapAuthentication.php (modified) (history)

Diff [purge]

Index: trunk/extensions/LdapAuthentication/LdapAuthentication.php
——@@ -387,6 +387,7 @@
388388 global $wgLDAPDomainNames, $wgLDAPUseLocal;
389389 global $wgLDAPAddLDAPUsers;
390390 global $wgLDAPAutoAuthDomain;
 391+ global $wgLDAPMailPassword;
391392
392393 $this->printDebug( "Entering modifyUITemplate", NONSENSITIVE );
393394
——@@ -395,7 +396,12 @@
396397 }
397398
398399 $template->set( 'usedomain', true );
399 - $template->set( 'useemail', false );
 400+
 401+ if ( isset( $wgLDAPMailPassword ) && $wgLDAPMailPassword[$_SESSION['wsDomain']] ) {
 402+ $template->set( 'useemail', true );
 403+ } else {
 404+ $template->set( 'useemail', false );
 405+ }
400406
401407 $tempDomArr = $wgLDAPDomainNames;
402408 if ( $wgLDAPUseLocal ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r68436Fix possible remote_global vulnerabilities, and fix for r68288laner00:41, 23 June 2010

Comments

#Comment by Reedy (talk | contribs)   13:02, 20 June 2010

Isn't the binary or a bit OTT?

Wouldn't something like the below make more sense? 

$template->set( 'useemail', isset( $wgLDAPMailPassword ) && $wgLDAPMailPassword[$_SESSION['wsDomain'] );
#Comment by Nikerabbit (talk | contribs)   13:19, 20 June 2010

I don't see $wgLDAPMailPassword set anywhere with default value, so this is potential register_globals vulnerability.

#Comment by Ryan lane (talk | contribs)   13:29, 22 June 2010

If this is a register_globals vulnerability, then every global I use has this problem. In my next major release I plan on refactoring to fix the way the globals are set.

As of right now, if this is an issue, I think the code should die with a warning if register globals is enabled.

#Comment by Catrope (talk | contribs)   13:36, 22 June 2010

You need to set the $wgLDAP* vars to a default value in the extension setup file. Probably easier than adding a warning.

#Comment by Ryan lane (talk | contribs)   13:38, 22 June 2010

Well, if this is an open vulnerability, I'd prefer to add the warning for now. I'm going to rework the configuration code properly, and it is likely to take some time.

#Comment by Ryan lane (talk | contribs)   00:43, 23 June 2010

On second thought, this is something I am likely going to do when I refactor. Fixed in r68436.

Status & tagging log