RHEL Engineering is moving the tracking of its product development work on RHEL 6 through RHEL 9 to Red Hat Jira (issues.redhat.com). If you're a Red Hat customer, please continue to file support cases via the Red Hat customer portal. If you're not, please head to the "RHEL project" in Red Hat Jira and file new tickets here. Individual Bugzilla bugs in the statuses "NEW", "ASSIGNED", and "POST" are being migrated throughout September 2023. Bugs of Red Hat partners with an assigned Engineering Partner Manager (EPM) are migrated in late September as per pre-agreed dates. Bugs against components "kernel", "kernel-rt", and "kpatch" are only migrated if still in "NEW" or "ASSIGNED". If you cannot log in to RH Jira, please consult article #7032570. That failing, please send an e-mail to the RH Jira admins at rh-issues@redhat.com to troubleshoot your issue as a user management inquiry. The email creates a ServiceNow ticket with Red Hat. Individual Bugzilla bugs that are migrated will be moved to status "CLOSED", resolution "MIGRATED", and set with "MigratedToJIRA" in "Keywords". The link to the successor Jira issue will be found under "Links", have a little "two-footprint" icon next to it, and direct you to the "RHEL project" in Red Hat Jira (issue links are of type "https://issues.redhat.com/browse/RHEL-XXXX", where "X" is a digit). This same link will be available in a blue banner at the top of the page informing you that that bug has been migrated.
Bug 717643 - Fopen without NULL check and other Coverity issues
Summary: Fopen without NULL check and other Coverity issues
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: pki-core
Version: 6.1
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: rc
: ---
Assignee: Andrew Wnuk
QA Contact: Chandrasekar Kannan
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-06-29 13:16 UTC by Michal Luscon
Modified: 2015-01-04 23:49 UTC (History)
5 users (show)

Fixed In Version: pki-core-9.0.3-16.el6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-12-06 16:29:11 UTC
Target Upstream Version:
Embargoed:

Attachments (Terms of Use)
proposed patch (613 bytes, patch)
2011-08-09 22:16 UTC, Andrew Wnuk 		mharmsen: review+
Details | Diff
updated patch (646 bytes, patch)
2011-08-12 00:00 UTC, Andrew Wnuk 		mharmsen: review+
Details | Diff
patch for OVERRUN_STATIC CID 106455 (base/native-tools/src/setpin/setpin.c:254-255) (1.48 KB, patch)
2011-08-12 18:51 UTC, Andrew Wnuk 		mharmsen: review+
Details | Diff
patch for RESOURCE_LEAK CID 106458 (base/native-tools/src/setpin/setpin.c:1179,1214) (2.45 KB, patch)
2011-08-12 23:44 UTC, Andrew Wnuk 		mharmsen: review+
Details | Diff
patch for all high priority issues (11.49 KB, patch)
2011-08-16 21:15 UTC, Andrew Wnuk 		jmagne: review+
Details | Diff
patch correcting coverity issue CID 10959 (1.46 KB, patch)
2011-08-18 19:16 UTC, Andrew Wnuk 		jmagne: review+
Details | Diff
Show Obsolete (4) View All


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2011:1655 0 normal SHIPPED_LIVE pki-core bug fix and enhancement update 2011-12-06 00:50:24 UTC

Description Michal Luscon 2011-06-29 13:16:15 UTC 
Description of problem:

pki-core-9.0.3/base/native-tools/src/setpin/options.c:79 - If function fopen returns NULL, NULL pointer will be dereferenced in function fwrite.

Version-Release number of selected component (if applicable):
9.0.3-10

Additional info:

This defect was probably introduced by Red Hat patches.
Comment 2 Andrew Wnuk 2011-08-08 20:38:57 UTC 
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
Comment 4 Andrew Wnuk 2011-08-09 22:16:17 UTC 
Created attachment 517499 [details]
proposed patch
Comment 5 Kamil Dudka 2011-08-09 22:32:54 UTC 
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 6 Kamil Dudka 2011-08-10 01:53:11 UTC 
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);
>
Comment 7 Kamil Dudka 2011-08-10 01:58:50 UTC 
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.
Comment 8 Andrew Wnuk 2011-08-12 00:00:01 UTC 
Created attachment 517912 [details]
updated patch
Comment 9 Andrew Wnuk 2011-08-12 18:51:43 UTC 
Created attachment 518092 [details]
patch for OVERRUN_STATIC CID 106455 (base/native-tools/src/setpin/setpin.c:254-255)
Comment 10 Andrew Wnuk 2011-08-12 23:44:31 UTC 
Created attachment 518125 [details]
patch for RESOURCE_LEAK CID 106458 (base/native-tools/src/setpin/setpin.c:1179,1214)
Comment 11 Andrew Wnuk 2011-08-16 21:15:18 UTC 
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 13 Jack Magne 2011-08-16 22:43:46 UTC 
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..
Comment 14 Andrew Wnuk 2011-08-17 19:16:01 UTC 
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.
Comment 15 Andrew Wnuk 2011-08-17 19:19:31 UTC 
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.
Comment 17 Andrew Wnuk 2011-08-18 22:05:46 UTC 
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.
Comment 18 Andrew Wnuk 2011-08-18 22:10:12 UTC 
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.
Comment 22 Michal Luscon 2011-09-07 13:06:28 UTC 
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.
Comment 25 errata-xmlrpc 2011-12-06 16:29:11 UTC 
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
Note You need to log in before you can comment on or make changes to this bug.