Tainted string message from Coverity using getenv

Question

Running Coverity on my code results in tainted string error message. I am using the "path" variable declared in the stack, so I am not sure why I am seeing errors. I can only think that using getenv() directly in the strncpy() is causing the error. Would the fix below eliminate this error?

char path[1024] = {NULL, };
if(getenv("A"))
    strncpy(path, getenv("A"), strlen(getenv("A")));

to

char path[1024] = {NULL, };
char * adriver = getenv("A");
if(adriver)
    strncpy(path, adriver, strlen(adriver));

Overmeasure

It is not a good idea to use NULL as an initializer for a char, especially if the implementation defines NULL as ((void *)0), which is a legitimate value in C (but not legitimate in C++). You could use char path[1024] = ""; instead. You are also abusing strncpy(); you should be limiting it to the length of path (minus 1). If someone runs your code with a PATH that is more than 1023 characters long, you will overflow the array and may end up crashing and will not get a null-terminated string. It is unlikely that either of these is a factor in the 'tainted string' message. – Medalist

Having looked at the code again, I realize you're doing a superfluous memset() after initializing path to all bytes zero. – Medalist

to add to @JonathanLeffler sir's comment, the second approach is better, because it saves you two redundant getenv() calls. – Protestantism

At one time, JS1 observed — accurately — that: In the second piece of code, you should realize that if (!adriver) is only true if adriver is NULL. In other words, you have reversed the meaning of the if statement from the first piece of code. You probably meant if (adriver). – Medalist

@JonathanLeffler thanks, I made the changes. But I am still not sure if this eliminates tainted string issue. – Overmeasure

@JayChung: I am not surprised that the changes I suggested didn't resolve the tainted strings problem. I don't know anything specific about Coverity and tainted strings. By analogy with Perl (a dangerous game to play), I'm assuming that you have to do some analysis of the value returned by getenv() before you do much with it in order to get rid of the taintedness. But I've not gone manual bashing to find out more -- you can do that about as well as I can. – Medalist

You might find Tainted string in C of some help. A Google search on terms 'coverity tainted string' shows a number of other related items. I haven't found a definitive answer, but neither have I visited the Coverity site to see what manuals are available. – Medalist

Answer 1

No, this will probably not fix the error.

Coverity is telling you that the data within environment variable "A" could be pretty much anything; this data is not under the control of your program.

Therefore, you need to have some sanity checks on the data before you use it.

Your proposed fix will currently have a buffer overflow, if somebody sets environment variable A to a string containing 1025 characters.

Additionally, neither version of code will ever NUL-terminate the "path" string. This is because, you are using strncpy, which will not NUL-terminate if the byte limit is applied (which it will in this case, because you say "limit the copied string to the length I just got from the string").

What you should be doing, is size-check the string first. If it is too large, return some sort of error code; the path in variable A is too big, so your code will not function as desired. If it is not too large, copy it into the path buffer. If you want to use strncpy, make sure to leave room for a NUL at the end, and then explicitly add it, since strncpy is not guaranteed to put a NUL there.

Carpophagous

You are 100% right on why Coverity complained. But, there is no reason to use strncpy here. In fact, it is only useful for a little tiny corner case. You have already computed the length to know if it is going to fit. It's probably much more efficient to just use memcpy to copy it, then manually add the NUL at the end. – Lusty

The main reason strncpy() is probably not useful is that if the contents of the environment variable are legitimate but too long, then truncating the value is the wrong thing to do. Such truncation could be a security issue in itself, but in any case is unlikely to produce the desired beharior. The program should terminate with an error instead, or at least reject the overlength value altogether. – Roger

The end of a string is 0, not NULL. – Duplication

You're right...to be pedantic a zero is an ASCII "NUL", not " NULL". Corrected. ;-) – Carpophagous

To be ultra-pedantic, a C string is terminated with a 0 byte, which is not an ASCII "NUL" character. On some processors (namely DSPs), bytes are 32 bits. ASCII is not. – Stiegler

Answer 2

