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

[Tom Evans]

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/