patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6969, PPP: missing PAP authentication...

 
 

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

patch #6969: PPP: missing PAP authentication UNTIMEOUT

Submitter:  Iordan Neshev <iordan_neshev>
Submitted:  Fri 06 Nov 2009 10:17:40 AM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Sun 06 Dec 2009 11:24:32 AM UTC, comment #5: 

Fixed. However, bug #13315 is still open.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 04 Dec 2009 04:52:05 PM UTC, comment #4: 


>However, bug #13315 is not exatcly the same: it is about NACK,
>where this patch is about ACK. However, it could also be fixed
>by calling UNTIMEOUT there.

Yes, but one of its solutions is similar. We should continue the work on it in its thread.

>I wonder if that bug exists in pppd, also...

I've just sent an email about this to Paul. I'll let you know of his answer.

Iordan Neshev <iordan_neshev>
Fri 04 Dec 2009 03:39:00 PM UTC, comment #3: 

OK, so I agree that the problem exists (just debugged it with PPPoE) and that the UNTIMEOUT() fixes it. I'll check it in.

However, bug #13315 is not exatcly the same: it is about NACK, where this patch is about ACK. However, it could also be fixed by calling UNTIMEOUT there. I wonder if that bug exists in pppd, also...

Simon Goldschmidt <goldsimon>
Group administrator
Fri 04 Dec 2009 09:03:43 AM UTC, comment #2: 

Funny that using lwIP PPP leads to finding bugs in the original sources ;-)

You mentioned a dead loop here, is that really solved by adding the UNTIMEOUT()?

I guess when a PPP developer gives his OK for this, it should be pretty safe to check in... I'll try to debug it today.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 25 Nov 2009 09:34:29 AM UTC, comment #1: 

Yesterday I sent a mail to one of the pppd developers
(paulus@samba.org) about this patch. This is his response:

"Your proposed fix looks reasonable, although I would probably put the UNTIMEOUT() just before the assignment to
u->us_clientstate."

This is not submitted yet to the pppd bugtracker.
I think this is the solution of:

http://ppp.samba.org/cgi-bin/ppp-bugs/incoming?id=1163;user=guest
https://savannah.nongnu.org/bugs/?13315

As Paul suggested, I propose to add
UNTIMEOUT(upap_timeout, u); /* Cancel timeout */
just before u->us_clientstate = UPAPCS_OPEN;

I think this should go into lwip 1.3.2. Any objections?


Iordan Neshev <iordan_neshev>
Fri 06 Nov 2009 10:17:40 AM UTC, original submission:  

static void
upap_rauthack(upap_state *u, u_char *inp, int id, int len)
{
  u_char msglen;
  char *msg;

  LWIP_UNUSED_ARG(id);

  UPAPDEBUG((LOG_INFO, "pap_rauthack: Rcvd id %d s=%d\n", id, u->us_clientstate));

  if (u->us_clientstate != UPAPCS_AUTHREQ) { /* XXX */
    UPAPDEBUG((LOG_INFO, "pap_rauthack: us_clientstate != UPAPCS_AUTHREQ; returning\n"));
    return;
  }

  /*
   * Parse message.
   */
  if (len < (int)sizeof (u_char)) {
    UPAPDEBUG((LOG_INFO, "pap_rauthack: rcvd short packet.\n"));
    return;
  }
  GETCHAR(msglen, inp);
  len -= sizeof (u_char);
  if (len < msglen) {
    UPAPDEBUG((LOG_INFO, "pap_rauthack: rcvd short packet.\n"));
    return;
  }
// THIS IS MISSING --->
#if 1
  UNTIMEOUT(upap_timeout, u);    /* Cancel timeout */
#endif
// <----

  msg = (char *) inp;
  PRINTMSG(msg, msglen);

  u->us_clientstate = UPAPCS_OPEN;

  auth_withpeer_success(u->us_unit, PPP_PAP); // follows NETWORK phase
}

When the LCP layer is up, lwip sends PAP authenticate request using upap_sauthreq(). It sends the packet and sets a TIMEOUT(upap_timeout) which is supposed to resend the request if we do not receive ACK in a reasonable time.

upap_rauthack() is called when the peer acknowledges our PAP authenticate request. It starts the network phase of the PPP phase diagram. Before that the PAP timeout should be canceled (but it's not). As a result I see it a little bit after the PPP session is completely ready.

It's harmless, because this if() prevents sending
another auth request.

if (u->us_clientstate != UPAPCS_AUTHREQ) {
  return;
}

Anyway, I think it should be avoided. This bug is missing in ChapReceiveSuccess() (this does the same as upap_rauthack when tha auth protocol is CHAP).
 


Iordan Neshev <iordan_neshev>

 

(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 goldsimon (Posted a comment)
  • -email is unavailable- added by iordan_neshev (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
    2009-12-06 goldsimon StatusNone Done
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code