Bug 840920

Summary: pmcd (and others) heap-based buffer overflow in __pmDecodeNameList
Product: [Fedora] Fedora Reporter: Florian Weimer <fweimer>
Component: pcpAssignee: Nathan Scott <nathans>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 16CC: fche, kenj, mgoodwin, nathans, security-response-team
Target Milestone: ---Keywords: Security
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
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: ---
Bug Depends On:    
Bug Blocks: 840765, 841698    
Attachments:
Description Flags
Proposed patch to resolve issues in pcp namelist pdu handling
none
Revised PCP namelist PDU decoding patch
none
Proof-of-concept patch avoiding structs for decoding
none
Updated version of Florian alternative approach none

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