Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

To avoid hashing of huge passwords limit them to 100 chars for now #16

Merged
merged 1 commit into from May 12, 2020

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented May 11, 2020

Signed-off-by: Roeland Jago Douma roeland@famdouma.nl

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@@ -168,6 +168,12 @@ public function submitPassword(string $token, string $email, string $password, s
], 'guest');
}

if (\mb_strlen($password) > 100) {
return new TemplateResponse('core', 'error', [
'errors' => array(array('error' => $this->l10n->t('Password to long')))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'errors' => array(array('error' => $this->l10n->t('Password to long')))
'errors' => [['error' => $this->l10n->t('Password to long')]]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#PHPCS

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beside the nitpick 👍

@skjnldsv skjnldsv added the enhancement New feature or request label May 11, 2020
@MorrisJobke MorrisJobke merged commit 65ecbb2 into master May 12, 2020
@delete-merged-branch delete-merged-branch bot deleted the enh/no_huge_passwords branch May 12, 2020 08:55
@MorrisJobke
Copy link
Member

@skjnldsv Mind to do a new release and push it to the appstore?

@spaze
Copy link

spaze commented Jul 29, 2020

Hi, can you please point me to where hashing long passwords causes issues? There's no difference in hashing 10 characters long password and 10M characters long password. PHP's bcrypt implementation (like many others ) even truncates passwords at 72 chars, so there's there's no difference beyond 72 chars. Also see this benchmark:

>>> $o = ['cost' => 12]
=> [
     "cost" => 12,
   ]
>>> timeit password_hash(str_repeat('A', 10), PASSWORD_BCRYPT, $o)
=> "$2y$12$VORcqUusFzA0uKwidZyEG.rba8.svSJiBqrKwHiRbke8zHf4LijKm"
Command took 0.291033 seconds to complete.
>>> timeit password_hash(str_repeat('A', 10_000_000), PASSWORD_BCRYPT, $o)
=> "$2y$12$qpzFTZeb72nrCo/U2oswZujt1BUCMUuT4O4JvW3GNPzOR8dy7eM.O"
Command took 0.283745 seconds to complete.
>>>

>>> $o = ['memory_cost' => 65536 * 2, 'time_cost' => 8, 'threads' => 2]
=> [
     "memory_cost" => 131072,
     "time_cost" => 8,
     "threads" => 2,
   ]
>>> timeit password_hash(str_repeat('A', 10), PASSWORD_ARGON2ID, $o)
=> "$argon2id$v=19$m=131072,t=8,p=2$NjBEaUthbG1rNE9GSmRUMQ$DTDldfjp/CH9ZpqIInkdRhvCnqY+jHRTLl79kR0d+b0"
Command took 2.581087 seconds to complete.
>>> timeit password_hash(str_repeat('A', 10_000_000), PASSWORD_ARGON2ID, $o)
=> "$argon2id$v=19$m=131072,t=8,p=2$N2thZC5LbzFaaDFpZjRFaQ$7DRvhkyXNsLSjqcyyktc7thnsX9sx4zoABQu893WF8s"
Command took 2.577008 seconds to complete.

@nickvergessen
Copy link
Member

Well this was an issue reported on our hackerone project and I was able to reproduce it back then:
https://hackerone.com/reports/840598

But now I see the same things you observe.

@spaze
Copy link

spaze commented Jul 30, 2020

The HackerOne report, recently disclosed, is how this caught my attention and how I got here initially :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants