1
\$\begingroup\$

I am developing an application that must generate a random password for each new user in the system (initial password).

My idea is to use passwords of 16 characters long.

Would like to know if the code below is a secure way to generate passwords and if it can be improved. The system will be used by hundreds of thousands of users and security is a priority.

import java.security.SecureRandom;
import java.util.Random;

public class SecureRandomPasswordGenerator {

    private static final Random RANDOM = new SecureRandom();
    private static final String POSSIBLE_CHARACTERS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_-+=!@#$%&*()[]{}<>:.,?";

    public String generatePassword(int length) {
        StringBuilder password = new StringBuilder(length);
        for (int i = 0; i < length; i++) {
            password.append(POSSIBLE_CHARACTERS.charAt(RANDOM.nextInt(POSSIBLE_CHARACTERS.length())));
        }
        return password.toString();
    }

}
\$\endgroup\$
  • 2
    \$\begingroup\$ A function generatePassword(int length, int minDigits, int minSpecial) would be more secure. Maybe, your random only generates letters.... \$\endgroup\$ – Holger Sep 20 '18 at 9:39
  • \$\begingroup\$ Read my updated answer below! \$\endgroup\$ – Mr Pro Pop Sep 25 '18 at 21:26
  • 1
    \$\begingroup\$ @Holger, to see the flaw in that argument consider the logically identical argument that when generating a 4-digit PIN you should ensure that two digits are even and two are odd. By doing so, you reduce the key space from 10000 to 3750. How is that "more secure"? By Kirchoff's principle you should assume that an attacker knows how you generate keys, so that the only secret is the key itself; but then you want to maximise entropy in the key generation. \$\endgroup\$ – Peter Taylor Sep 26 '18 at 11:52
  • \$\begingroup\$ @Holger That is not more secure; quite the opposite. Your suggestion reduces the amount of actual randomness because your approach is not random; it only relies on the assumption of what "looks" more "random" to the human eye, but in reality that's not the case. It ends up reducing the amount of entropy. \$\endgroup\$ – code_dredd Oct 9 '18 at 23:32
4
\$\begingroup\$

Top hit on Google looks very similar; in any case no need to reinvent this.

But really, if this is the initial password, the users are going to get it how? Are the being forced to reset it immediately? All that is more a question for https://security.stackexchange.com/ than here.

But sure, looks okay, AFAIK SecureRandom is thread-safe (is that important?) and it's going to produce "random enough" output, the loop is okay too.

Though why specifically 16 characters and not e.g. 32? Do they have to enter them from a sheet of paper? Then there would be a couple of issues with it, like possibly mistaking i, I, l, L and 1, or 0, o and O, ... and so on.

\$\endgroup\$
  • 1
    \$\begingroup\$ I checked the source code of the commons-lang class RandomStringUtils and discovered that it uses java.util.Random instead of java.security.SecureRandom as the source of randomness. So I believe it is less secure. Source: commons.apache.org/proper/commons-lang/apidocs/src-html/org/… \$\endgroup\$ – Daniel Sep 20 '18 at 14:15
  • \$\begingroup\$ Actually, the user will not know his own password. It will be used by the system to perform SSO (single sign-on) on another system. When user login to system1 for the first time, the system1 will call system2 API to create user in system2. And then, use an auto-submit html form using the generated credentials to perform SSO to system2. \$\endgroup\$ – Daniel Sep 20 '18 at 14:22
  • \$\begingroup\$ I will take your suggestion and increase password length from 16 to 32. Although I believe 16 characters is already pretty safe, it will not hurt using 32. \$\endgroup\$ – Daniel Sep 20 '18 at 14:25
  • 1
    \$\begingroup\$ Pretty sure on the list of attack vectors this password isn't going to be the top-most one. Again the other StackExchange site might help you review the architecture a bit better if that's of any concern. \$\endgroup\$ – ferada Sep 20 '18 at 15:49
1
\$\begingroup\$

As what @Holger said would be more secure. I have gave it a try, and came up with following using a really simple Fluent interface

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Random;

public class PasswordGenerator {