Your code is incorrect: you have a potential buffer overflow in both alternatives.

I am not sure Coverity diagnoses the problem correctly, you did not post the exact error message. Coverity is possibly indicating that you use a string from the environment, that could have any length, potentially causing a buffer overflow when copied by your code into a 1024 byte buffer, indeed it is a good thing it pointed you to this. Here is why:

strncpy does not do what you think it does. This function should never be used, its semantics are error prone, it is not the right tool for your purpose. strncpy(dest, src, n) copies no more than n chars from src to dest and fills the rest of the array at dest with '\0' bytes until n bytes have been written. dest must point to an array of at least n chars. If src is shorter, the behavior is inefficient as the padding is usually unnecessary, but if src has a length of at least n, dest will not be null terminated by strncpy, leading to undefined behavior in many cases.

Your code:

char path[1024] = { NULL, };
if (getenv("A"))
    strncpy(path, getenv("A"), strlen(getenv("A")));

is equivalent to

char path[1024] = { NULL, };
if (getenv("A"))
    memcpy(path, getenv("A"), strlen(getenv("A")));

As you see, no real protection is granted.

You call getenv 3 times, it would indeed be more efficient to use your alternative implementation, but there are other problems:

You initialize path with { NULL, }. This is inconsistent and in many cases incorrect. NULL is commonly #defined as ((void*)0), thus an invalid initializer for char. path could be initialized this way:

char path[1024] = { 0 };

To avoid overflowing the destination buffer, use this code:

char path[1024] = { 0 };
char *p = getenv("A");
if (p != NULL) {
    strncat(path, p, sizeof(path) - 1);
}

But this would truncate the environment value, which may not be appropriate depending on how you use path.

An alternative is to use the environment value directly:

char *path = getenv("A");
if (path == NULL)
    path = "";

And if you change the environment values with setenv during the execution of your program, you might want to make a copy of the environment value with path = strdup(path);. This might also fix the tainted string warning from Coverity, although the copy would be the same size as the original and enough memory might not be available, which should be tested. Judging from Tainted string in C , it seems Coverity is a bit extreme about tainted strings. While the warning you get indicates a real problem, getting rid of the warning may sometimes require strange workarounds.

Jaimeejaimes

so far only 2 things got rid of the coverity warning for me: [1] in C use strdup followed by a call to a macro that did an snprintf [2] in C++ << to a stringstream and then return .str(). Not sure why strdup by itself didn't work in the C++ file. – Padauk

Answer 3

char *path = getenv("A");

isn't that all you need? Is there a reason you need to make a copy of what getenv returns? If so,

char *path = NULL, *A = getenv("A");

if(A != NULL)
   {
   path = malloc(strlen(A)+1);
   strcpy(path, A);
   }

Odoacer

Use strdup for copying a string and allocating memory at the same time. – Duplication

Simple solution, but with a semantic difference: path is NULL as opposed to "" if the environment does not have a value for A. – Jaimeejaimes

strdup... us old folks didn't have such niceties... Had to walk uphill both ways just to go to school 'n' all... you young folks got it good... Where's my spitoon??? :-P – Odoacer

Answer 4

Would the fix below eliminate this error?

No, but this almost certainly would:

char *e = getenv("A");
char path[e ? strlen(e) + 1 : 1];
strcpy(path, e ? e : "");

The main problem with your code occurs when getenv("A") returns a string that's 1024 bytes or longer; copying this into path as you are results in a buffer overflow, as it isn't guaranteed that strlen(getenv("A")) will be less than or equal to 1024. You could fix this by using:

strncpy(path, getenv("A"), sizeof path); // NOT RECOMMENDED

... but then path won't be terminated with a '\0' character; path won't be a string in this situation, because by definition a string must be terminated with a '\0' character... This means you wouldn't be able to use it as a string.

That is what Coverity is most likely complaining about here, and my code eliminates the problem by using a VLA (variable length array) that's large enough to store the entire string, plus the '\0' at the end of the string.

Kinkajou