Minor bug in Profile.php

[snoopy_virtual]

As I said a few days ago I'm working now for a community using SMF 2.0.10 where a lot of accounts were hacked a few months ago.

As a security specialist (and member of SMF support team), they asked me to find the problem and sort it.

I am 99% sure the problem was not caused via a hole in SMF, because I have been using SMF for more than 10 years now in a lot of sites, I have hundreds of friends running sites with SMF and I have never seen a similar problem in any of them.

But inside this community I'm working for, there are some people who think that's the reason they were hacked (a hole in SMF core). So (to prove them wrong) I started to check very slowly and thoroughly all the files with anything to do with the login process and the changes in profile areas.

All this years using SMF I had read parts of these files, but never so carefully as I'm doing it now. That's why in the last few weeks I have discovered a few bugs in them.

When I discovered the "secret answer bug" I thought I had found the hole, but (as margarett pointed out) that's not a hole any hacker could use, so I went on searching.

Today I have discovered another minor bug. It's so small I even thought for a while it was not worth it to talk about it, but I suppose I better tell you just in case.

In the file Profile.php find:

Code Select Expand


// If we've changed the password, notify any integration that may be listening in.
if (isset($profile_vars['passwd']))
call_integration_hook('integrate_reset_pass', array($cur_profile['member_name'], $cur_profile['member_name'], $_POST['passwrd2']));


When we call the integration function, we send it 3 parameters:
$cur_profile['member_name']
$cur_profile['member_name']
$_POST['passwrd2']

Why do we want to send the same parameter twice?

Was it just [unknown] (or whoever wrote that function in the first place) having too much coffee and pressing ctrl+v twice by mistake? 

Was their intention to send another different parameter and wrote twice the same one by mistake?

AFAIK sending the member name and the password is more than enough. Is there another parameter we would like to send?

As I said it's a VERY minor bug, but maybe somebody here can see something I have overlooked.

[Kindred]

I think it is supposed to pass real name and display name...

[snoopy_virtual]

Why the display name?

What the integrate function needs is the username they use for login (member_name) and the password.

The only other parameter that would make any sense to me is the email. Maybe the integrate program use email and password instead of nickname and password.

Any other parameter doesn't make any sense.

[Shambles]

More likely it's to cater for "old name" and "new name", when a member undergoes a name change?

[snoopy_virtual]

More likely it's to cater for "old name" and "new name", when a member undergoes a name change?

That's been taken care of in another part of Profile.php. This one here is only when they change the password:

Code Select Expand


// If we've changed the password, notify any integration that may be listening in.


AFAIK you cannot change your nickname and your password at the same time.

[Shambles]

More likely it's to cater for "old name" and "new name", when a member undergoes a name change?

That's been taken care of in another part of Profile.php.

I was thinking of more instances than just Profile.php, such as Subs-Auth.php

Code Select Expand

call_integration_hook('integrate_reset_pass', array($old_user, $user, $newPassword));

[Shambles]

http://www.simplemachines.org/community/index.php?topic=173483.msg1106081#msg1106081

[Suki]

Yes, exactly as Shambles say, that hook expects 3 params, oldname, newname and password, the particular case you are referring to happens only when a new password has been set, thus, the old name is still the same, it gets passed twice because the function needs 3 params and no less. There are other instances where that same hook is called where indeed the old and new name are passed.

[snoopy_virtual]

My mistake.

I should have done a search looking for the same function called somewhere else. Sorry.

You can mark this one as solved then.