sbhatla sbhatla - 10 months ago 152
C Question

Possible security vulnerability from using fgets() and recommended solution?

I'm using coverity's SA tool for errors. I'm getting a few errors due to the usage of fgets(). This is a snippet (SA errors shown as comments)-

FILE *fp;
char my_pubkey[1024];

fp = fopen("", "r");

//tainted_string_argument: fgets taints variable my_pubkey.
if (!fgets(my_pubkey, sizeof(my_pubkey), fp)) {
printf("failure to read pub key file");
goto error;

//tainted_string: Passing tainted string my_pubkey to a parameter that cannot accept a tainted format string.
if (fprintf(fp, my_pubkey) != strlen(my_pubkey)) {
printf ("failure to write pub key in key file");
goto error;

In my investigation, some reports suggest using getline() instead, but is that really needed? If this is a valid issue, what can be the vulnerability? And what's the best solution?

EDIT: If this is a false positive, why so? What can be an example of when it would be an actual issue?


Coverity seems to be complaining that my_pubkey is receiving a value from an external source. It is therefore "tainted", because the program cannot inherently be confident that the data so received are correct or valid. This is a genuine concern that you may simply need to manage. I wouldn't expect using getline() instead of fgets() to change that -- that would be one way to address a different problem involving a different function (gets()).

Coverity is also complaining that you are passing your tainted string to printf() as a format string. This is also a bona fide security concern, and maybe even a simple correct functionality concern. It is a very bad idea to use an externally supplied string as a [f]printf() format string, because such a string may contain printf() field codes. You should instead either provide an explicit format:

fprintf(fp, "%s", my_pubkey)

or use fputs():

fputs(my_pubkey, fp)