Secret unswer bug

Started by snoopy_virtual, August 01, 2015, 02:23:16 PM

Previous topic - Next topic

snoopy_virtual

I have been searching to see if anybody else had ever reported this bug and I cannot find it. If it was already reported please merge this topic with the other one.

The problem is when you are editing a profile, either your own one or anybody else's

If you add a secret question and a secret answer, the first time you do it there is no problem.

The second time you edit the same profile you will only see the secret question, but the secret answer is blank for security reasons (like the password that is also blank).

If this second time you are editing something else (as the email or any other field) and you forget to write the secret answer, when you save the profile the program delete the old answer and leave it blank in the DB.

That's not a big deal if you are editing your own profile, because you are suppose to know your own secret answer, but if you are an admin editing somebody else's profile you are not supposed to know the secret answer.

I think we should add a conditional before saving the profile. Something like:

If there is already a secret question and a secret answer (both not empty) and the secret question has not changed (old one == new one) and the new secret answer is empty, don't change it in the DB.

I am going to check exactly where we should add that conditional (somewhere inside Subs.php or Subs-Members.php I suppose) and try to write a fix for it

As I said at the beginning, if this bug has been reported already (or even better if it has already been sorted) please let me know

==========

Edit: Sorry, I forgot to say I am talking about SMF 2.0.10

El verdadero sabio es aquel que lo ve todo, lo estudia todo, lo analiza todo y molesta poco.
A true wise man is he who sees everything, studies everything, analyses everything and hardly ever annoys.

snoopy_virtual

Found a possible solution.

File: Profile-Modify.php

Find:


