buglwIP - A Lightweight TCP/IP stack - Bugs: bug #20743, Potential buffer leak in pppInput()

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #20743: Potential buffer leak in pppInput()

Submitted by:  Tom Evans <tom_evans>
Submitted on:  Fri 10 Aug 2007 05:29:54 AM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: NoneStatus: Fixed
Privacy: PublicAssigned to: None
Open/Closed: ClosedPlanned Release: None
lwIP version: None

Mon 27 Aug 2007 09:41:57 AM UTC, comment #2:

Marc, you have closed this bug record, is it because you have already fix it in your changes for "Added PPPoE support and various PPP improvements." ?

Frédéric Bernon <fbernon>
Project Member
Fri 10 Aug 2007 10:43:49 AM UTC, comment #1:

Thanks for looking around ppp, it clearly needs some attention!

Unfortunately (and as you probably know), we currently don't have active developers for PPP, so this bug might be open for a while. Unless you'd become an active developer ;-)

Nevertheless, it's worth to document these things for the futurue, even if we can't solve them right now.

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 10 Aug 2007 05:29:54 AM UTC, original submission:

I'm looking at the "head" revision of ppp.c (probably Revision 1.13) in the CVS repository.

I'm just reviewing the code (comparing the current head revision with the 1.2.0 sources I'm working from) and not finding problems from running the code (yet).

I think I've found some pbuf leaks.

pppInput is called from pppInProc() as follows:

/* Dispatch the packet thereby consuming it. */
if(tcpip_callback(pppInput, pc->inHead) != ERR_OK) {

That clearly states that pppInput() has to consume or dispose of the buffer.

The very first test in pppInput() is:

if(pbuf_header(nb, -(int)sizeof(struct pppInputHeader))) {
/* Can we cope with this failing? Just assert for now */
LWIP_ASSERT("pbuf_header failed\n", 0);
return;
}

That's a new addition in Revision 1.11. I'm pretty sure the above should NOT "return", but should "goto drop" like most of the others do, as the "drop:" and "out:" flags lead to code that disposes of the buffer.

The second test (of lcp_phase[] and other things) performs "goto drop".

The third one:

if (vj_uncompress_tcp(&nb, &pppControl[pd].vjComp) >= 0) {
if (pppControl[pd].netif.input != NULL) {
pppControl[pd].netif.input(nb, &pppControl[pd].netif);
}
return;
}

also looks like it could leak a pbuf and should probably be:

if (vj_uncompress_tcp(&nb, &pppControl[pd].vjComp) >= 0) {
if (pppControl[pd].netif.input != NULL) {
pppControl[pd].netif.input(nb, &pppControl[pd].netif);
return;
} else {
goto drop;
}
}

The fourth one is the same as the third one, but calls vj_uncompress_uncomp() instead; same fix applies.

The fifth one looks to have the same bug:

case PPP_IP: /* Internet Protocol */
PPPDEBUG((LOG_INFO, "pppInput[%d]: ip in pbuf
len=%d\n", pd, nb->len));
if (pppControl[pd].netif.input != NULL) {
pppControl[pd].netif.input(nb, &pppControl[pd].netif);
}
return;

The last four lines should be:

if (pppControl[pd].netif.input != NULL) {
pppControl[pd].netif.input(nb, &pppControl[pd].netif);
return;
} else {
goto drop;
}

That's the end of the pbuf leaks that I can see on a code inspection.

While we're here:

/*
* Upcall the proper protocol input routine.
*/
for (i = 0; (protp = ppp_protocols[i]) != NULL; ++i) {
if (protp->protocol == protocol && protp->enabled_flag) {
...

would be a little more robust if it included a check on the function pointer as follows before it uses the pointer:

if (protp->protocol == protocol && protp->enabled_flag &&
protp->input != NULL) {
...

Tom Evans <tom_evans>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by fbernon (Posted a comment)
  • -unavailable- added by marcbou (Updated the item)
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by tom_evans (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 27 Aug 2007 12:23:00 AM UTCmarcbouStatusNone=>Fixed
      Open/ClosedOpen=>Closed
      Planned Release=>1.3.0

    Back to the top


    Powered by Savane 3.1-cleanup1