Memory management with password retrieval

Questions:

• Is this code secure? (I think that it is, but I'm a newbie so I want to be sure.)

• Is the get_pass function correct with passing the arguments to the free_memory function?

• Do I have to delete the pass' buffer with memset?

```
#include
#include
#include
#include
#include
#include
#include
#include
#include

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

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.

StackExchange Code Review Q#11438, answer score: 6