Attachment 600251 Details for Bug 840920 – Proof-of-concept patch avoiding structs for decoding

[patch] Proof-of-concept patch avoiding structs for decoding

no-struct.patch (text/plain), 5.30 KB, created by Florian Weimer on 2012-07-25 09:48:08 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Florian Weimer

Created: 2012-07-25 09:48:08 UTC

Size: 5.30 KB

patch

obsolete

>diff --git a/src/libpcp/src/p_pmns.c b/src/libpcp/src/p_pmns.c
>index a2908a0..a5c291e 100644
>--- a/src/libpcp/src/p_pmns.c
>+++ b/src/libpcp/src/p_pmns.c
>@@ -284,89 +284,119 @@ __pmSendNameList(int fd, int from, int numnames, char *namelist[],
>  * statuslist is optional ... if NULL, no status values will be returned
>  */
> int
>-__pmDecodeNameList(__pmPDU *pdubuf, int *numnames, 
>+__pmDecodeNameList(__pmPDU *pdubuf, int *numnamesp,
>                   char*** namelist, int** statuslist)
> {
>-    namelist_t	*namelist_pdu;
>+    char	*pdu_current;
>+    char 	*pdu_end;
>+    int		pdu_len;
>     char        **names;
>+    char	*dest, *dest_end;
>     int		*status = NULL;
>-    int 	need;
>-    int		numstatus;
>+    int 	namesize, numnames;
>+    int 	statussize, numstatus;
>+    int 	nstrbytes;
>+    int		namelen;
>+    int		i;
> 
>-    namelist_pdu = (namelist_t *)pdubuf;
>+    int get_int(int *result)
>+    {
>+	if (sizeof(int) < (size_t)(pdu_end - pdu_current))
>+	    return 0;
>+	memcpy(result, pdu_current, sizeof(int));
>+	*result = ntohl(*result);
>+	pdu_current += sizeof(int);
>+	return 1;
>+    }
>+
>+    int realign_int()
>+    {
>+	size_t pos = pdu_current - (char *)pdubuf;
>+	size_t needed = (4 - (pos % 4)) % 4; /* 0->0, 1->3, 2->2, 3->1 */
>+	if (needed < (size_t)(pdu_end - pdu_current))
>+	    return 0;
>+	pdu_current += needed;
>+	return 1;
>+    }
>+    
>+    pdu_current = (char *)pdubuf;
>+    pdu_len = pdubuf[0];
>+    pdu_end = pdu_current + pdu_len;
>+    pdu_current += sizeof(__pmPDUHdr);
> 
>     *namelist = NULL;
>     if (statuslist != NULL)
> 	*statuslist = NULL;
> 
>-    *numnames = ntohl(namelist_pdu->numnames);
>-    numstatus = ntohl(namelist_pdu->numstatus);
>+    if (!(get_int(&numnames) && get_int(&numstatus) && get_int(&nstrbytes)))
>+	return PM_ERR_IPC;
> 
>-    if (*numnames == 0)
>+    if (numnames == 0) {
>+	*numnamesp = 0;
> 	return 0;
>+    }
> 
>-    if (*numnames < 0 || numstatus < 0)
>-	/* should not happen */
>+    /* validity checks - none of these conditions should happen */
>+    if (numnames < 0 || nstrbytes < 0)
>+	return PM_ERR_IPC;
>+    /* anti-DOS measure - limiting allowable memory allocations */
>+    if (numnames > pdu_len || nstrbytes > pdu_len)
>+	return PM_ERR_IPC;
>+    /* numstatus must be one (and only one) of zero or numnames */
>+    if (numstatus != 0 && numstatus != numnames)
> 	return PM_ERR_IPC;
> 
>     /* need space for name ptrs and the name characters */
>-    need = *numnames * ((int)sizeof(char*)) + ntohl(namelist_pdu->nstrbytes);
>-    if ((names = (char**)malloc(need)) == NULL)
>+    if (numnames >= (INT_MAX - nstrbytes) / (int)sizeof(char *))
>+	return PM_ERR_IPC;
>+    namesize = numnames * ((int)sizeof(char*)) + nstrbytes;
>+    if ((names = (char**)malloc(namesize)) == NULL)
> 	return -oserror();
> 
>     /* need space for status values */
>     if (statuslist != NULL && numstatus > 0) {
>-	need = numstatus * (int)sizeof(int);
>-	if ((status = (int*)malloc(need)) == NULL) {
>+	if (numstatus >= INT_MAX / (int)sizeof(int))
>+	    goto corrupt;
>+	statussize = numstatus * (int)sizeof(int);
>+	if ((status = (int*)malloc(statussize)) == NULL) {
> 	    free(names);
> 	    return -oserror();
> 	}
>     }
> 
>-    /* copy over ptrs and characters */
>-    if (numstatus == 0) {
>-	int	i, j = 0;
>-	char	*dest = (char*)&names[*numnames];
>-	name_t	*np;
>-	int	namelen;
>-
>-        for (i = 0; i < *numnames; i++) {
>-	    np = (name_t*)&namelist_pdu->names[j/sizeof(__pmPDU)];
>-	    names[i] = dest;
>-	    namelen = ntohl(np->namelen);
>-
>-	    memcpy(dest, np->name, namelen);
>-	    *(dest + namelen) = '\0';
>-	    dest += namelen + 1; 
>+    dest = (char*)&names[numnames];
>+    dest_end = (char*)names + namesize;
> 
>-	    j += sizeof(namelen) + PM_PDU_SIZE_BYTES(namelen);
>-	}
>-    }
>-    else { /* status fields included in the PDU */
>-	int		i, j = 0;
>-        char		*dest = (char*)&names[*numnames];
>-	name_status_t	*np;
>-	int		namelen;
>-
>-	for (i = 0; i < *numnames; i++) {
>-	    np = (name_status_t*)&namelist_pdu->names[j/sizeof(__pmPDU)];
>-	    names[i] = dest;
>-	    namelen = ntohl(np->namelen);
>+    for (i = 0; i < numnames; i++) {
>+	names[i] = dest;
>+	if (numstatus != 0) {
>+	    int st;
>+	    if (!get_int(&st))
>+		goto corrupt;
> 	    if (status != NULL)
>-		status[i] = ntohl(np->status);
>-
>-	    memcpy(dest, np->name, namelen);
>-	    *(dest + namelen) = '\0';
>-	    dest += namelen + 1; 
>-
>-	    j += sizeof(np->status) + sizeof(namelen) + PM_PDU_SIZE_BYTES(namelen);
>+		status[i] = st;
> 	}
>+	if (!get_int(&namelen))
>+	    goto corrupt;
>+	if (namelen < 0 || namelen > (pdu_end - pdu_current)
>+	    || namelen > (dest_end - dest))
>+	    goto corrupt;
>+	memcpy(dest, pdu_current, namelen);
>+	pdu_current += namelen;
>+	dest += namelen;
>+	if (pdu_current == pdu_end || dest == dest_end)
>+	    goto corrupt;
>+	++pdu_current;
>+	*dest = '\0';
>+	++dest;
>+	if (!realign_int())
>+	    goto corrupt;
>     }
> 
> #ifdef PCP_DEBUG
>     if (pmDebug & DBG_TRACE_PMNS) {
> 	fprintf(stderr, "__pmDecodeNameList\n");
>-	__pmDumpNameList(stderr, *numnames, names);
>+	__pmDumpNameList(stderr, numnames, names);
> 	if (status != NULL)
> 	    __pmDumpStatusList(stderr, numstatus, status);
>     }
>@@ -375,7 +405,14 @@ __pmDecodeNameList(__pmPDU *pdubuf, int *numnames,
>     *namelist = names;
>     if (statuslist != NULL)
> 	*statuslist = status;
>-    return *numnames;
>+    *numnamesp = numnames;
>+    return numnames;
>+
>+corrupt:
>+    if (status != NULL)
>+	free(status);
>+    free(names);
>+    return PM_ERR_IPC;
> }
> 
>

Actions: View | Diff

Attachments on bug 840920: 599485 | 599886 | 600251 | 601724