6
\$\begingroup\$

In response to a question on Information Security on brute-forcing passwords, I wrote code that helped solve the problem:

Generate a password list of 10 character passwords containing only a combination of 3 - 6 numbers and 3 - 6 uppercase letters.

I'd like a code review done of the snippet I wrote. I don't know really anything about optimizing software. I can write it (self-taught) but I don't have the deep insights needed to improve already working software, so I've started posting code snippets in here for you guys to give me insights. I really think this channel is great and without further ado, I'll get to it.

#include <iostream>
#include <vector>
#include <random>
#include <string>

const char charset[] = {'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','0','1','2','3','4','5','6','7','8','8','9'};
int main()
{

    std::cout << "Please enter the number of passwords to generate here: ";
    int num_pass;
    std::cin >> num_pass;
    std::random_device dev;
    std::mt19937_64 rng(dev());
    std::vector<std::string> passwds;
    std::uniform_int_distribution<std::mt19937_64::result_type> dist(0, sizeof(charset) - 1);

    for (int i = 0; i < num_pass; ++i) {
        std::string pass = "";
        int num_nums = 0, num_chars = 0;
        while (pass.length() < 10) {
            char c = charset[dist(rng)];
            if (isdigit(c) && num_nums < 6) {
                pass += c;
                num_nums++;
            }
            else if (isalpha(c) && num_chars < 6) {
                pass += c;
                num_chars++;
            }
        }
        passwds.push_back(pass);
        std::cout << pass << std::endl;
    }
    std::cin.get();
}
```
\$\endgroup\$
  • \$\begingroup\$ If you have a 10 character password, with 3 digits, and at most 6 uppercase letters, what is the 10th character? Or with 3 uppercase letters and at most 6 digits, what is the 10th character? \$\endgroup\$ – AJNeufeld May 9 at 0:37
  • \$\begingroup\$ I saw that discrepancy. I believe the OP meant 4 to 6 not 3 to 6 letters/digits. So it just uses 4-6 random digits. Which means it either has 4 letters and 6 digits or 6 digits and 4 letters. \$\endgroup\$ – leaustinwile May 9 at 0:39
  • 2
    \$\begingroup\$ You shouldn't edit the code in the question after you have received an answer. See this help topic. \$\endgroup\$ – 1201ProgramAlarm May 9 at 4:27
7
\$\begingroup\$

In general, placing constraints on your passwords, such as that they have x-many numerals and y-many letters, makes your password generation scheme slightly worse; there's no advantage.
But sometimes we're required to do silly things.

In this situation, it looks like the requirement is to have at least four of each, but that could change, as could the desired length of the password. So if this is going to be a reusable tool, it sounds like it should take at least 4 parameters, three of them optional:

  • password_length
  • max_alpha - default password_length
  • max_num - default password_length
  • quantity - default 1

This will give you useful and appropriate behavior by default. The other convenient thing is that you can exclude a character class altogether by setting the max_x to 0.
Do remember to check that the input values are reasonable (non-negative or whatever).

I think the problem 1201ProgramAlarm points out is real, but his proposed solution is a little vague (and sounds hard to maintain), and the edit you made to solve the problem doesn't look like it addresses it at all.

Here's what I would do, both for the above reason and for maintainability:

  • Define each character class as a separate vector. (Should upper-case and lower-case be their own classes? this could get complicated fast. Let's assume for now that there are just the 2 classes)
  • Calculate the minimum number of characters we need from each class.
  • Define a vector containing the union of the character classes. We'll use this to select only the characters who's class we don't care about.
  • Calculate how many don't-care characters there will be, such that
    min_alpha + min_num + dont_care == password_length.
    Counting the don't-care's, we have 3 "classes", and an exact number of characters we want from each class.
  • For each of the target character quantities, select that many characters (uniformly at random with replacement) from the respective class (vector). You could probably be pushing all of these to a single string/buffer as you go.
  • Shuffle the string.

Also, because this is Code Review:

  • You're building a list passwds, but you're not using it.
  • Taking arguments from stdin isn't as user-friendly as taking them as arguments to main(), in my opinion.
  • The whole business of removing duplicates from the list seems fishy to me, but I guess it's better to do it in a separate step than to bake it into your password generator.
  • Is the business with the clock purely diagnostic?
  • Similarly, the part at the end where you hold for the user to hit enter seems un-friendly to me, but I guess I don't know your use-case.
  • If you do implement my advice above, then you're almost certainly going to want to break this up into a couple different functions.
\$\endgroup\$
  • \$\begingroup\$ So I hear all your points, but you have to remember, this wasn't programmed for maximum efiiciency. It was just written as a PoC for another question in a different channel. I just thought I'd post it in here to learn. The business with the clock is purely diagnostic. That was simply in there for my purposes, to see when it ended during debug, if the strings generated were in fact non-uniform. I agree about the stdin vs. argv point too, again, it was just created in VS as a PoC for someone else's problem. Good points, but shuffling the array and then again picking a random index \$\endgroup\$ – leaustinwile May 9 at 4:56
  • \$\begingroup\$ Hi! To clarify: None of my points will make the program more efficient. A lot of it is about making the code easier to use, which might not matter. My suggestion about using multiple character-class vectors is would solve @1201ProgramAlarm's point, and would make the code more obviously correct. Shuffling the source array adds nothing; it does not solve this problem. \$\endgroup\$ – ShapeOfMatter May 9 at 11:35
  • 1
    \$\begingroup\$ But wouldn't using two vectors and then selecting from a randomized union of them have the same effect as just combining them into one vector? I'm not talking about code-correctness, I'm just referring to the bias that ProgramAlarm identified. But I also just realized, that there's no way to remove that bias without violating the constraints that the person who needed the code had. \$\endgroup\$ – leaustinwile May 9 at 17:37
  • 1
    \$\begingroup\$ Ah; I understand your confusion. I've edited the answer to clarify what I mean. \$\endgroup\$ – ShapeOfMatter May 9 at 17:58
7
\$\begingroup\$

Your passwords will be biased, with more letters appearing towards the front of the password and more digits towards the end.

A better approach would be to determine how many digits you will have in the password. Then, for each character, determine if it should be a digit based on the number of digits you want to have and the number of characters left to fill by checking if (random(characters_left) < digits_left). Then select either a random digit or letter for that position.

When you declare pass, you don't need to pass it an empty string. It is default constructed as empty.

std::string pass;
\$\endgroup\$
  • \$\begingroup\$ Can you tell me why it would be biased? It's easily solved with one line. \$\endgroup\$ – leaustinwile May 9 at 1:09
  • 1
    \$\begingroup\$ @leaustinwile 26 times out of 36, the first character will be a letter (since you have 26 letters and 10 digits). Since this continues for the remaining characters, you'll run thru letters faster than digits, resulting in more digits at the end. \$\endgroup\$ – 1201ProgramAlarm May 9 at 1:10
  • \$\begingroup\$ Review the code again, I've solved this problem and fixed the "" declaration of the string. (Old java habits). \$\endgroup\$ – leaustinwile May 9 at 1:31
  • \$\begingroup\$ sorry for fuzzing your points. having read the previous question, i understand that some of what's going on is a design requirement for whatever reason. \$\endgroup\$ – ShapeOfMatter May 9 at 1:49
  • \$\begingroup\$ I ran the password generator for 100_000 random passwords, and there is how often a letter came up in the positions: [72102, 72331, 72292, 72208, 72302, 71974, 62152, 44757, 28754, 16336]. This bias is much more extreme than I expected. \$\endgroup\$ – Roland Illig May 9 at 18:34

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.