'input_validate' => create_function('&$value', '
$value = $value != \'\' ? md5($value) : \'\';
return true;
'),


Replace with:


'input_validate' => create_function('&$value', '

// If we didn\'t try to change it and we already had one then ignore it!
if ($value == \'\' && !empty($cur_profile[\'secret_question\']) && !empty($cur_profile[\'secret_answer\']) && !empty($_POST[\'secret_question\']) && $_POST[\'secret_question\'] == $cur_profile[\'secret_question\'])
return false;

$value = $value != \'\' ? md5($value) : \'\';
return true;
'),

El verdadero sabio es aquel que lo ve todo, lo estudia todo, lo analiza todo y molesta poco.
A true wise man is he who sees everything, studies everything, analyses everything and hardly ever annoys.

snoopy_virtual

Nope. That doesn't work.

The change should be add somewhere else

El verdadero sabio es aquel que lo ve todo, lo estudia todo, lo analiza todo y molesta poco.
A true wise man is he who sees everything, studies everything, analyses everything and hardly ever annoys.

Suki

OK, I can replicate this, thank you for your report, will include a patch for 2.1 and 2.0.x.

The code you proposed possibly is failing because $cur_profile isn't been globalized inside create_function but I'm not 100% sure, will need to test this further.
Disclaimer: unless otherwise stated, all my posts are personal and does not represent any views or opinions held by Simple Machines.

snoopy_virtual

#4
Found the solution:

File: Profile-Modify.php

Find:


'input_validate' => create_function('&$value', '
$value = $value != \'\' ? md5($value) : \'\';
return true;
'),


Add after that:


'is_dummy' => create_function('&$value', '

// If we didn\'t try to change it and we already had one then ignore it!
if ($value == \'\' && !empty($cur_profile[\'secret_question\']) && !empty($cur_profile[\'secret_answer\']) && !empty($_POST[\'secret_question\']) && $_POST[\'secret_question\'] == $cur_profile[\'secret_question\'])
return true;

return false;
'),


$cur_profile is global inside that function, the problem was I was doing the conditional inside the input_validate instead of is_dummy

I have done a hotfix package in case anybody needs it before the next SMF version

Edit, Removed the file as requested.

El verdadero sabio es aquel que lo ve todo, lo estudia todo, lo analiza todo y molesta poco.
A true wise man is he who sees everything, studies everything, analyses everything and hardly ever annoys.

Suki

Been testing your fix, unfortunately, is_dummy wasn't really designed for validation so if its set to true, the field will not be saved even though new data is been provided.

Now, SMF by default doesn't load the value of "secret_answer" into the $cur_profile array so checking the user provided date against it will always result in an error, your code doesn't produce any error because you are checking for !empty($cur_profile[\'secret_answer\']) which will always return false.

So in order to properly fix this, the input_validate field will have to be used and a query to get the correct value from secret_answer will have to be done in order to check it against whatever data the user provided.
Disclaimer: unless otherwise stated, all my posts are personal and does not represent any views or opinions held by Simple Machines.

Suki

OK, further testing. is_dummy is not really a callable so using create_function as its value will not do anything.

If doing a query for secret_answer is a bit too much, heres another solution, rather hackish but gets the job done:

'is_dummy' => empty($_POST['secret_answer']) ? true : false,

Do not that I didn't explicitly use empty() as a return value, some host have problems when checking empty() directly.
Disclaimer: unless otherwise stated, all my posts are personal and does not represent any views or opinions held by Simple Machines.

live627

Quotesome host have problems when checking empty() directly
Really? I did not know that. Is the problem type mismatch?

snoopy_virtual

Quote from: Suki on August 01, 2015, 06:34:59 PM
If doing a query for secret_answer is a bit too much, heres another solution, rather hackish but gets the job done:

'is_dummy' => empty($_POST['secret_answer']) ? true : false,

The problem is not the query. That was my first idea. The problem is "where" to put the query. Needs to be after $_POST['secret_answer'] has been validated and before it's saved to the DB, but cannot find the right spot to put it. Could I put it inside the validating function and do it at the same time we are validating?

Anyway your fix works perfect for me. It doesn't take into consideration if the secret question has been changed or not, but who cares? If the secret answer is blank we can assume the user didn't want to change it anyway, so I am going to stop looking for a place to put the query and I'm going to use your fix.

Thanks.

Quote from: live627 on August 01, 2015, 07:29:54 PM
Quotesome host have problems when checking empty() directly
Really? I did not know that. Is the problem type mismatch?

I didn't know that as well and I'm also curious. In what cases are there problems with empty()?

El verdadero sabio es aquel que lo ve todo, lo estudia todo, lo analiza todo y molesta poco.
A true wise man is he who sees everything, studies everything, analyses everything and hardly ever annoys.

snoopy_virtual

I have been trying to modify my second reply to delete the file I added to it (snp_secret_answer_hotfix.zip) but cannot modify it.

Can anybody delete that file for me please?

Just in case somebody download it and apply it. The code is wrong.  ;)

El verdadero sabio es aquel que lo ve todo, lo estudia todo, lo analiza todo y molesta poco.
A true wise man is he who sees everything, studies everything, analyses everything and hardly ever annoys.

Suki

My bad,  I was thinking about something else, yes it is safe to directly use the returned value from empty()
Disclaimer: unless otherwise stated, all my posts are personal and does not represent any views or opinions held by Simple Machines.

snoopy_virtual

I've been thinking about it a little more and I think that doesn't solve the problem completely.

We are not doing really any kind of validation with secret_answer. We should at least check if there is a secret question or not.

An account with a secret question but with an empty secret answer would be too easy to hack.

I need to get some sleep. It's 4 in the morning here in Spain. I will continue with it tomorrow if nobody else do it before I wake up.  8)

El verdadero sabio es aquel que lo ve todo, lo estudia todo, lo analiza todo y molesta poco.
A true wise man is he who sees everything, studies everything, analyses everything and hardly ever annoys.


snoopy_virtual

I couldn't sleep so I have been doing something on it.

Quote from: live627 on August 01, 2015, 10:40:11 PM
http://www.elkarte.net/community/index.php?topic=2806.0

I cannot answer there because I haven't got an account in elkarte's. I should get one though. I didn't know the site, but I can see some good old friends there.  ;)

I don't think that emanuele's code is the solution. I see they are using anyway their function getBasicMemberData() (that we don't have) and I suppose that function load the value of secret_answer from the DB. Our function loadMemberData(), with $set == 'profile' (the one we use to load $cur_profile) gets the value of secret_question but not the value of secret_answer.

Anyway, even after you have loaded the value of secret_answer from the DB I would treat it as I was doing, but adding also a couple of errors if we have a question without answer or an answer without question.

Something like this:


'input_validate' => create_function('&$value', '

global $cur_profile, $smcFunc, $db_query, $context;

// By default $cur_profile doesn\'t load the value of secret_answer. Let\'s get it
$request = $smcFunc[\'db_query\'](\'\', \'
SELECT secret_answer
FROM {db_prefix}members
WHERE ID_MEMBER = {int:current_member}
LIMIT 1\',
array(
\'current_member\' => $context[\'id_member\'],
)
);
list ($secret_answer_db) = $smcFunc[\'db_fetch_row\']($request);
$smcFunc[\'db_free_result\']($request);

// If we didn\'t try to change it and we already had one then ignore it!
if ($value == \'\' && !empty($cur_profile[\'secret_question\']) && !empty($secret_answer_db) && !empty($_POST[\'secret_question\']) && $_POST[\'secret_question\'] == $cur_profile[\'secret_question\'])
return false;

// If we have a secret question we need to have a secret answer or they will hack you in a minute
if ($value == \'\' && !empty($_POST[\'secret_question\']))
return $txt[\'empty_secret_answer\'];

// Can we have a secret answer without a secret question? It\'s not a security risk but we shouldn\'t just in case
if (!empty($value) && empty($_POST[\'secret_question\']))
return $txt[\'empty_secret_question\'];

$value = $value != \'\' ? md5($value) : \'\';
return true;
'),


Now we should add to Profile.english.php a couple of lines. Something like:
$txt['empty_secret_answer'] = 'You cannot have a question without an answer';
$txt['empty_secret_question'] = 'You cannot have an answer without a question';

I cannot give you the rest of the languages, but in Spanish that would be:
$txt['empty_secret_answer'] = 'No puedes tener una pregunta sin respuesta';
$txt['empty_secret_question'] = 'No puedes tener una respuesta sin pregunta';

As emanuele was saying, working with create_function always give me the creepers. Too many backslashes for me  ;D

So I am not 100% sure. What do you think now?

El verdadero sabio es aquel que lo ve todo, lo estudia todo, lo analiza todo y molesta poco.
A true wise man is he who sees everything, studies everything, analyses everything and hardly ever annoys.

snoopy_virtual

I have make a package and tested it in one of my sites and the code is wrong.

For some strange reason the value of $cur_profile['secret_question'] is not the value of the secret_question in the DB. Somewhere in the process since we loaded it with loadMemberData() until here, it has changed to the new value we have just entered ($_POST['secret_question']). Don't ask me where or why because I always get lost trying to follow the precess inside Profile.php

I have seen this adding a few debugging lines at the top of the function. Like this


if (!empty($cur_profile[\'secret_question\']))
return $cur_profile[\'secret_question\'];
else
return \'no cur_profile secret_question\';


And a few similar ones, to see exactly the values of every variable involved there.

So I have changed the code to this:


'input_validate' => create_function('&$value', '

global $smcFunc, $context, $txt;

// $cur_profile doesn\'t have the values of secret_question and secret_answer in th DB. Let\'s get them
$request = $smcFunc[\'db_query\'](\'\', \'
SELECT secret_question, secret_answer
FROM {db_prefix}members
WHERE ID_MEMBER = {int:current_member}
LIMIT 1\',
array(
\'current_member\' => $context[\'id_member\'],
)
);
while ($row = $smcFunc[\'db_fetch_assoc\']($request))
{
$secret_question_db = $row[\'secret_question\'];
$secret_answer_db = $row[\'secret_answer\'];
}
$smcFunc[\'db_free_result\']($request);

// If we didn\'t try to change it and we already had one then ignore it!
if ($value == \'\' && !empty($secret_question_db) && !empty($secret_answer_db) && !empty($_POST[\'secret_question\']) && $_POST[\'secret_question\'] == $secret_question_db)
return false;

// If we have a secret question we need to have a secret answer or they will hack you in a minute
if ($value == \'\' && !empty($_POST[\'secret_question\']))
return $txt[\'empty_secret_answer\'];

// Can we have a secret answer without a secret question? It\'s not a security risk but we shouldn\'t just in case
if (!empty($value) && empty($_POST[\'secret_question\']))
return $txt[\'empty_secret_question\'];

$value = $value != \'\' ? md5($value) : \'\';
return true;
'),


I have made a package with that and this time it works for me.

If I had already a question and I don't touch it and edit something else (adding an additional group for example) it saves everything properly and doesn't change the secret_answer

If I change the secret question but leave empty the secret answer I get an error:

"There were some errors trying to save:
You cannot have a question without an answer"

With the code that emanuele did I think there is a possibility of having a user with a valid question but without an answer.

If anybody wants to check it here is attached the package I have used

Note for Spanish speaking people: I have modified only spanish_es-utf8 because it's the one I use. If you are using a different one, unzip the package and edit the file install.xml

El verdadero sabio es aquel que lo ve todo, lo estudia todo, lo analiza todo y molesta poco.
A true wise man is he who sees everything, studies everything, analyses everything and hardly ever annoys.

snoopy_virtual

The reason of all this is that I have been asked to help some friends who have a forum where a lot of accounts were hacked recently.

They gave me admin powers and I have been checking logs, DBs, files modified, etc.

I found a few days ago a lot of accounts with secret question but without answers, so I started to investigate the whole thing.

The first thing I saw was how easy is to hack an account with question but without answer, so I deleted all the question without answer.

The second thing was when I found that saving the profile leaving the answer blank was deleting it in the DB and that was when I started this post.

I think it's a very serious security bug and should be updated in the core asap.

In the mean time, after further testing in some other forums I think now my last code was right, so I am applying that fix to all my forums immediately.

That's of course if nobody else comes with a better code for this bug  8)

El verdadero sabio es aquel que lo ve todo, lo estudia todo, lo analiza todo y molesta poco.
A true wise man is he who sees everything, studies everything, analyses everything and hardly ever annoys.

live627

#16
QuoteFor some strange reason the value of $cur_profile['secret_question'] is not the value of the secret_question in the DB. Somewhere in the process since we loaded it with loadMemberData() until here, it has changed to the new value we have just entered ($_POST['secret_question']). Don't ask me where or why because I always get lost trying to follow the precess inside Profile.php
I'm looking at the code now ans have figured out how it happens.

$cur_profile is copied to $old_profile (another global) before any rewriting happens. This is all done in saveProfileFields(). $cur_profile['secret_question'] is changed on line 928, because it comes first in the massive $profile_fields array.

snoopy_virtual

It doesn't matter really.

I had to do a query already to get the value of the secret answer inside the DB, so it's not a big deal to get 2 values in the same visit.  ;D

By the way. I have been thinking a lot more about how they were using this bug to hack accounts and already found a few methods I could reproduce in any SMF forum (without this security fix) to hack accounts.

I am not going to write them here because this is public and I don't want to give ideas to the bad ones (they have too many already)  8)

El verdadero sabio es aquel que lo ve todo, lo estudia todo, lo analiza todo y molesta poco.
A true wise man is he who sees everything, studies everything, analyses everything and hardly ever annoys.

snoopy_virtual

More about it.

After installing the fix in a few forums I realized I should clean the DB in all of them checking if there were any questions without answers.

I did the first one manually, opening phpMyAdmin and doing a simple query:


SELECT * FROM `smf_members`
WHERE (`secret_answer` != ''
AND `secret_question` = '')
OR (`secret_question` != ''
AND `secret_answer` = '')


All my forums have some questions without answer, so after a while I was fed up of deleting them one by one and did an script. fix_empty_answers.php

I have attached it here if anybody want to use it on its own

I have added it also to the fix package and make it run just modifying the file package-info.xml and adding a line like:
<code type="file">fix_empty_answers.php</code>

El verdadero sabio es aquel que lo ve todo, lo estudia todo, lo analiza todo y molesta poco.
A true wise man is he who sees everything, studies everything, analyses everything and hardly ever annoys.

yakyakyak

Thanks for the files snoopy_virtual

Advertisement: