Memory management with password retrieval

Question

Questions:

1. Is this code secure? (I think that it is, but I'm a newbie so I want to be sure.)
2. Is the get_pass function correct with passing the arguments to the free_memory function?
3. Do I have to delete the pass' buffer with memset?

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <termios.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netdb.h>
#include <netinet/in.h>

void get_pass(char **host, char **user, char **pass);
void free_memory(char *h, char *u, char *p);

int main(){

    char *host, *user, *pass;

    host = (char *) calloc(64, sizeof(char)); /* spazio per max 64 caratteri e inizializzo a 0 (maggior sicurezza) */
    if(!host){
        fprintf(stdout, "\nErrore di allocazione della memoria\n");
        exit(EXIT_FAILURE);
    };

    user = (char *) calloc(64, sizeof(char));
    if(!user){
        fprintf(stdout, "\nErrore di allocazione della memoria\n");
        exit(EXIT_FAILURE);
    };

    pass = (char *) calloc(64, sizeof(char));
    if(!pass){
        fprintf(stdout, "\nErrore di allocazione della memoria\n");
        exit(EXIT_FAILURE);
    };

    /* Immissione di hostname, username e password.
     * Controllo inoltre i 'return code' dei vari fscanf e, se non sono 0, esco.
     * Per evitare buffer overflow imposto limite massimo a 64 caratteri
     */
    fprintf(stdout,"--> Inserisci hostname (max 64 caratteri): ");
    if(fscanf(stdin, "%63s", host) == EOF){
        fprintf(stdout, "\nErrore, impossibile leggere i dati\n");
        free_memory(host,user,pass);
        exit(EXIT_FAILURE);
    }
    fprintf(stdout,"\n--> Inserisci username (max 64 caratteri): ");
    if(fscanf(stdin, "%63s", user) == EOF){
        fprintf(stdout, "\nErrore, impossibile leggere i dati\n");
        free_memory(host,user,pass);
        exit(EXIT_FAILURE);
    };
    fprintf(stdout, "\n--> Inserisci password (max 64 caratteri): ");
    get_pass(&host,&user,&pass);

    /* Stampo a video le informazioni immesse */
    fprintf(stdout, "\n\nHost: %s\nUser: %s\nPass: %s\n\n", host,user,pass);

    free_memory(host,user,pass);

    return EXIT_SUCCESS;
}

void get_pass(char **host, char **user, char **pass){
    /* Grazie a termios.h posso disabilitare l'echoing del terminale (password nascosta) */
    struct termios term, term_orig;
    tcgetattr(STDIN_FILENO, &term);
        term_orig = term;
        term.c_lflag &= ~ECHO;
        tcsetattr(STDIN_FILENO, TCSANOW, &term);
        /* Leggo la password e controllo il 'return code' di fscanf */
        if(fscanf(stdin, "%63s", *pass) == EOF){
        fprintf(stdout, "\nErrore, impossibile leggere i dati\n");
        tcsetattr(STDIN_FILENO, TCSANOW, &term_orig);
        free_memory(*host, *user, *pass);
        exit(EXIT_FAILURE);
    };
        /* Reimposto il terminale allo stato originale */
        tcsetattr(STDIN_FILENO, TCSANOW, &term_orig);
}

void free_memory(char *h, char *u, char *p){
    /* Libero la memoria occupata */
    free(h);
    free(u);
    free(p);
}

Answer 1

I'm not sure it's exactly insecure, but I'd consider it excessively verbose and repetitive. Given that your user, host and pass are all small, fixed-size, and allocated for the duration of a single function, dynamic allocation seems to gain little (and cost quite a bit of verbosity) with them. I think I'd write something like this:

void getprompt(char *string, size_t max, char const *prompt) { 
    char buffer[16];

    sprintf(buffer, "%%%ds", max-1);
    printf("%s", prompt);
    if (scanf(buffer, string) == EOF) {
        printf("\nErrore, impossibile leggere i dati\n"); 
        exit(EXIT_FAILURE);
    }       
}

int main(){
    char host[64], user[64], pass[64];

    getprompt(host, sizeof(host), "--> Inserisci hostname (max 64 caratteri): ");
    getprompt(user, sizeof(user), "\n--> Inserisci username (max 64 caratteri): ");

    printf("\n--> Inserisci password (max 64 caratteri): ");
    get_pass(&host,&user,&pass);

    /* Stampo a video le informazioni immesse */
    printf("\n\nHost: %s\nUser: %s\nPass: %s\n\n", host,user,pass);

    return EXIT_SUCCESS;
}

There also seems to be no reason to pass host or user to get_pass -- it doesn't really use them (and logically, shouldn't). I'd probably write it as a wrapper around getprompt -- do the tcsetattr, call getprompt, then undo the tcsetattr. Although you have matched the sizes correctly in this case, your get_pass has implicit knowledge of the size of pass; it works as-is, but is relatively fragile, so in the long term it would be fairly easy for somebody to create a hole by adjusting the size in get_pass but not in main (or vice versa).

I'd also consider changing getprompt to return a bool indicating success/failure rather than having it directly exit the program. Along with the suggested change to getpass, that would turn your main into something like:

int main() {
    char host[64], user[64], pass[64];

    if (getprompt(host /* ... */) &&
        getprompt(user /* ... */) &&
        get_pass(pass /* ... */) 
    {
        printf("\n\nHost: %s\nUser: %s\nPass: %s\n\n", host, user, pass);
    }
    return EXIT_SUCCESS;
};

Bottom line: I don't see any obvious security holes in the code as it stands, but it strikes me as excessively verbose and rather fragile. It's unnecessarily difficult to be certain that it's correct now, and likely to get broken in the long term even if it is all correct now.

Answer 2

A few more comments to add to JC's:

• Your use of fscanf in get_pass prevents the user from using a pass phrase containing spaces. fgets might be a better choice.

• The GNU equivalent of your get_pass function (http://www.gnu.org/software/libc/manual/html_node/getpass.html) uses TCSAFLUSH not TCSANOW. Not sure whether this has any security implications.

• There might be a getpass() on your system (OS-X has one)

Answer 3

In addition to what has already been said, never typecast the result of malloc/calloc in C. That is dangerous practice. Read this and this.