dos2unix / Bugs / #18 important issues found by covscan

#18 important issues found by covscan

Milestone: v1.0_(example)

Status: closed-fixed

Owner: Erwin Waterlander

Labels: None

Priority: 5

Updated: 2019-06-13

Created: 2018-11-10

Creator: Than Ngo

Private: 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

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");
 }

Anonymous

Add attachments
Cancel
You seem to have CSS turned off. Please don't fill out this field.

You seem to have CSS turned off. Please don't fill out this field.

Erwin Waterlander - 2018-11-14

Hi Than,

Thanks for reporting.
I will have a look soon.

best regards,
Erwin

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Anonymous
  
  Add attachments
  Cancel
  You seem to have CSS turned off. Please don't fill out this field.
  
  You seem to have CSS turned off. Please don't fill out this field.

Erwin Waterlander - 2018-12-01

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.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Anonymous
  
  Add attachments
  Cancel
  You seem to have CSS turned off. Please don't fill out this field.
  
  You seem to have CSS turned off. Please don't fill out this field.

Erwin Waterlander - 2018-12-01

Fix uploaded. See [48217a]

Related

Commit: [48217a]

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Anonymous
  
  Add attachments
  Cancel
  You seem to have CSS turned off. Please don't fill out this field.
  
  You seem to have CSS turned off. Please don't fill out this field.

Erwin Waterlander - 2019-06-13

status: open --> closed-fixed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Anonymous
  
  Add attachments
  Cancel
  You seem to have CSS turned off. Please don't fill out this field.
  
  You seem to have CSS turned off. Please don't fill out this field.

Anonymous

important issues found by covscan

Dos2Unix / Unix2Dos - Text file format converters

Group

Searches

Help

#18 important issues found by covscan

Discussion

Related