« Right to madness
Password flood bug from rev 1879
Cannot edit own posts. »

Nao

  • Dadman with a boy
  • Posts: 16,082
Password flood bug from rev 1879
« on July 18th, 2013, 12:13 AM »
As mentioned in the thoughts (private ones[1]), someone has been having trouble logging into the forum.
I looked into it, reset their password, and it still didn't work... They got a brute force warning.

Now, here's the thing. I remembered that Pete had committed some SMF fixes for Wedge, regarding brute force issues, and I'm thinking, that's where the problem lies -- in this fix.
So, is it an SMF bug, or a Wedge bug..?

The SMF 2.1 rewrite:

Code: [Select]
$number_tries = $time_stamp < time() - 20 ? 2 : $number_tries;

The Wedge rewrite:

Code: [Select]
$number_tries = time() - $time_stamp < 20 ? 2 : $number_tries;

From what I understand: first one means, "if recorded flood timestamp is older than 20 seconds ago, reset it to 2, otherwise don't touch it.
Second one means the exact opposite... Meaning, once the flood record is older than 20 seconds, the user is forever locked out of the forum.
It took me an hour to finally find the culprit, and I'd like to know what exactly went into this rewrite..?

Also, the Elk version is cleaner, and much shorter...

Code: [Select]
// Timestamp invalid or non-existent?
if (empty($number_tries) || $time_stamp < (time() - 10))
{
// If it wasn't *that* long ago, don't give them another five goes.
$number_tries = !empty($number_tries) && $time_stamp < (time() - 20) ? 2 : 0;
$time_stamp = time();
}

Finally, a note about the code that tests the timestamp and resets it if it's too recent.. (?)

In SMF 2.1:

Code: [Select]
if ($time_stamp < time() - 10)

(Means "if time stamp is older than 10 seconds...")

in Wedge:

Code: [Select]
if (time() - $time_stamp < 10)

(Means "if time stamp is more recent than 10 seconds ago...")

I'm inclined to believe the Wedge version is the correct one here, but honestly, I have no fucking idea. I'm just not comfortable enough with the whole validatePasswordFlood function, to be clear. I understand what it does, but not the magic of it.
So, err... Need your opinion, guys..?

Hopefully Pete sees this topic through the magic of 3G, or something... :^^;:
Posted: July 18th, 2013, 12:08 AM

Additional note...

Looks like I can login with my hardcoded flood value, but it doesn't get reset to nothing, so I'm still with the same flood value after that... (?!)
 1. I thought that using a public topic for this was okay, because I'm not going into specifics with the source code, and it's close enough to the SMF version anyway, which is freely available on github, so...

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: Password flood bug from rev 1879
« Reply #2, on July 18th, 2013, 06:22 AM »
OK, quick history. It was a bug in SMF 2.0.x, patched in the 2.0.4 patch as per http://custom.simplemachines.org/upgrades/index.php?action=upgrade;file=smf_patch_2.0.4.tar.gz;smf_version=2.0.3 (see the edit to LogInOut)

Now, if I remember rightly, the issue stemmed from the situation where the system tried to prevent ongoing brute forcing of an account; in theory you're supposed to have a few attempts before you're pushed to the account reminder page, but if those attempts to log in aren't within a given period of time, you're free to do it again. This is fine because it's a balance between account access and security and not preventing legitimate users from being adversely affected.

I think I was trying to make it more readable than the SMF version at the time. Oh, and no, the Elk version is not magically 'more readable', packing more crap into fewer lines does not make it more readable. I deliberately tried to make it *easier* to read rather than having to make sense of all the combinations of brackets, however it looks like I screwed up the operator ordering.
When we unite against a common enemy that attacks our ethos, it nurtures group solidarity. Trolls are sensational, yes, but we keep everyone honest. | Game Memorial

Nao

  • Dadman with a boy
  • Posts: 16,082

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: Password flood bug from rev 1879
« Reply #5, on July 18th, 2013, 09:31 AM »
Yeah, on second thought, your version had the same issue, i.e. once the time stamp is older than 10 seconds, it never gets updated again, and thus doesn't serve a purpose anymore... ;)
It was past midnight, etc.

As for screwing up the logic, I'm not going to blame anyone for it, these things happen. It's just unfortunate that in this case, people who were locked out of wedge.org didn't consider e-mailing me, or creating a new account and telling us... :^^;:
« Right to madness
Cannot edit own posts. »