passing tainted variable to a tainted sink

Question

I am getting the Coverity warning Untrusted value as argument with the below code when I pass argv as an argument to test_run(). It says tainted variable passed to tainted sink but I am not sure how to make argv not-tainted!

#include<stdio.h>
#include<string.h>
#include<stdlib.h>

int
main (int argc, char **argv)
{

    /* some code  */

//    char **nstr;
//    nstr = malloc(sizeof(char*));
//    if(!nstr) {
//        return 1;
//    }
//    int i=0;
//    for(i=0;i<argc;i++){
//        nstr[i] = strdup(argv[i]);
//    }

      test_run(argc-1, argv, testf);

}

int test_run(int argc, char **argv, testf_handle testf)
{
    if (!argv) {
        return 1;
    }

    /* some code */

    testf->prog_name = argv[0];
    parse_context(&pc, argc, argv);

    /*- - other code - -*/

}

I tried by using the commented code and pass nstr insted of argv but still getting same warning when at line:

nstr[i] = strdup(argv[i]);

I am not sure what sanity check I should do before passing it as an argument. I am using gcc compiler on a linux machine.

testf is an instance of structure test_framework which has several member variables.

Thanks.

Answer 1

You can make argv not tainted by checking it to ensure it conforms to some particular specification. For example, checking the length of the string under argv to ensure it's less than some upper limit, ensuring it doesn't contain bad character sequences, etc.

Coverity will detect your sanitization attempts and the defect will be resolved once you actually are sanitizing your inputs.

Answer 2

I have not used Coverity, but have used other static analysis tools and they all have similarities with each other.

In general, tools like this are dumb. They raise a warning when they see taint flowing from source to sink, yet they have no way of knowing whether the data has been sanitised or not. It requires a human reviewer to check whether the warning is real or (all too often) a false positive.

EDIT: @Caleb claims that Coverity can detect data sanitisation -- see his comment below.

The tool assumes inputs such as command line arguments ( argv ) are a security risk. The first question is whether that is a correct assumption in the context of your application. If this is test code (it looks like it may be), then that assumption is wrong and you can flag the warning as a false positive. Otherwise, let's go ahead on the grounds that the assumption is true.

Now Coverity is panicking because something is happening with that taint, and it is thinking that something dangerous can happen with the strdup(). What is the danger here? Normally the tool tells you what the risk here (examples: buffer overflow, DoS due to excessive memory allocation, etc...).

I'm not seeing a buffer overflow risk here, but I could be wrong. Perhaps the warning is DoS due to excessive memory allocation. If that is the problem, then the solution is to write code that rejects unreasonable sized inputs. In general, the solution is to write code that addresses the problem that Coverity is panicking about, and thus you have sanitised your inputs.

However, once you code up the solution, Coverity might still give you the same warning. The problem is that although you have sanitised your input, the dumb tool cannot detect that. This is where the human code reviewer must step in and tell the tool that the problem is dealt with, and hence suppress the warning.

Bottom line: the tools are there to aid a human code reviewer, not replace one. Don't try to use them in an automated way without assistance from the human security expert. (EDIT: @Caleb is suggesting that Coverity does quite well in pruning out false positives.)