Fortify : Path Manipulation in C - White List Implementation doesn't work - fopen issue

Question

Hello everyone I have a fortify issue "Path manipulation" it produced by fopen use. According with fortify I could implement a white list in order to fix it, so there have my white list validator:

white_list.c

#define BUFF_WHITE_LIST_FILE 200
const char *white_list_validator( char *variable )
{
  FILE *fp = NULL;
  ssize_t read;
  char * line = NULL;
  size_t len = 0;
  char white_list_file_buff[BUFF_WHITE_LIST_FILE];
  if ( __secure_getenv("WHITE_LIST_FILE") == NULL )
    return NULL;
  else
  {
    strncpy(white_list_file_buff,
         __secure_getenv("WHITE_LIST_FILE"),sizeof(white_list_file_buff)-1);
    fp = fopen(white_list_file_buff,"r");
    if ( fp == NULL )
        return NULL;
    else
    {
        while( (read = getline(&line, &len, fp)) != -1 )
        {
            if ( strncmp(line,variable,read - 1) == 0 ){
                fclose(fp);
                return variable;
            }

        }
        fclose(fp);

    }
    if(line)
        free(line);
 }
 return NULL;
}

it return NULL if it doesn't find the variable inside White.list (*) or return a pointer to char if it find it

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

    FILE *fp = NULL;
    char mrd[50]={0};
    const char *ptr = white_list_validator(argv[1]);

    if ( argv[1] == NULL )
        return -1;

    if(ptr==NULL)
        return -1;
    else
    {
        strncpy(mrd,ptr,sizeof(mrd)-1);
        printf("variables found : %s\n",mrd);

        fp = fopen(mrd,"w");  <------   SINK
        if ( fp == NULL ){
                printf("line 22\n");
                exit(1);
        }
        fprintf(fp, "%s %s %s %d", "We", "are", "in", 2077);
        fclose(fp);

    }

  return 0;
 }

but when I run the fortify report appear a manipulation path vulnerability in fopen, I do not know why .You can see in the code, before that manage to file with fopen it validated for a white_list_validator. so anybody has an idea why it does not work correctly?

NOTE(*) : export WHITE_LIST_FILE=/path/White.list

cat White.list

test1

test2

something

When I run the binary:

./white_list something

variables found : something

There is an `else if` clause on line 22 of the first code, which does not follow a matching `if`. It also has a memory leak because at `return variable;` there is no `free(line)`. — Weather Vane, Jan 10 '18 at 21:39
**[cargo-cult alert]** The use of `strncpy()` here is dangerous ( it allmost always is) , since `white_list_file_buf` is not initialized with a final NUL byte. — wildplasser, Jan 10 '18 at 21:52
sorry is a typed mistake , it will be: if ( strncmp(line,variable,read - 1) == 0 ){ fclose(fp); return variable; } — Miguel Ángel Retamozo Sanchez, Jan 10 '18 at 21:54

score 3 · Accepted Answer · answered Jan 10 '18 at 21:47

A quick search of __secure_getenv lead to this page:

https://refspecs.linuxfoundation.org/LSB_1.1.0/gLSB/baselib---secure-getenv-1.html

Quote:

__secure_getenv(name) has the same specification as getenv(name) with the exception that if the program is running SUID or SGID enabled, the result is always NULL.

So, the question is: is your program running with set SUID or SGID bits? As far as I can see, __secure_getenv has been renamed to secure_getenv (my man page says it appeared in glibc 2.17). You should use that instead.

Another cause could be: if the length of the source string is longer than the size argument of strncpy, it won't add the '\0' terminating byte. When using strncpy you should always make sure to write the '\0' terminating byte.

man strcpy
#include <string.h>
char *strncpy(char *dest, const char *src, size_t n);
The strncpy() function is similar, except that at most n bytes of src are copied. Warning: If there is no null byte among the first n bytes of src, the string placed in dest will not be null-terminated.

white_list_file_buff might not be '\0'-terminated, thus fopen fails. But you say you did export WHITE_LIST_FILE=/path/White.list. Is /path/White.list the real value you used or some path that is longer than 200 characters?