    /**
     * @Author Mr Pro Pop
     * 
     *         This creates a simple secure password
     * 
     * @todo Encryption
     */

    private final ArrayList<Character> UPPERCASE_LETTERS = new ArrayList<>(Arrays.asList('A', 'B', 'C', 'D', 'E', 'F',
            'G', 'H', 'I', 'G', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'));
    private final ArrayList<Character> LOWERCASE_LETTERS = new ArrayList<>(Arrays.asList('a', 'b', 'c', 'd', 'e', 'f',
            'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'));
    private final ArrayList<Character> DIGITS = new ArrayList<>(
            Arrays.asList('0', '1', '2', '3', '4', '5', '6', '7', '8', '9'));
    private final ArrayList<Character> SPECIAL_CHARACTERS = new ArrayList<>(
            Arrays.asList('!', '@', '#', '$', '%', '&', '*', '(', ')', '+'));

    private final int SIZE = 16;
    private final Random random = new Random();

    // Stores all the characters being created to build a password
    private List<Character> characters = new ArrayList<Character>();

    /**
     * The function gets a random character from the array, and adds it to the list
     * 
     * @param count The count of lower case characters to be created
     * @return The current characters in the list
     */
    public PasswordGenerator lowercase(int count) {
        for (int i = 0; i < count; i++) {
            characters.add(LOWERCASE_LETTERS.get(random.nextInt(LOWERCASE_LETTERS.size())));
        }
        return this;
    }

    /**
     * The function gets a random character from the array, and adds it to the list
     * 
     * @param count The count of upper case characters to be created
     * @return The current characters in the list
     */
    public PasswordGenerator uppercase(int count) {
        for (int i = 0; i < count; i++) {
            characters.add(UPPERCASE_LETTERS.get(random.nextInt(UPPERCASE_LETTERS.size())));
        }
        return this;
    }

    /**
     * The function gets a random digit from the array, and adds it to the list
     * 
     * @param count The count of digits characters to be created
     * @return The current characters in the list
     */
    public PasswordGenerator digits(int count) {
        for (int i = 0; i < count; i++) {
            characters.add(DIGITS.get(random.nextInt(DIGITS.size())));
        }
        return this;
    }

    /**
     * The function gets a random special from the array, and adds it to the list
     * 
     * @param count The count of specials characters to be created
     * @return The current characters in the list
     */

    public PasswordGenerator specials(int count) {
        for (int i = 0; i < count; i++) {
            characters.add(SPECIAL_CHARACTERS.get(random.nextInt(SPECIAL_CHARACTERS.size())));
        }
        return this;
    }

    /**
     * Shuffles the characters in the lsit among themselves for randomability that
     * digits and other characters of similar type don't end up being next to each
     * other
     * 
     * @return The current list of password
     */

    public PasswordGenerator shuffle() {
        Collections.shuffle(characters);
        return this;
    }

    /**
     * This fucntion assigns values from the list to a character array and then
     * deletes the list
     * 
     * @return The password that was created
     */

    public char[] getPassword() {
        char[] password = new char[SIZE];
        for (int i = 0; i < characters.size(); i++) {
            password[i] = characters.get(i);
        }
        characters.clear();
        return password;
    }
}

An example of usage:

   public static void main(String[] args) {
        PasswordGenerator passwordGenerator = new PasswordGenerator();
        for (int i = 0; i < 10; i++) {
            System.out
                    .println(passwordGenerator.uppercase(4).lowercase(6).digits(3).specials(3).shuffle().getPassword());
        }
    }

Output:

!F9F8Hq4yK#)xydw

5u4XaO+++aGkE2go

6oz#ueAr&1z3C%UI

yo!z(8RVyG1(taX9

7!$A)2kxCuZNpi9g

l%!u2vI1FXjm#s9K

!C1beaa2Y!Xt+3Vb

8aQXPb)09!p$Nrfn

P2s(xpNy12UKy%l@

B5rOcw$usU7p+3$B

This was just a quick example that I wrote in 10 minutes, so it is not in it's perfect form, however it works.

Improvements that you could do are:

  • Use Encryption
  • Maybe make a function that take parameters of minimum letters, digits, and so on
  • wipe data after intention is used (Read below)

Further notes char[] is preferred over String for passwords and here is why

Strings are immutable. Once written, they cannot be overwritten. Whenever a string is created, it will be available in memory until Garbage collector clears. So anyone who has access to the memory can read the value of the string. Since String is used in String pool for reusability, there is pretty high chance that it will remain in memory for the long duration, which poses a security threat. Anyone has access to memory dump can find the password in clear text.

Once you've created the String, if another process can dump memory, there's no way (aside from reflection) you can get rid of the data before garbage collection kicks in. Now garbage collection in Java doesn't happen at any guaranteed interval. The String can thus persist in memory for a long time, and if a process crashes during this time, the contents of the string may end up in a memory dump or some log.

char[] is mutable and by using them, they reduce the window of opportunity for an attacker, and it's only for this specific type of attack. You can explicitly wipe the data by setting each character to zero once the purpose of the password is served and the password won't be present anywhere in the system, even before garbage collection which reduces the window of opportunity for an attacker, and it's only for this specific type of attack.

If the value of the string is modified then it will end up creating a new string. (Example: string = null;) So both the original value and the modified value stay in the memory until it is garbage collected. With plain String you have much higher chances of accidentally printing the password to logs, monitors or some other insecure place whereas char[] is less vulnerable.

Consider this:

public static void main(String[] args) {
        Object pass = "Password";
        System.out.println("String: " + pw);

        pass = "Password".toCharArray();
        System.out.println("Array: " + pw);
}

Prints:

String: Password

Array: [C@5829428e

Example of using reflection

private static void clearString(String password) {
    try {
        Field value = String.class.getDeclaredField("value");
        value.setAccessible(true);
        char[] chars = (char[]) value.get(password);
        Arrays.fill(chars, '*');
    } catch (Exception e) {
        throw new AssertionError(e);
    }
}

I also suggest working with hashed or encrypted passwords instead of plain text.

Facts:

Java itself recommends using getPassword() method of JPasswordField which returns a char[] and deprecated getText() method which returns password in clear text stating security reason. It's good to follow advice from Java team.

I also remember that it was in one of google interview questions.

Note: These information has been taken from different resources and summarized

\$\endgroup\$
  • \$\begingroup\$ I doubt that the shuffle method is really necessary, if it's true randomness it shouldn't matter if it has been shuffled or not. \$\endgroup\$ – Simon Forsberg Sep 26 '18 at 9:04
  • \$\begingroup\$ @SimonForsberg I don't mean security by randomness of positions, just don't want it to always be in the exact same form and guessable which letters come first, followed by digits, and so on. Wanted like kind of a mixed form. It is not that necessary, I agree with you, however, from discrete mathematics, total combination of 16 characters of 4 different types 96^16 is different from when you know which group comes first, then the other 3. That would make it a little bit easier to crack by the pattern cause of less combinations when it comes to brute forcing. \$\endgroup\$ – Mr Pro Pop Sep 26 '18 at 11:22
  • \$\begingroup\$ Here is an example: license plate have 4 letters followed by 3 digits. There are 52 letters (lower case and upper case) and 10 digits, then the possibilities are 52^4 * 10^3 = 7311616000 possibilities. let's now mix the form, it would then be 66^7 = 3521614606208. Large difference as you see, and I hope you got me. \$\endgroup\$ – Mr Pro Pop Sep 26 '18 at 11:25
  • \$\begingroup\$ Apart from the fact that the sample code would give compile-time errors because you can't access private members from a different compilation unit, Holger's comment is just plain wrong. The entropy of a password is decreased by forcing its ratios of certain groups of characters, making it less secure. \$\endgroup\$ – Peter Taylor Sep 26 '18 at 11:44
  • \$\begingroup\$ @PeterTaylor I changed them to public. Hmm about entropy yes, I agree with you. So the first example of the user would be fine? \$\endgroup\$ – Mr Pro Pop Sep 26 '18 at 11:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.