Checking that a password contains a '$', a digit, and an uppercase letter

Question

I have this code, which reads in a string password from the user and checks if it has a dollar sign $, a number, and an upper case letter. If it has all three, it prints:

Your password is correct

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>

#define MAXSIZE 100

void password_read(char password[]);
int password_check(char password[]);

int
main(int argc, char *argv[]){

    char password[MAXSIZE];
    int sum;

    password_read(password);

    sum = password_check(password);

    if (sum) {
        printf("Your password is correct\n");
    } else {
        printf("Your password sucks\n");
    }

    return 0;
}

void
password_read(char password[]) {
    printf("Enter password:\n");
    scanf("%s", password);
}

int
password_check(char password[]) {
    int upper = 0;
    int digit = 0; 
    int dollar = 0;
    int i;

    for (i = 0; i < strlen(password); i++) {

        if (isupper(password[i])) {
            upper = 1;
        } 

        else if (isdigit(password[i])) {
            digit = 1;
        }

        else if (password[i] == '$') {
            dollar = 1;
        }
    }
    return (upper && digit && dollar);
}

I'm pretty shabby with C and I was just wondering if there was a way to reduce part of the code. I also just want to check if the pointer/array handling is correct, since I've only just started using it properly.

Answer 1

Design

• So the point of this program is to check a password, right? There are many problems with your approach to verifying a password. To list a few:

  1. Password could be brute-forced in a matter of milliseconds (on a slow machine) due to small length

  2. Anyone could reverse-engineer your code by de-compiling it and find out how to enter a valid password, even if length was increased to something not as easily brute-forced.

  3. You aren't masking the password when it's entered into the command line in any way; someone looking over your shoulder now knows your password

• Your MAXSIZE is 100 when our password could be a length of 3? Or larger? We should be dynamically allocating the input variable.

Readability & Maintainability

• Understandable variable and function names, good

• No documentation, despite it being a small and easy to understand program you should still include this

• Should have a header for those function prototypes, and perhaps a separate source file for the password handling functions.

• You don't use command line arguments, declare main() as such:

  int main(void)
  

• You should be using puts() instead of printf() since you are not formatting strings (this also lets us exclude the \n)

• You don't have to return 0 at the end of main(). The C standard knows how frequently this is used, and lets us omit it.

  C99 & C11 §5.1.2.2(3)

  ...reaching the } that terminates the main() function returns a value of 0.

Functionality

• While the program is only supposed to verify a password, the functionality isn't great. Why? Well look at the conditions for our password to be "correct" (quite a large range of strings will fall into this range):

  1. an uppercase letter

  2. a digit

  3. a $ character.

• So H8$ is a valid password. Okay... but what if our password isn't "correct"? You output "Your password sucks". But that's actually quite unlikely, my passwords could be MUCH stronger than yours. Reference this relevent XKCD:

Security

• All in all, you shouldn't even be handling passwords by yourself. You should be relying on external libraries to get and encrypt the password.

• To get a password, you could use getpass(), but it is deprecated. See this answer for an alternative implementation.

• To hash a password, I would definitely go with OpenSSL. Please, when it comes to encryption, don't try and roll your own, or just find something someone posted on the Internet. Go with something like OpenSSL that is verified, and trusted by millions every day.

Answer 2

Safety

Your password_read is fairly unsafe. In particular, it uses scanf("%s", without specifying the size of the buffer you're reading into, so if the user enters anything longer than that, you get a buffer overflow (the exact same sort responsible for many attacks).

scanf("%99s", password);

If you prefer to avoid hard-coding the length into the format string (hard-coding like this is almost never a good idea) you can use sprintf to generate a format string at runtime:

char fmt[16];

sprintf("%%%ds", fmt, sizeof(password);
scanf(fmt, password);

Efficiency

Depending somewhat on your compiler, this loop:

for (i = 0; i < strlen(password); i++)

...may easily end up calling strlen once for every iteration of the loop. With the right compiler and standard library, this won't happen, but otherwise it'll change your loop from \$ O(N)\$ to \$O(N^2)\$. Unless you're quite sure you'll only ever use the right compiler, it'd be better to get the length before the loop:

size_t len = strlen(password);
for (i=0; i<len; i++)
    // ....

It's an extra line, but it could make your code run ~100 times faster. In this case, it probably doesn't matter much, but one of those habits you probably want to cultivate.

Messages

"Your password is correct" implies that the user has entered the correct password. I'd use wording more like: "Your password meets complexity requirements".

Requirements

That leads directly to the requirements themselves--requiring a digit and a letter seems reasonable. Requiring the specific character $ seems a lot less reasonable--especially for users outside the US, many (most?) of whom won't have that character on their keyboard at all. Depending on their configuration, they may be able to use their local currency symbol as a substitute--but then again, they may not (and may be left wondering why your program started to fail after they made a seemingly unrelated change).

Answer 3

Know your standard library

There's a standard function for finding one of a set of characters, and that's strcspn().

int password_check(const char *const password)
{
    const size_t len = strlen(password);
    return strcspn(password, "ABCDEFGHIJKLMNOPQRSTUVWXYZ") == len
        || strcspn(password, "0123456789") == len
        || !strchr(password, '$');
}

Alternatively, keep the loop, but terminate it when all three required characters are seen, rather than continuing to the end:

int password_check(const char *const password)
{
    enum {
        upper = 1,
        digit = 2,
        dollar = 4,
    } seen = 0;

    for (const unsigned char *c = (const unsigned char*)password;  *c;  ++c) {
        if (isupper(*c)) {
            seen |= upper;
        } else if (isdigit(*c)) {
            seen |= digit;
        } else if (*c == '$') {
            seen |= dollar;
        }
        if (seen == upper|digit|dollar) {
            return 1;
        }
    }
    return 0;
}

Important - always use unsigned char with the <ctype.h> functions, as plain char may be a signed type.