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

snoopy_virtual

Quote from: yakyakyak on August 02, 2015, 07:29:59 AM
Thanks for the files snoopy_virtual

you are welcome.

You should check this post in a couple of days though, just in case some of the big coders can improve my solution.  ;)

Cheers

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

Quote from: snoopy_virtual on August 01, 2015, 09:56:26 PM
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)

Yes the code should be checking both question and answer, if it doesn't' already should be added.

Quote from: snoopy_virtual on August 02, 2015, 12:15:42 AM


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?

Thats what I originally meant when I said a new query needs to be done to get the secret answer and do proper checks and thats what I'm going to do.
Disclaimer: unless otherwise stated, all my posts are personal and does not represent any views or opinions held by Simple Machines.

snoopy_virtual

Quote from: Suki on August 02, 2015, 09:59:39 AM
Thats what I originally meant when I said a new query needs to be done to get the secret answer and do proper checks and thats what I'm going to do.

There are a couple of mistakes in that code you quoted though. It was improved in the next answer.

Check it out anyway to see if I have any mistakes left.

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

I'm planning on doing it from scratch anyway but thanks.  Do you mind if I remove your previous hotfix packages?  people very often try to install things right away without realizing what they are installing, plus, they will get issues when trying to install the next release.
Disclaimer: unless otherwise stated, all my posts are personal and does not represent any views or opinions held by Simple Machines.

snoopy_virtual

As far as I know version 3 is right. And version 4 is the same one but with the file "fix_empty_answers.php" inside it to clean the DB automatically at the same time. There are no more files left here.

I have already installed and tested it in 10 forums with no issues so far.

When you update SMF to the next version anybody with this hotfix installed just need to uninstall it before updating SMF.

I don't usually worry too much with the people that doesn't read the full posts and manuals before installing something (I know, there are thousands of them out there LOL) but if you want to delete the files from here it's up to you. I would leave them until you finish the update. After that of course I would delete them.

But as I said it's up to you. I'm happy with whatever you decide to do.  ;)

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.

margarett

Do note that, while it is obviously wrong to delete the secret answer, this is NOT a security issue. You CAN'T use either a blank answer or random text if the answer is blank.
// Check if the secret answer is correct.
if ($row['secret_question'] == '' || $row['secret_answer'] == '' || md5($_POST['secret_answer']) != $row['secret_answer'])
{
log_error(sprintf($txt['reminder_error'], $row['member_name']), 'user');
fatal_lang_error('incorrect_answer', false);
}
Se forem conduzir, não bebam. Se forem beber... CHAMEM-ME!!!! :D

QuoteOver 90% of all computer problems can be traced back to the interface between the keyboard and the chair

snoopy_virtual

Quote from: margarett on August 03, 2015, 07:57:59 AM
Do note that, while it is obviously wrong to delete the secret answer, this is NOT a security issue. You CAN'T use either a blank answer or random text if the answer is blank.

Actually I was thinking about that this morning and I was going to check Reminder.php to see how it was doing the validation. As far as I know it's the only case where you use the secret answer ain't it?

It means you are right. It's only a minor bug. Not a security issue.

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.

margarett

Yes, I think it is (just gave a quick search for "secret_answer" in 2.0.10 and it only shows in Register/Subs-Members, Profile-Modify and Reminder)
Se forem conduzir, não bebam. Se forem beber... CHAMEM-ME!!!! :D

QuoteOver 90% of all computer problems can be traced back to the interface between the keyboard and the chair

Ninja ZX-10RR

Any updates on this by any chance?
Quote from: BeastMode topic=525177.msg3720020#msg3720020
It's so powerful that on this post and even in the two PMs you sent me,you still answered my question very quickly and you're apologizing for the delay. You're the #1 support I've probably ever encountered man, so much respect for that. Thank you, and get better soon.

I'll keep this in my siggy for a while just to remind me that someone appreciated what I did while others didn't.

♥ Jess ♥

STOP EDITING MY PROFILE

snoopy_virtual

As far as I know the only fix just now is the one I did.

I have installed it in all my sites and I will uninstall it when there is an official one. In the mean time ...

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.

Advertisement: