Menu

#18 important issues found by covscan

v1.0_(example)
closed-fixed
None
5
2019-06-13
2018-11-10
Than Ngo
No
Error: SECURE_TEMP (CWE-377):
dos2unix-7.4.0/common.c:992: secure_temp: Calling "mkstemp" without securely setting umask first.
#  990|       goto make_failed;
#  991|   #else
#  992|->   if ((fd = mkstemp(fname_str)) == -1)
#  993|       goto make_failed;
#  994|   

Error: RESOURCE_LEAK (CWE-772):
dos2unix-7.4.0/common.c:959: alloc_fn: Storage is returned from allocation function "strdup".
dos2unix-7.4.0/common.c:959: var_assign: Assigning: "cpy" = storage returned from "strdup(OutFN)".
dos2unix-7.4.0/common.c:975: identity_transfer: Passing "cpy" as argument 1 to function "dirname", which returns that argument.
dos2unix-7.4.0/common.c:975: noescape: Resource "cpy" is not freed or pointed-to in "dirname".
dos2unix-7.4.0/common.c:975: var_assign: Assigning: "dir" = storage returned from "dirname(cpy)".
dos2unix-7.4.0/common.c:977: noescape: Resource "dir" is not freed or pointed-to in "strlen".
dos2unix-7.4.0/common.c:1004: leaked_storage: Variable "dir" going out of scope leaks the storage it points to.
dos2unix-7.4.0/common.c:1004: leaked_storage: Variable "cpy" going out of scope leaks the storage it points to.
# 1002|       free(*fname_ret);
# 1003|       *fname_ret = NULL;
# 1004|->     return NULL;
# 1005|   }
# 1006|   

Error: CHECKED_RETURN (CWE-252):
dos2unix-7.4.0/common.c:1979: check_return: Calling "fputc" without checking return value (as is done elsewhere 20 out of 22 times).
dos2unix-7.4.0/common.c:2675: example_checked: Example 1: "fputc(c_lead, f)" has its value checked in "fputc(c_lead, f) == -1".
dos2unix-7.4.0/common.c:2750: example_checked: Example 2: "fputc(0, f)" has its value checked in "fputc(0, f) == -1".
dos2unix-7.4.0/common.c:2792: example_checked: Example 3: "fputc(mbs[i], f)" has its value checked in "fputc(mbs[i], f) == -1".
dos2unix-7.4.0/dos2unix.c:147: example_checked: Example 4: "fputc(CurChar, ipOutF)" has its value checked in "fputc(CurChar, ipOutF) == -1".
dos2unix-7.4.0/dos2unix.c:154: example_checked: Example 5: "fputc(10, ipOutF)" has its value checked in "fputc(10, ipOutF) == -1".
# 1977|       }
# 1978|       if (ipFlag->file_info & INFO_PRINT0)
# 1979|->       fputc(0, stdout);
# 1980|       else
# 1981|         D2U_UTF8_FPRINTF(stdout, "\n");

Error: CHECKED_RETURN (CWE-252):
dos2unix-7.4.0/common.c:2010: check_return: Calling "fputc" without checking return value (as is done elsewhere 20 out of 22 times).
dos2unix-7.4.0/common.c:2675: example_checked: Example 1: "fputc(c_lead, f)" has its value checked in "fputc(c_lead, f) == -1".
dos2unix-7.4.0/common.c:2750: example_checked: Example 2: "fputc(0, f)" has its value checked in "fputc(0, f) == -1".
dos2unix-7.4.0/common.c:2792: example_checked: Example 3: "fputc(mbs[i], f)" has its value checked in "fputc(mbs[i], f) == -1".
dos2unix-7.4.0/dos2unix.c:147: example_checked: Example 4: "fputc(CurChar, ipOutF)" has its value checked in "fputc(CurChar, ipOutF) == -1".
dos2unix-7.4.0/dos2unix.c:154: example_checked: Example 5: "fputc(10, ipOutF)" has its value checked in "fputc(10, ipOutF) == -1".
# 2008|     }
# 2009|     if (ipFlag->file_info & INFO_PRINT0)
# 2010|->     fputc(0, stdout);
# 2011|     else
# 2012|       D2U_UTF8_FPRINTF(stdout, "\n");

Error: CLANG_WARNING:
dos2unix-7.4.0/common.c:2542:11: warning: Value stored to 'conversion_error' is never read
#          conversion_error = GetFileInfo(argv[ArgIdx], pFlag, progname);
#          ^                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
dos2unix-7.4.0/common.c:2542:11: note: Value stored to 'conversion_error' is never read
#          conversion_error = GetFileInfo(argv[ArgIdx], pFlag, progname);
#          ^                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# 2540|         } else {
# 2541|           if (pFlag->file_info) {
# 2542|->           conversion_error = GetFileInfo(argv[ArgIdx], pFlag, progname);
# 2543|             print_messages_info(pFlag, argv[ArgIdx], progname);
# 2544|           } else {

Error: TAINTED_SCALAR (CWE-20):
dos2unix-7.4.0/dos2unix.c:383: tainted_data_return: Function "fgetc" returns tainted data.
dos2unix-7.4.0/dos2unix.c:383: var_assign: Assigning: "TempChar" = "fgetc", which taints "TempChar".
dos2unix-7.4.0/dos2unix.c:384: lower_bounds: Checking lower bounds of signed scalar "TempChar" by "TempChar < 32".
dos2unix-7.4.0/dos2unix.c:402: tainted_data: Using tainted variable "TempChar" as an index to pointer "ConvTable".
#  400|               if (TempChar == '\x0a') /* Count all DOS and Unix line breaks */
#  401|                 ++line_nr;
#  402|->             if (fputc(ConvTable[TempChar], ipOutF) == EOF) {
#  403|                 RetVal = -1;
#  404|                 d2u_putc_error(ipFlag,progname);

Error: TAINTED_SCALAR (CWE-20):
dos2unix-7.4.0/dos2unix.c:420: tainted_data_return: Function "fgetc" returns tainted data.
dos2unix-7.4.0/dos2unix.c:420: var_assign: Assigning: "TempChar" = "fgetc", which taints "TempChar".
dos2unix-7.4.0/dos2unix.c:421: lower_bounds: Checking lower bounds of signed scalar "TempChar" by "TempChar < 32".
dos2unix-7.4.0/dos2unix.c:439: tainted_data: Using tainted variable "TempChar" as an index to pointer "ConvTable".
#  437|                 if (TempChar == '\x0a') /* Count all DOS and Unix line breaks */
#  438|                   ++line_nr;
#  439|->               if(fputc(ConvTable[TempChar], ipOutF) == EOF) {
#  440|                   RetVal = -1;
#  441|                   d2u_putc_error(ipFlag,progname);

Error: TAINTED_SCALAR (CWE-20):
dos2unix-7.4.0/unix2dos.c:337: tainted_data_return: Function "fgetc" returns tainted data.
dos2unix-7.4.0/unix2dos.c:337: var_assign: Assigning: "TempChar" = "fgetc", which taints "TempChar".
dos2unix-7.4.0/unix2dos.c:338: lower_bounds: Checking lower bounds of signed scalar "TempChar" by "TempChar < 32".
dos2unix-7.4.0/unix2dos.c:379: tainted_data: Using tainted variable "TempChar" as an index to pointer "ConvTable".
#  377|             if (TempChar == '\x0a') /* Count all DOS and Unix line breaks */
#  378|               ++line_nr;
#  379|->           if (fputc(ConvTable[TempChar], ipOutF) == EOF) { /* put LF or other char */
#  380|                 RetVal = -1;
#  381|                 d2u_putc_error(ipFlag,progname);

Error: TAINTED_SCALAR (CWE-20):
dos2unix-7.4.0/unix2dos.c:397: tainted_data_return: Function "fgetc" returns tainted data.
dos2unix-7.4.0/unix2dos.c:397: var_assign: Assigning: "TempChar" = "fgetc", which taints "TempChar".
dos2unix-7.4.0/unix2dos.c:398: lower_bounds: Checking lower bounds of signed scalar "TempChar" by "TempChar < 32".
dos2unix-7.4.0/unix2dos.c:414: tainted_data: Using tainted variable "TempChar" as an index to pointer "ConvTable".
#  412|             }
#  413|             if (TempChar != '\x0a') { /* Not an LF */
#  414|->             if(fputc(ConvTable[TempChar], ipOutF) == EOF) {
#  415|                 RetVal = -1;
#  416|                 d2u_putc_error(ipFlag,progname);

Discussion

  • Than Ngo

    Than Ngo - 2018-11-10

    the following patch should fix the RESOURCE_LEAK and CHECKED_RETURN errors.
    I'm not sure to fix the Error: TAINTED_SCALA. I know we need to check the TempChar before calling fputc.

    diff -up dos2unix-7.4.0/common.c.me dos2unix-7.4.0/common.c
    --- dos2unix-7.4.0/common.c.me  2018-11-10 12:54:36.357255451 +0100
    +++ dos2unix-7.4.0/common.c     2018-11-10 13:36:09.521902844 +0100
    @@ -981,6 +981,7 @@ FILE* MakeTempFileFrom(const char *OutFN
       *fname_ret = fname_str;
    
       free(cpy);
    +  cpy = NULL;
    
     #ifdef NO_MKSTEMP
       if ((name = d2u_mktemp(fname_str)) == NULL)
    @@ -999,6 +1000,10 @@ FILE* MakeTempFileFrom(const char *OutFN
       return (fp);
    
       make_failed:
    +    if (cpy) {
    +       free(cpy);
    +       cpy = NULL;
    +    }
         free(*fname_ret);
         *fname_ret = NULL;
         return NULL;
    @@ -1976,7 +1981,7 @@ void printInfo(CFlag *ipFlag, const char
           D2U_UTF8_FPRINTF(stdout, "FILE");
         }
         if (ipFlag->file_info & INFO_PRINT0)
    -      fputc(0, stdout);
    +      (void) fputc(0, stdout);
         else
           D2U_UTF8_FPRINTF(stdout, "\n");
         header_done = 1;
    @@ -2007,7 +2012,7 @@ void printInfo(CFlag *ipFlag, const char
         D2U_UTF8_FPRINTF(stdout, "%s",ptr);
       }
       if (ipFlag->file_info & INFO_PRINT0)
    -    fputc(0, stdout);
    +    (void) fputc(0, stdout);
       else
         D2U_UTF8_FPRINTF(stdout, "\n");
     }
    
     
  • Erwin Waterlander

    Hi Than,

    Thanks for reporting.
    I will have a look soon.

    best regards,
    Erwin

     
  • Erwin Waterlander

    The range of TempChar is correct (0..255), because it is an unsigned char cast to an int.
    Adding a check is not needed and will slow dos2unix down.
    I propose to waive that error.

     
  • Erwin Waterlander

    Fix uploaded. See [48217a]

     

    Related

    Commit: [48217a]

  • Erwin Waterlander

    • status: open --> closed-fixed
     

Anonymous
Anonymous

Add attachments
Cancel





Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.