Bug 717643

Here is code in question:

  static char* OPT_parseOptFile(char *filename, char*validlist[])
  {
    FILE *fp;
    char buffer[1024];

    if (filename == NULL || filename[0] == '\0') {
       return ("Bad syntax for 'optfile'\n");
    }
    fp = fopen(filename,"r");              <---- line 79
    if (fp == NULL) {
       return ("Options file could not be opened for reading\n");
    }
    while (fgets(buffer,1024,fp)) {
       if (buffer[strlen(buffer)-1] == '\n') buffer[strlen(buffer)-1] = '\0';
       if (buffer[strlen(buffer)-1] == '\r') buffer[strlen(buffer)-1] = '\0';

       OPT_parseArgument(strdup(buffer),validlist);
    }
    fclose(fp);
    return NULL;
  }


1. NULL pointer returned from fopen is handled.
2. There is no existing file handle value that can be de-referenced by fopen.

This code is safe and I would like to qualify this case reported by Coverity as
  Classification: Intentional
          Action: Ignore

Created attachment 517499 [details]
proposed patch

Comment on attachment 517499 [details]
proposed patch

You are still going to leak cinfo on fopen() failure I am afraid.  But at least the crash should be fixed by this patch...

Comment on attachment 517499 [details]
proposed patch

>Index: pki/base/native-tools/src/p7tool/p7tool.c
>===================================================================
>--- pki/base/native-tools/src/p7tool/p7tool.c	(revision 2128)
>+++ pki/base/native-tools/src/p7tool/p7tool.c	(working copy)
>@@ -263,6 +263,10 @@
>             sprintf(filename, "%s%d.der", prefix, i);
> 
>             outFile = fopen(filename, "wb");
>+            if (outFile == NULL) {
>+                fprintf(out, "Couldn't open '%s' file\n", filename);

Did you want to call SEC_PKCS7DestroyContentInfo(cinfo) at this point?

>+                return -1;
>+            }
>             nb = fwrite((char *) cert, 1, items[i]->len, outFile);
>             fclose(outFile);
>

I would also suggest to say "Couldn't open '%s' file for writing\n" in the error message to make the patch more user-friendly.

Created attachment 517912 [details]
updated patch

Created attachment 518092 [details]
patch for OVERRUN_STATIC CID 106455 (base/native-tools/src/setpin/setpin.c:254-255)

Created attachment 518125 [details]
patch for RESOURCE_LEAK CID 106458 (base/native-tools/src/setpin/setpin.c:1179,1214)

Created attachment 518565 [details]
patch for all high priority issues

patch for all high priority issues listed by https://dell-per610-03.lab.eng.brq.redhat.com/coverity/rhel-6.1-20110527/pki-core-9.0.3-10.el6/run1/pki-core-9.0.3-10.el6.html

includes CIDs: 106454-106459,106465,106466,60812,60813,60844
+ 3 issues without CIDs that are similar to 60812,60813,60844.

Comment on attachment 518565 [details]
patch for all high priority issues

One caveat. Check this out first before finishing:
Block 275-285

 loser: 	

  errcode=14;	
  exitError(errbuf);	
  free(errbuf);

May be freeing something after the program is gone..

IPA_v2_RHEL_6_ERRATA_BRANCH:

svn commit pki
Sending        pki/base/native-tools/src/p7tool/p7tool.c
Sending        pki/base/native-tools/src/p7tool/secutil.c
Sending        pki/base/native-tools/src/setpin/setpin.c
Sending        pki/base/native-tools/src/setpin/setpin_options.c
Sending        pki/base/native-tools/src/setpin/setpin_options.h
Sending        pki/base/native-tools/src/tkstool/key.c
Sending        pki/base/native-tools/src/tkstool/random.c
Sending        pki/base/native-tools/src/tkstool/secutil.c
Transmitting file data ........
Committed revision 2151.

trunk:

svn commit pki
Sending        pki/base/native-tools/src/p7tool/p7tool.c
Sending        pki/base/native-tools/src/p7tool/secutil.c
Sending        pki/base/native-tools/src/setpin/setpin.c
Sending        pki/base/native-tools/src/setpin/setpin_options.c
Sending        pki/base/native-tools/src/setpin/setpin_options.h
Sending        pki/base/native-tools/src/tkstool/key.c
Sending        pki/base/native-tools/src/tkstool/random.c
Sending        pki/base/native-tools/src/tkstool/secutil.c
Transmitting file data ........
Committed revision 2152.

IPA_v2_RHEL_6_ERRATA_BRANCH:

svn commit pki/base/native-tools/src/setpin/setpin_options.c
Sending        pki/base/native-tools/src/setpin/setpin_options.c
Transmitting file data .
Committed revision 2153.

trunk:

svn commit pki/base/native-tools/src/setpin/setpin_options.c
Sending        pki/base/native-tools/src/setpin/setpin_options.c
Transmitting file data .
Committed revision 2154.

Issue in pki-core-9.0.3/base/native-tools/src/p7tool/p7tool.c:265 was fixed. That was verified by the scan of current version of package.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2011-1655.html