Bug 840920 - pmcd (and others) heap-based buffer overflow in __pmDecodeNameList
pmcd (and others) heap-based buffer overflow in __pmDecodeNameList
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: pcp (Show other bugs)
16
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Nathan Scott
Fedora Extras Quality Assurance
: Security
Depends On:
Blocks: 840765 CVE-2012-3418
  Show dependency treegraph
 
Reported: 2012-07-17 11:23 EDT by Florian Weimer
Modified: 2012-08-19 23:52 EDT (History)
5 users (show)

See Also:
Fixed In Version: pcp-3.6.5
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-08-19 23:52:33 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Proposed patch to resolve issues in pcp namelist pdu handling (4.44 KB, patch)
2012-07-21 00:06 EDT, Nathan Scott
no flags Details | Diff
Revised PCP namelist PDU decoding patch (4.92 KB, patch)
2012-07-23 21:37 EDT, Nathan Scott
no flags Details | Diff
Proof-of-concept patch avoiding structs for decoding (5.30 KB, patch)
2012-07-25 05:48 EDT, Florian Weimer
no flags Details | Diff
Updated version of Florian alternative approach (5.38 KB, patch)
2012-08-01 08:07 EDT, Nathan Scott
no flags Details | Diff

  None (edit)
Description Florian Weimer 2012-07-17 11:23:24 EDT
__pmDecodeNameList fetches the number of bytes to allocate for storing the incoming name strings from the PDU.  The function does not check if the strings provided later actually fit into the buffer:

    /* need space for name ptrs and the name characters */
    need = *numnames * ((int)sizeof(char*)) + ntohl(namelist_pdu->nstrbytes);
    if ((names = (char**)malloc(need)) == NULL)
	return -oserror();
    ...
	char	*dest = (char*)&names[*numnames];
        ...
	    memcpy(dest, np->name, namelen);
	    *(dest + namelen) = '\0';
	    dest += namelen + 1; 

This leads to a heap-based buffer overflow.  Exploitation is aided by the /rpoc information leak (bug 840765).

In addition, __pmDecodeNameList does not properly check the length of the status and names arrays against the PDU length, and does not guard against integer overflow when calculating the malloc argument.  This leads to another heap-based buffer overflow (probably on all architectures because the intermediate value is stored in an int).  This is similar to bug 840422.
Comment 5 Nathan Scott 2012-07-21 00:06:28 EDT
Created attachment 599485 [details]
Proposed patch to resolve issues in pcp namelist pdu handling

I believe this patch addresses each of the issues listed in the bug.  A code review would be much appreciated!

I've single-stepped through the handling of the test case provided (thanks!) with a debug build of pmcd, and that particular case is verified as fixed.
Comment 6 Mark Goodwin 2012-07-22 19:43:24 EDT
Nathan reqested assignment, thanks Nathan.
Comment 7 Florian Weimer 2012-07-23 06:14:22 EDT
(In reply to comment #5)
> Created attachment 599485 [details]
> Proposed patch to resolve issues in pcp namelist pdu handling
> 
> I believe this patch addresses each of the issues listed in the bug.  A code
> review would be much appreciated!
> 
> I've single-stepped through the handling of the test case provided (thanks!)
> with a debug build of pmcd, and that particular case is verified as fixed.

The patch no longer updates *numnamesp with zero if there are no names.  This could be a real bug.

    /* validity checks - none of these conditions should happen */
    if (numnames < 0 || numstatus < 0 || nstrbytes < 0)
	return PM_ERR_IPC;

You should check that nstrbytes is less than the PDU size before trying to allocate that much memory.  (This is an anti-DoS measure.)  No need to be very precise about this, the limit can be too large.  Same for numnames and numstatus.

	    if ((char *)np + sizeof(name_t) > pdu_end)  // (1)

The comparison should be instead:

	    if (sizeof(name_t) > (size_t)(pdu_end - (char *)np))

(char *)np + sizeof(name_t) is undefined if you run past the end of the buffer.  The size_t cast is necessary to silence a compiler warning: the RHS always positive, so it's a false positive.

	    namelen = ntohl(np->namelen);

namelen needs to be checked against 0, the packet is corrupt if it's negative.

	    if ((char *)np + sizeof(np->namelen) + namelen > pdu_end)

Same problem as (1) above.

	    if ((namelen + 1) > (dest_end - dest))

Technically, this needs two separate checks to prevent overflow of namelen + 1.  But the check directly above should cover that, so perhaps just add a comment.

Due to the code duplication, the comments above apply to both branches of the if statement.

 	    if (status != NULL)
 		status[i] = ntohl(np->status);

Can status be NULL at this point?  I don't think so.  In any case, this needs a check against numstatus, or numstatus needs to be compared to numnames at some point.

    if (status != NULL)
	free(status);

Nowadays, it's safe to pass NULL to free, so the check is redundant.
Comment 8 Nathan Scott 2012-07-23 21:35:37 EDT
(In reply to comment #7)
> (In reply to comment #5)
> ...

Thanks.  New patch shortly - couple of minor clarifications though,
on things the next patch doesn't address (if no comments, expect it
to be addressed):

>  	    if (status != NULL)
>  		status[i] = ntohl(np->status);
> 
> Can status be NULL at this point?  I don't think so. 

Yes, it will be NULL if the PDU contained status info but the caller
did not pass a pointer in to copy that to.  Next patch adds explicit
checking that numstatus in the PDU must be zero or equal to numnames
- nothing else makes sense - but the above check is needed, AIUI.

> Nowadays, it's safe to pass NULL to free, so the check is redundant.

I'm not sure thats true for all of the platforms PCP has been ported
to (in particular, Win32 and Solaris), and its a coding pattern used
consistently throughout, so I've left this.

Thanks again Florian.
Comment 9 Nathan Scott 2012-07-23 21:37:26 EDT
Created attachment 599886 [details]
Revised PCP namelist PDU decoding patch

Updated to include Florians review comments, except for the two discussed.
Comment 10 Florian Weimer 2012-07-25 05:46:04 EDT
(In reply to comment #9)
> Created attachment 599886 [details]
> Revised PCP namelist PDU decoding patch
> 
> Updated to include Florians review comments, except for the two discussed.

I think this

	    np = (name_t*)&namelist_pdu->names[j/sizeof(__pmPDU)];

can cause np to point past pdu_end because this check

	    if (sizeof(np->namelen) + namelen > (size_t)(pdu_end - (char *)np))

does not take into account the padding added here:

	    j += sizeof(namelen) + PM_PDU_SIZE_BYTES(namelen);

The net effect is that (size_t)(pdu_end - (char *)np) ends up being a large positive value, causing checks to succeed incorrectly.

I'm beginning to wonder if having those wire-format structs (name_t, name_status_t, namelist_t) is beneficial.
Comment 11 Florian Weimer 2012-07-25 05:48:08 EDT
Created attachment 600251 [details]
Proof-of-concept patch avoiding structs for decoding

This is just a proof of concept, a real patch would have to get rid of the nested functions.
Comment 12 Nathan Scott 2012-08-01 08:07:35 EDT
Created attachment 601724 [details]
Updated version of Florian alternative approach

This patch fixes the first few issues I came across trying out that patch Florian.  I'm not sure about this approach - the structures do give an inherent ordering to fields which is easy to get wrong, and if we remove the here this will be the only PDU which doesn't decode using structures (bit funky for maintenance or future extension, perhaps).  Its also quite invasive, and I'm not overly comfortable about the risk level esp. given all the other changes we're making.

The attached patch also still doesn't quite work yet, some quick checks show several command commands quickly hitting PM_ERR_IPC where they have valid PDU contents.

Getting late here, I think tomorrow I might revisit the earlier patch and make that index use (j/sizeof(__pmPDU) approach) more robust if possible.

cheers.
Comment 13 Huzaifa S. Sidhpurwala 2012-08-15 23:44:44 EDT
Upstream patch:

http://oss.sgi.com/cgi-bin/gitweb.cgi?p=pcp/pcp.git;a=commit;h=f0eaefe046b1061797f45b0c20bb2ac371b504a5

This issue has been addressed in pcp-3.6.5

Note You need to log in before you can comment on or make changes to this bug.