[bug #21680] PPP upap_rauthnak() drops legal NAK packets

[Arthur Marsh-2]

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

                 Summary: PPP upap_rauthnak() drops legal NAK packets
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: tom_evans
            Submitted on: Friday 11/30/2007 at 00:27
                Category: None
                Severity: 3 - Normal
              Item Group: Faulty Behaviour
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release:

    _______________________________________________________

Details:

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.





    _______________________________________________________

Reply to this item at:

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

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



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Arthur Marsh-2]

Update of bug #21680 (project lwip):

                  Status:                    None => Fixed                  
             Assigned to:                    None => goldsimon              
             Open/Closed:                    Open => Closed                
            lwIP version:                    None => CVS Head              

    _______________________________________________________

Follow-up 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.

    _______________________________________________________

Reply to this item at:

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

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Arthur Marsh-2]

Follow-up Comment #2, bug #21680 (project lwip):

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

    _______________________________________________________

Reply to this item at:

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

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Arthur Marsh-2]

Follow-up Comment #3, bug #21680 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

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

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



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel

[Arthur Marsh-2]

Follow-up Comment #4, bug #21680 (project lwip):

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

Additional Item Attachment:

File name: fsm.c                          Size:23 KB
File name: ipcp.c                         Size:43 KB
File name: lcp.c                          Size:57 KB
File name: pap.c                          Size:15 KB


    _______________________________________________________

Reply to this item at:

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

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



_______________________________________________
lwip-devel mailing list
lwip-devel@...
http://lists.nongnu.org/mailman/listinfo/lwip-devel