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()

Submitter:  Tom Evans <tom_evans>
Submitted:  Fri 10 Aug 2007 05:29:54 AM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned 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>
Group 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>
Group 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>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-08-27 marcbou StatusNone Fixed
        Open/ClosedOpen Closed
        Planned Release 1.3.0

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code