JustSteveKing / password-generator

Generate random, memorable passwords easily.
MIT License
22 stars 1 forks source link

nice idea, but not very helpful #1

Open damac23 opened 1 year ago

damac23 commented 1 year ago

There are some implementation issues like the very, very limited list of words - yes one can override this, but who's gonna do that? The other one is using array_rand: https://www.php.net/manual/en/function.array-rand.php As mentioned there: "This function does not generate cryptographically secure values, and must not be used for cryptographic purposes, or purposes that require returned values to be unguessable."

But from apart that this is a very bad idea. You should at no circumstance generate a password serverside if it is meant to be used by someone else. This is not good practice. That's why there is a password generator in current browsers for clientside usage.

So the least you should do is warn package users that this is for demonstration purposes and/or for some limited internal use.

I would not have commented here, if this package wasn't promoted on laravel-news.

snapey commented 1 year ago

I have to agree. It's a pity that Laravel News does not allow comments on what will be seen as authoritative posts.

I built a similar passphrase generator at https://talltips.novate.co.uk/laravel/passwordless-login to create a one time passphrase to be mailed to users for them to use instead of a password. It uses a dictionary of words from the EFF, curated for this purpose.

https://www.eff.org/deeplinks/2016/07/new-wordlists-random-passphrases

As for changing certain characters for similar shaped numbers, and calling it a more secure version is just laughable.

sneycampos commented 1 year ago

He said "secure"

alfiosalanitri commented 1 year ago

Password without special characters?

In my personal password manager I use this package https://github.com/ircop/passworder that generate strong password with special characters.

snapey commented 1 year ago

Length trumps complexity every day of the week. All that matters is that special characters might be used

Besides, @damac23 is right, your PHP has no need to ever know the user's password.

alfiosalanitri commented 1 year ago

Besides, @damac23 is right, your PHP has no need to ever know the user's password.

Why?

damac23 commented 1 year ago

There are some points that can be discussed. Like using server side genrated passwords for one time usage - but the consrained there is time. That may sound like a small factor, but if a password is only usable for a certain timeframe, some shortcomings might be not as bad. But, as I said, it's a total no go if you use functions that are specifically not safe in that kind of context and implemations that are missleading (like "secure" if you always replace those "special characters"... heck, this is no problem for any brute force attempt).

One can argue that a password generated service side might be ok, because if you authenticate against that specific website, your browser sends the password (within SSL) in cleartext, so the webapplicaton can hash it and compare it with the stored hash. I might be able to follow that for some extend, but it is still not necessary. And in any security context you should avoid adding unnecessary complexity - or in other words: unecessary (communication)partners.

Btw. the mentioned ircop/passworder is also flawed: https://github.com/ircop/passworder/blob/master/src/Passworder.php It uses mt_rand.... it would be so nice if people would read the f* manuals...

alfiosalanitri commented 1 year ago

@damac23 I have changed ircop/passworder with this https://github.com/ircmaxell/RandomLib and this is my function:

public static function generatePassword(int $length = 15)
    {
        $factory = new Factory();
        $generator = $factory->getGenerator(new Strength(Strength::MEDIUM));
        return $generator->generateString($length, '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ-_!?%#');
    }

what do you think?

valorin commented 1 year ago

There are some implementation issues like the very, very limited list of words - yes one can override this, but who's gonna do that?

This is definitely the biggest issue I can see with this. It would be better to pull a huge list of words by default, or not include any, and require a significantly sized set to be seeded before passwords are generated. Using a small list like this makes the array_rand() usage irrelevant - easier to brute-force from the limited subset than waste time predicting the guessable randomness.

The other one is using array_rand: https://www.php.net/manual/en/function.array-rand.php As mentioned there: "This function does not generate cryptographically secure values, and must not be used for cryptographic purposes, or purposes that require returned values to be unguessable."

I'd suggest replacing array_rand() with a loop that uses random_int() to properly pick random words from the list. This is a very basic idea of what would be better:

for ($i = 0; $i <= $length; $i++) {
    $password[] = $words[random_int(0, $max)];
}

See https://codereview.stackexchange.com/questions/275832/cryptographically-secure-version-of-the-core-array-rand-function for an interesting discussion about this sort of thing.

Timing attacks aren't much of an issue in this situation, given you're generating a fresh password - not trying to compare an existing one. If you can accurately measure the password generation, then you can probably snoop on it too!

But from apart that this is a very bad idea. You should at no circumstance generate a password serverside if it is meant to be used by someone else. This is not good practice. That's why there is a password generator in current browsers for clientside usage.

There are some situations where it's unavoidable - such as sending out passwords via email or where there isn't Javascript that can securely generate the password. And before that get's me yelled at, think about embedded devices/TVs or limited access systems.

So the least you should do is warn package users that this is for demonstration purposes and/or for some limited internal use.

Agreed on this point.

Your PHP has no need to ever know the user's password.

Rubbish. Your PHP needs the plain text password to hash it when updating or validating the password.

The problem with generating a password server side is if the response body is somehow exposed. This has happened on WordPress before with the user registration page being cached by a third-party caching plugin and the suggested generated password was included in the cached page. 🤣 (I can't find a link, sorry)

JustSteveKing commented 1 year ago

Slow your horses gents, this was a tutorial repo showing how you can generate passwords or one time passwords - not a defacto "you should use this". Also perhaps my usage of air quotes on secure was not clear enough?

I will update the README and tutorial mentioning this should not be used for passwords in a production environment.