Password generator

Question

I am playing around with CSS/HTML/PHP and created a password generator:

<?php

$lowercase_characters = "abcdefghijklmnopqrstuvwxyz";
$uppercase_characters = strtoupper($lowercase_characters);
$numbers = "0123456789";
$symbols = ".-+=_,!@$#*%<>[]{}";
$chars = "";
$password ="";

function generatePassword($length, $chars){

    $charsArray = str_split($chars);

    for($i = 0; $i < $length; $i++){
        shuffle($charsArray);
        $password .= $charsArray[0];

    }

    echo '<div class="content-wrapper"><div class="result-content">Your generated password is:<br /><br /><input type="text" value="' .  $password . '" onclick="select()" readonly="true"></div></div>';

}

if(isset($_POST['generate_password'])){

    if(isset($_POST['symbols'])){
        $chars .= $symbols;
    }

    if(isset($_POST['numbers'])){
        $chars .= $numbers;
    }

    if(isset($_POST['lowercase_characters'])){
        $chars .= $lowercase_characters;
    }

    if(isset($_POST['uppercase_characters'])){
        $chars .= $uppercase_characters;
    }

    generatePassword(htmlspecialchars($_POST['p_length']), $chars);

}

?>

<div class="content-wrapper">
    <div class="content">
        <div class="content-header">
            Password Generator
        </div>

        <form method="post">
            <table class="index-table">
                <tr>
                    <td width="30%">Password Lenght:</td>
                    <td width="70%"><input type="number" name="p_length" value="15" required /></td>
                </tr>
                <tr>
                    <td>Include Symbols:</td>
                    <td><input type="checkbox" name="symbols" checked /> (e.g. @#$%)</td>
                </tr>
                <tr>
                    <td>Include Numbers:</td>
                    <td><input type="checkbox" name="numbers" checked /> (e.g. 123456)</td>
                </tr>
                <tr>
                    <td>Include Lowercase Characters:</td>
                    <td><input type="checkbox" name="lowercase_characters" checked /> (e.g. abcdefg)</td>
                </tr>
                <tr>
                    <td>Include Uppercase Characters:</td>
                    <td><input type="checkbox" name="uppercase_characters" checked /> (e.g. ABCDEFG)</td>
                </tr>
                <tr>
                    <td></td>
                    <td><input type="submit" value="Generate Password" name="generate_password" class="generate-button" style="border: none;" /></td>
                </tr>
            </table>
        </form>

    </div>
</div>

It would be great if I could get some feedback on the code that I wrote, how to improve it and if there are any safety issues.

Answer 1

Your code, in general, is neat, practical, and generally pleasant.

Security

From a security perspective, it is common for passwords to require some character types, but in your code, these are just suggestions. It is quite possible for your code to generate the password "password". For all the password generators I have used, when you check the "special characters" check box, it guarantees there will be special character... yours does not.

Single Responsibility

Your function generatePassword($length, $chars) {...} should be a true function to generate a password. Instead, it does two things, it generates the password, and it displays it.

Your code really should be:

echo '<div class="content-wrapper"><div class="result-content">Your generated password is:<br /><br /><input type="text" value="'
     .  generatePassword($length, $chars)
     . '" onclick="select()" readonly="true"></div></div>';

Note how you call the function from inside the presentation code, and the function returns the value?

Algorithm

From an algorithm perspective, I think there's a better way to pull random characters, than what you are doing. You shuffle all the available characters, then pull the first one. It would be better to just pull a random one. Your code is:

$charsArray = str_split($chars);

for($i = 0; $i < $length; $i++){
    shuffle($charsArray);
    $password .= $charsArray[0];

}

I would prefer to see it as:

$charsArray = str_split($chars);
$lastChar = sizeof($charsArray) - 1;

for($i = 0; $i < $length; $i++){
    $password .= $charsArray[rand(0, $lastChar)];
}

The above code saves a shuffle on each character, and the result is equally 'shuffled' from a random perspective.

Answer 2

The answer of rolfl should be the accepted one. It handles nearly everything. I just want to add some remarks:

Bugs

If none of the $_POST variables are set. Something will break...

Security

Your password generator is not secure.

Shuffle internally uses the same 'randomness' as rand(). More info here. This means that your 'random' password is not random when talking security. It will seem random for the human eye, but a computer will quickly burst that bubble.

A much better an easier way of generating a random password is using openssl_random_pseudo_bytes.