buglwIP - A Lightweight TCP/IP stack - Bugs: bug #21680, PPP upap_rauthnak() drops legal...

 
 

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

bug #21680: PPP upap_rauthnak() drops legal NAK packets

Submitted by:  Tom Evans <tom_evans>
Submitted on:  Fri 30 Nov 2007 12:27:44 AM UTC  
 
Category:  None Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Fri 04 Dec 2009 09:05:34 AM UTC, comment #5: 

I just saw there are files attached, sorry. Restructuring our PPP files for an easy diff to pppd is a great idea. I'll try to test them as soon as I can.

I'd like make a diff to the ucip sources, too to get an overview of how much the code has been changed while porting from ucip to lwIP.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 25 Nov 2009 09:23:29 AM UTC, comment #4: 

I compared the the current lwip ppp implementation
with ucip (which seems to be dead) and pppd 2.4.5
(which is just 2 weeks old).

The latest release (ucip1-0-3.zip) that can be
downloaded from http://sourceforge.net/projects/ucip/
is much different when compared with the sources I got
via cvs. The conclusion is that there is nothing
to port from ucip, but it was very helpful to
look at it because I understood better what Marc did.
Thanks, Simon.

There are some fixes in pppd 2.4.5 that I wish to
backport here. Unfortunately the function/struct/var/const
definitions in pppd appear in different order.
The order in lwip is better, but I reverted it because
the comparison is much harder.

The new files are attached. I'll be glad if someone
can test them. I may have mistaken some #if 0 or #endif.

PS: please take a look at
https://savannah.nongnu.org/patch/?6969 !

(file #19129, file #19130, file #19131, file #19132)

Iordan Neshev <iordan_neshev>
Thu 29 Oct 2009 08:57:03 AM UTC, comment #3: 

Thanks, Simon. I'll take a look soon.
I recently met a bug, causing a deadlock
when something goes wrong near the end of
the PPP link establishment.
I'll invsetigate further and report.

Iordan Neshev <iordan_neshev>
Mon 19 Oct 2009 04:32:58 PM UTC, comment #2: 

Just for the record: the ppp code seems to be copied from ucos-Net (later ucIP), which in turn is copied from one of the BSD OSes. Just if someone wants to have a look at that code to compare for bugfixes...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 01 May 2009 12:11:06 PM UTC, comment #1: 

OK, so at least now we know (we speculated about that before), that our PPP-implementation is ported from PPPD. Since it was ported over 6 years ago, I guess there might have been numerous bugfixes in the PPPD code since then. I guess comparing the lwIP code to the PPPD code might make sense to see if more bugfixes are available.

Anyway, I'm not using PPP, but comparing to the PPPD source, I think this fix is correct (I trust in PPPD being correct here:).

I've checked it in, also another if clause in that function that we are missing compared to PPPD 2.4.4.

Thanks for reporting.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 30 Nov 2007 12:27:44 AM UTC, original submission:  

The code in upap_rauthnak() is as follows:

static void upap_rauthnak(upap_state u, u_char inp,
                          int id, int len)
{
    ....
    /*
     * Parse message.
     */
    if (len < sizeof (u_char)) {
        UPAPDEBUG((LOG_INFO,
                  "pap_rauthnak: rcvd short packet.\n"));
        return;
    }
    ...
    u->us_clientstate = UPAPCS_BADAUTH;

   

    UPAPDEBUG((LOG_ERR, "PAP authentication failed\n"));
    auth_withpeer_fail(u->us_unit, PPP_PAP);
}

Unfortunately, at the above point "len" is the length of the PAP packet MINUS the four-byte NAK header. There's no requirement for the server to send anything more in this packet. I assume some servers do provide an explanatory message (no user, bad password etc), but the one we're working with doesn't.

So it drops a good packet and ignores it. It doesn't set the client state or call auth_withpeer_fail() and that means it doesn't record WHY the link dropped out in the error callbacks and status messages.

This isn't a showstopper as auth_withpeer_fail() then does:

     * We've failed to authenticate ourselves to our peer.
     * He'll probably take the link down, and there's not much
     * we can do except wait for that.

I guess most peers tear the link down. Ours doesn't (it has bugs as well, but they're not ours to fix).

The same function in the PPPD 2.4.4 code is as follows:

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

    if (u->us_clientstate != UPAPCS_AUTHREQ) /* XXX */
return;

    /*
     * Parse message.
     */
    if (len < 1) {
UPAPDEBUG(("pap_rauthnak: ignoring missing msg-length."));
    } else {
GETCHAR(msglen, inp);
if (msglen > 0) {
    len -= sizeof (u_char);
    if (len < msglen) {
UPAPDEBUG(("pap_rauthnak: rcvd short packet."));
return;
    }
    msg = (char *) inp;
    PRINTMSG(msg, msglen);
}
    }

    u->us_clientstate = UPAPCS_BADAUTH;

    error("PAP authentication failed");
    auth_withpeer_fail(u->us_unit, PPP_PAP);
}

BTW the current PPPD 2.4.4 code calls "lcp_close()" in its auth_withpeer_fail() code, as does mine now.

I don't have CVS access, I'm not up to date on the latest source and I've diverged from the CVS base code too far for me to be able to fix or change it.

Tom Evans <tom_evans>

 

Attached Files
file #19132:  pap.c added by iordan_neshev (16KiB - application/octet-stream - Reordered files which can be compared easier with the current pppd code)
file #19131:  lcp.c added by iordan_neshev (57KiB - application/octet-stream - Reordered files which can be compared easier with the current pppd code)
file #19130:  ipcp.c added by iordan_neshev (43KiB - application/octet-stream - Reordered files which can be compared easier with the current pppd code)
file #19129:  fsm.c added by iordan_neshev (23KiB - application/octet-stream - Reordered files which can be compared easier with the current pppd code)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

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

    Only logged-in users can vote.

     

     

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-11-25 iordan_neshev Attached File- => Added fsm.c, #19129
        Attached File- => Added ipcp.c, #19130
        Attached File- => Added lcp.c, #19131
        Attached File- => Added pap.c, #19132
    2009-05-01 goldsimon StatusNone => Fixed
        Assigned toNone => goldsimon
        Open/ClosedOpen => Closed
    2007-11-30 tom_evans Carbon-Copy- => Added tev@aapl.com.au

    Back to the top


    Powered by Savane 3.5