Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm still learning C, but I'm trying to make sure I've got a decent grasp on working with "strings" and data structures.

If possible, I'd like a little input on how I'm handling this and see if

  1. it could be done more efficiently
  2. I'm setting myself up for unforeseen negative consequences
  3. I'm using the functions appropriately

void test(char *username, char *password) {

    printf("Checking password for %s - pw: %s\n",username,password);
    char *query1 = "SELECT password FROM logins WHERE email = '";
    char *query2 = "' LIMIT 1";

    char *querystring = malloc(strlen(query1) + strlen(username) + strlen(query2) * sizeof(char));

    strncpy(querystring,query1,strlen(query1));
    strncat(querystring,username,strlen(username));
    strncat(querystring,query2,strlen(query2));

    printf("Query string: %s\n",querystring);

    mysql_query(mysql_con,querystring);
    MYSQL_RES *result = mysql_store_result(mysql_con);


    int num_fields = mysql_num_fields(result);
    int num_rows = mysql_num_rows(result);

    if (num_rows != 0) {

        MYSQL_ROW row;
        printf("Query returned %i results with %i fields\n",num_rows,num_fields);

        row = mysql_fetch_row(result);

        printf("Password returned: %s\n",row[0]);

        int comparison = strncmp(password, row[0], strlen(password));

            if (comparison == 0) {
                printf("Passwords match!\n");
            } else {
                printf("Passwords do NOT match!\n");
            }

            }
    else {
        printf("No such user... Password is invalid");
    }
        free(querystring);
}

Output of program:

Checking password for test@blah.com - pw: 5f4dcc3b5aa765d61d8327deb882cf99
Query string: SELECT password FROM logins WHERE email = 'test@blah.com' LIMIT 1
Query returned 1 results with 1 fields
Password returned: 5f4dcc3b5aa765d61d8327deb882cf99
Passwords match!
share|improve this question
    
As a side note, if you do not need to read the password, you could also add it to your WHERE ... clause, then the compare is done by the SQL. Although some people like to know whether the account exists (email found in table) versus invalid password... depends whether you'd like to distinguish between both. –  Alexis Wilke Aug 30 at 0:02

6 Answers 6

Fairly good C. Few notes:

  • sizeof(char) is by 1 definition. It doesn't hurt to multiply by it, but still totally unnecessary.

  • malloc return value better be tested against NULL.

  • Calling functions of strn (emphasis on n) family in this context is kinda paranoid: you've already allocated enough memory; no need for extra safety. In fact, I'd rather go for sprintf("%s%s%s", querystring, query1, username, query2).

  • int comparison is just noise. It's OK to directly test results of strcmp(). BTW, strncmp in this context looks paranoid again.

  • I am not familiar with mysql. Shouldn't you release row somehow?

share|improve this answer

You have an SQL injection vulnerability, due to the way you composed the SQL query by incorporating the value of user without escaping it first.

size_t username_len = strlen(username);
char *escaped_username = malloc(2 * username_len + 1);
unsigned long escaped_username_len = mysql_real_escape_string(
    mysql_con, escaped_username, username, username_len
);

const char *query_head = "SELECT password FROM logins WHERE email = '";
const char *query_tail = "' LIMIT 1";
char *query = malloc(strlen(query_head) + strlen(query_tail) + escaped_username_len + 1);
sprintf(query, "%s%s%s", query_head, escaped_username, query_tail);
free(escaped_username);

/* Do query here... */

free(query);

In addition, storing plaintext passwords is poor security practice. You should be storing salted password hashes. (If you are already using hashes, then the function parameter should be renamed to pw_hash or something.)

share|improve this answer
1  
I made the same "mistake," but in his output the password looks like a hash... –  Alexis Wilke Aug 29 at 23:17
    
Thank you for the input. I completely overlooked that concept... Also, I changed up a bit and ended up discovering snprintf() which I used to also determine the amount of memory to malloc... Out of curiosity, I noticed that you malloc 2 * the length + 1. I know the +1 is to account for the nul at the end, but is the multiplier of 2 actually required consider the char *escaped_username indicates you're malloc'ing a char? Or would I actually be better off doing a malloc(sizeof(char) * username_len +1); ??? –  Jonny Whatshisface Aug 29 at 23:31
    
The 2 * strlen(input) + 1 requirement is from the mysql_real_escape_string() documentation. –  200_success Aug 29 at 23:53

You are using strncat() the wrong way. It is a very treacherous function. I have no clue why it was done that way, but you probably won't even believe what I tell you here:

The size is NOT the total size of the string, but the space left!

From the manual page:

If src contains n or more bytes, strncat() writes n+1 bytes to dest (n from src plus the terminating null byte). Therefore, the size of dest must be at least strlen(dest)+n+1.

If you are to deal with many strings and do copies, concatenate, etc. I would suggest you create a small structure representing a string and functions that handle your strings. That allows you to put all the copy code in one place and avoid potential problems littering your entire program.


In the following test, you use strncmp():

int comparison = strncmp(password, row[0], strlen(password));

If the length of password is shorter than the length of row[0], then your comparison is wrong. Assuming your password are a SHA512 (it should at least use that encryption) then all encrypted passwords would have the same size. However, I do not see any encryption of the password, so I would imagine that the length can change. You should have:

int comparison = strcmp(password, row[0]);

Assuming that the string in row[0] is NUL terminated.


Side note about vnp statement "sizeof(char) is 1" is wrong. Some processor have a sizeof(char) of 2. Not very many and for sure not the main Desktop computers, but you cannot expect that size of always be 1.see comments below

Your math is wrong:

char *querystring = malloc(strlen(query1) + strlen(username) + strlen(query2) * sizeof(char));

Here you say:

a + b + c × d

What you meant is:

(a + b + c) × d

Since d is 1 you do not see any difference, of course...

share|improve this answer
2  
Chapter and verse allowing sizeof(char) to be not 1, please. My version of Standard has 6.5.3.4:4, which says When sizeof is applied to an operand that has type char, unsigned char ,or signed char, (or a qualified version thereof) the result is 1 –  vnp Aug 29 at 23:04
    
If you assume that his C compiler is 100% C99 compliant. And people who see a compiler that is very similar to C as not a C compiler unless proven to be 100% C99, then you are correct. However, most sensible people who write in C do not care much whether the compiler is 100% compliant and there are some flavor with strange sizes because the architecture is strange... stackoverflow.com/questions/2215445/… (see answer #2 about PDP). –  Alexis Wilke Aug 29 at 23:14
    
That being said, it looks like sizeof(char) remains 1 even if the byte is not exactly 8 bits. –  Alexis Wilke Aug 29 at 23:16
    
It doesn't look like, it is. –  vnp Aug 29 at 23:18
    
@AlexisWilke Actually, even GCC isn't 100% C99 compliant; but you should develop C in concordance with the standards despite that. If a feature isn't supported, then a bug report should be filed. The the request isn't honored, then you should be using a different compiler with more apt developers. –  syb0rg Aug 30 at 0:40

To add to other peoples input:

  1. You could do without mysql_num_fields and mysql_num_rows since your SQL is always set for 1 field and 0 to 1 rows
  2. You should be making sure there is no error with mysql_errno
  3. Just like with PHP, you could also be using prepared statements (heres an example) to stop SQL injection attacks (I'm not going to code that for you)

        mysql_query(mysql_con,querystring);
    
        if (mysql_errno() != 0) {
            printf("Error #%d occurred trying to call mysql_query().\n", mysql_errno());
            return;
        }
    
        MYSQL_RES *result = mysql_store_result(mysql_con);
    
        if (mysql_errno() != 0) {
            printf("Error #%d occurred trying to call mysql_store_result().\n", mysql_errno());
            return;
        }
    
        MYSQL_ROW row;
    
        if ((row = mysql_fetch_row(result))) {
            printf("Query returned 1 results with 1 fields\n");
    
            // ...
    
        } else {
            printf("No such user... Password is invalid");
        }
    
share|improve this answer

(On top of the many excellent points in other answers)

Instead of piecing together the query string like this:

const char *query_head = "SELECT password FROM logins WHERE email = '";
const char *query_tail = "' LIMIT 1";
char *query = malloc(strlen(query_head) + strlen(query_tail) + escaped_username_len + 1);
sprintf(query, "%s%s%s", query_head, escaped_username, query_tail);
// ...
free(query);

How about using a format string + snprintf + a simple char[...], like this:

const char *format = "SELECT password FROM logins WHERE email = '%s' LIMIT 1";
int query_len = strlen(format) + escaped_username_len;
char query[query_len];
snprintf(query, query_len, format, escaped_username);
// ...
// no need anymore!
//free(query);
share|improve this answer
    
I didn't even realize I could use format strings in that manner... –  Jonny Whatshisface Aug 29 at 23:45

for example:

MYSQL_STMT *statement = mysql_stmt_init(*mysql);

char *query = "SELECT password FROM logins WHERE email = ? LIMIT 1";
mysql_stmt_prepare(statement, query, strlen(query));
MYSQL_BIND param;
memset(&param, 0, sizeof(param));
param.buffer_type = MYSQL_TYPE_STRING;
param.buffer = username;
param.length = strlen(username);
mysql_stmt_bind_param(statement, &param);
//and so on


mysql_stmt_close(statement);
share|improve this answer
    
What is bind? Is it supposed to be param? Also, how can you dereference it??? –  Alexis Wilke Aug 30 at 19:50
    
@AlexisWilke yeah mistake, and I meant & –  ratchet freak Aug 30 at 20:04

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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