Red Hat Bugzilla – Bug 840920
pmcd (and others) heap-based buffer overflow in __pmDecodeNameList
Last modified: 2012-08-19 23:52:33 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.
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.
Nathan reqested assignment, thanks Nathan.
(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.
(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.
Created attachment 599886 [details] Revised PCP namelist PDU decoding patch Updated to include Florians review comments, except for the two discussed.
(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.
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.
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.
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
This issue was addressed in Fedora and EPEL via the following security updates: Fedora-16: https://admin.fedoraproject.org/updates/pcp-3.6.5-1.fc16 Fedora-17: https://admin.fedoraproject.org/updates/pcp-3.6.5-1.fc17 Rawhide: https://admin.fedoraproject.org/updates/pcp-3.6.5-1.fc18 EPEL-5: https://admin.fedoraproject.org/updates/pcp-3.6.5-1.el5 EPEL-6: https://admin.fedoraproject.org/updates/pcp-3.6.5-1.el6