Tainted string in C

Question

I'm running Coverity tool in my file operation function and getting the following error.

As you can see below, I'm using an snprintf() before passing this variable in question to the line number shown in the error message. I guess that some sanitization of the string has to be done as a part of that snprintf(). But still the warning is shown.

Error:TAINTED_STRING (TAINTED string "fn" was passed to a tainted string sink content.) [coverity]

char fn[100]; int id = 0;
char* id_str = getenv("ID");
if (id_str) {
    id = atoi(id_str);
}
memset(fn, '\0', sizeof(fn));
snprintf(fn, 100, LOG_FILE, id);
if(fn[100-1] != '\0') {
     fn[100-1] = '\0';
}
log_fp = fopen (fn, "a");

Any help would be highly appreciated.

Solution 1

Try the following:

char* id_str = getenv("ID");
if (id_str) {
   id_str = strdup(id_str);
   id = atoi(id_str);
   free( id_str );
}

The fn string passed to fopen is tainted by an environment variable. Using strdup may act as "sanitizing".

Solution 2

Error:TAINTED_STRING is warning that (as far as Coverity can tell) some aspect of the behaviour is influenced by some external input and that the external input is not examined for 'safeness' before it influences execution.

In this particular example it would appear that Coverity is wrong because the value of LOG_FILE is "/log/test%d.log" and is used with an int in the snprintf, meaning that the content of char fn[100] is always well defined.

So a reasonable course of action would be to mark the error as a non-issue so that it is ignored on future runs.

Solution 3

Coverity wants to make sure you sanitize any string which is coming from outside of your program, be it getenv, argv, or from some file read.

You may have a function to sanitize the input(Tainted string) and have a comment provided by Coverty which tells Coverty that input string is sanitized and the SA warning will go away.

// coverity[ +tainted_string_sanitize_content : arg-0 ]
int sanitize_mystring(char* s) 
{
    // Do some string validation
    if validated()
        return SUCCESS;
    else
        return FAILED;
}

// coverity[ +tainted_string_sanitize_content : arg-0 ] is the line Coverty is looking

Hope this helps.

Comments

[SimonD]

The given answer introduces a bug: if getenv("ID") returns NULL then the behaviour of strdup is undefined as far as I can tell from the man page. (And crashes for me in practice)

[SimonD]

@manuell It addresses my point, however I would preferably avoid using C functions altogether in favour of C++ std::string etc... but the original question appears to be C so... Much easier to write Coverity-clean code in C++ than C IMO

[SimonD]

And I guess the other point is 'act as sanitizing' rather than actually address the TAINTED_STRING error. For example if this code is running with elevated privileges it could cause system files to be opened and written to.

[manuell]

@SimonD The file name is built with the LOG_FILE string and a "tainted" integer. I don't see how you could open system files. How would you fix the problem (in C)?

[SimonD]

Ah yes. I see now in a comment that the value of LOG_FILE is "/log/test%d.log" and is used with an int in the printf - didn't really read the question properly before and was talking too generally.

[SimonD]

I've added my own answer to suggest not adding code to fix what is apparently a mis-diagnosis by Coverity. @manuell nice talking to you!