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

• miracle173: What do you mean by "secure code" and why do you think your code is secure?
• polslinux: I mean that my code is not vulnerable to buffer overflow...i think that my code is secure because i think that i've done all the checks...but i'm a newbie so i'm not so sure xD

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.

• polslinux: wow O.O i don't know what to say :O i still have much to learn and understand...let's start from allocation. You have said that "Given that your user, host and pass are all small, fixed-size, and allocated for the duration of a single function" but i thought that with calloc i can made dynamic allocation so haven't to know how long is a string . I've used 64 chars because i have to put a "maximum value"...isn't it? Or i can go out of memory...
• Jerry Coffin: @polslinux: malloc/calloc/realloc allow you to allocate different sizes when needed, but right now your code only allocates one specific size -- and if that's what you're going to do, there's little reason to use them.
• polslinux: And how can i allocate something dynamically without going out of memory?
• Jerry Coffin: @polslinux: I'm not quite sure exactly what you're asking here...
• polslinux: nothing...nothing...i have to study more ;(

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)

• polslinux: Thanks! I've replaced my get_pass with the GNU one! :) ps: i can input password with space..i've tried!
• polslinux: PS: getpass function is obsolete :( man7.org/linux/man-pages/man3/getpass.3.html
• William Morris: password with space - using fscanf? I compiled your get_pass and tried it just to be sure yesterday and it does not input spaces (as expected). "getpass function is obsolete": oops!
• polslinux: O.o i'm sorry XD i've tried the other code (with getpass)!! You're right it doesn't accept spaces xD

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.