Etydhv

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();

```

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;

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 0

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.)


• Define a vector containing the union of the character classes.


• Calculate how many don't-care characters there will be, such that max_alpha + max_num (+ etc) + dont_care == password_length.


• 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.