Also your code here

while( (read = getline(&line, &len, fp)) != -1 )
{
    else if ( strncmp(line,variable,read - 1) == 0 ){
        fclose(fp);
        return variable;
    }

}

is either wrong or did you forget to paste the whole code? There is no previous if for that else.

Assuming that you've made a copy&paste error, how is the format if White.list? Every line contains nothing but the name of variables? Your strncmp compare the whole line you know, and if you want to match only a substring, you should use strstr.

I'll change strncmp by strstr and in the fortify report appear again path manipulation — Miguel Ángel Retamozo Sanchez, Jan 10 '18 at 22:06
@MiguelÁngelRetamozoSanchez can you post a few lines of your `White.list` file`. What is the real value of `WHITE_LIST_FILE`? — Pablo, Jan 10 '18 at 22:09
cat White.list test1 test2 something run : ./white_list something it sould be found it , becuase "something" word is inside White.list — Miguel Ángel Retamozo Sanchez, Jan 10 '18 at 22:10
@MiguelÁngelRetamozoSanchez comments don't show lines, can you please post that in your question? — Pablo, Jan 10 '18 at 22:15
@MiguelÁngelRetamozoSanchez I compiled your code (using `secure_getenv` and `strstr) and the program worked fine. I think either `WHITE_LIST_FILE` is wrong or the content of `White.lst` is wrong. — Pablo, Jan 10 '18 at 22:29
Yup me too , in the report it still apeear the path manipulation :S — Miguel Ángel Retamozo Sanchez, Jan 10 '18 at 22:32

score 1 · Answer 2 · answered Jan 11 '18 at 18:55

Fortify generally will not recognize whitelist validation as fixing the issue - once you've verified the routine, you need to teach it that this is a validation routine. This will ensure any trace that correctly validates with this routine is not reported.

You should ensure you've addressed the other comments, and validated the underlying routine itself is secure. Then you should register the whitelist routine as a validation routine by adding a custom rule via the rules editor. Best way to do this is a validation / cleansing rule that adds a taint flag of taintFlag="VALIDATED_PATH_MANIPULATION" - the sink rules for path manipulation should not report issues with this taint.

For more information on using the taint flags see https://community.softwaregrp.com/dcvta86296/attachments/dcvta86296/fortify-discussions/2950/1/HP_Fortify_SCA_Custom_Rules_Guide_4.21.pdf

VALIDATED_PATH_MANIPULATION is a fortify variable? , it means that I can use like a flag inside my white_list_validator function ? — Miguel Ángel Retamozo Sanchez, Jan 11 '18 at 20:23
It's a taint - you can taint a flow with any value via a rule, but some values (such as the validated taints) are recognized by the Fortify native rules and they will not report issues that are marked as validated. — Egret, Jan 13 '18 at 17:12

score 0 · Answer 3 · answered Jan 10 '18 at 21:55

Your whitelist approach approach obtains the name of the whitelist from an environment variable. This is so easily manipulable as to not even justify the added complexity of implementing a whitelist in the first place, and it certainly should be cause for its own path-manipulation vulnerability. Using __secure_getenv() to obtain the value doesn't really help; it just means that your program won't work if run in a secure context.

Even if the whitelist's own path were fixed at compile time, you're still obtaining the whitelisted paths from an external source. That's harder to manipulate than totally unverified user input, but it doesn't really resolve the vulnerability.

If you cannot compile your whitelist into the program, then consider instead hardcoding a whitelist of path prefixes and a whitelist of characters that may appear in the remaining path components. It's unclear whether Fortify would actually recognize this as resolving the issue, but that would be sufficient for me to be satisfied to set the issue's analysis to "Not an issue".

I'll put __secure_getenv insted getenv because fortify don't take like a vulnerability , but I put the complite path insted __secure_getenv and fortify doesn't take the a vulnerability — Miguel Ángel Retamozo Sanchez, Jan 10 '18 at 22:15

Fortify : Path Manipulation in C - White List Implementation doesn't work - fopen issue

3 Answers3