Secret unswer bug

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

Previous topic - Next topic

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: