C lang CGI form/string handle

Question

Started playing with C and CGI forms. Is this code ok? Can it be improved/optimized?

If I fill user with "user" and password with "pass" input in takelogin is user=user&password=pass

In the end I want to have user = 'user' and pass = 'pass'

This is the form

#include <stdio.h>

int main(void)
{
    printf("%s%c%c\n", "Content-Type:text/html;charset=iso-8859-1",13,10);
    printf("<title>Takelogin</title>\n");
    printf("<h3>Multiplication results</h3>\n");
    printf("%s\n", "<form action='/takelogin.cgi' method='post'>");
    printf("%s\n", "<div>Your input (80 chars max.):<BR>");
    printf("%s\n", "<input type='text' name='user' size='60' maxlength='80'><br>");
    printf("%s\n", "<input type='password' name='password' size='60' maxlength='80'><br>");
    printf("%s\n", "<input type='submit' value='Send'></div>");
    printf("%s\n", "</form>");
    return 0;
}

And this is takelogin

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

#define MAXINPUT 80

int main(void)
{
    char *lenstr;
    char *user;
    char *pass;
    char input[MAXINPUT];
    long len;
    char delim[2][2] = {"&", '='};

    printf("%s%c%c\n",
    "Content-Type:text/html;charset=iso-8859-1",13,10);
    printf("<TITLE>Response</TITLE>\n");

    lenstr = getenv("CONTENT_LENGTH");
    if(lenstr == NULL)
        printf("<P>Error in invocation - wrong FORM probably.");
    else {
        fgets (input , sizeof(input) , stdin);

        char *loginS = malloc(sizeof(input));
        strcpy(loginS, input);

        user = strtok(loginS, delim[0]);
        user = strtok(user, delim[1]);
        user = strtok(NULL, delim[1]);

        pass = strtok(input, delim[0]);
        pass = strtok(NULL, delim[0]);
        pass = strtok(pass, delim[1]);
        pass = strtok(NULL, delim[1]);
        printf("%s@%s", user, pass);
        free(loginS);
        return 0;
    }
    return 0;
}

Answer 1

Some bugs:

• malloc() can fail. When it does, it returns a null pointer, which we must not use as destination for strcpy().
• The fixed-length input array can be overrun by the remote user. That's a pattern that causes security problems in server code (which this is, via CGI). Our strategy to avoid that (by reading only sizeof input - 1 chars) means that we silently change the user's input.

Other issues:

• Instead of reading into input and then copying into loginS, we should allocate memory, then (if successful) call fgets(loginS, size, stdin).

• We should be testing for valid input before we output any headers, as we should be returning a HTTP error code (probably 400 Bad Request) rather than success (200).

• We could parse lenstr, and if successful, use that value to allocate a suitable size buffer instead of sizeof input chars (which will always be MAXINPUT). Don't forget + 1 for a terminating null char!

• I don't think the delim array is helpful - it would be clearer to put the literal strings in the code there. I know that breaks the rule of "no magic values", but I think that here the rule just obfuscates rather than clarifies the code.

• If the printf() fails, it might be a good idea to return EXIT_FAILURE, so the CGI engine knows that something has gone wrong (the user hasn't got their result).

• Instead of if/else, I prefer an early-return pattern:

    if (error_condition) {
        report_error();
        cleanup_resources();
        return EXIT_FAILURE;
   }
  

  That means less context to hold in mind whilst reading.