[lwip-devel] [bug #20743] Potential buffer leak in pppInput()

[Tom Evans]

URL:
  <http://savannah.nongnu.org/bugs/?20743>

                 Summary: Potential buffer leak in pppInput()
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: tom_evans
            Submitted on: Friday 08/10/2007 at 05:29
                Category: None
                Severity: 3 - Normal
              Item Group: None
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: 

    _______________________________________________________

Details:

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) {
       ...







    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?20743>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/