From owner-freebsd-net@FreeBSD.ORG Tue Dec 27 19:22:40 2011 Return-Path: Delivered-To: net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9B79C106564A for ; Tue, 27 Dec 2011 19:22:40 +0000 (UTC) (envelope-from glebius@freebsd.org) Received: from cell.glebius.int.ru (glebius.int.ru [81.19.64.117]) by mx1.freebsd.org (Postfix) with ESMTP id 341208FC1D for ; Tue, 27 Dec 2011 19:22:40 +0000 (UTC) Received: from cell.glebius.int.ru (localhost [127.0.0.1]) by cell.glebius.int.ru (8.14.5/8.14.5) with ESMTP id pBRJMdbO031285; Tue, 27 Dec 2011 23:22:39 +0400 (MSK) (envelope-from glebius@freebsd.org) Received: (from glebius@localhost) by cell.glebius.int.ru (8.14.5/8.14.5/Submit) id pBRJMcmr031284; Tue, 27 Dec 2011 23:22:38 +0400 (MSK) (envelope-from glebius@freebsd.org) X-Authentication-Warning: cell.glebius.int.ru: glebius set sender to glebius@freebsd.org using -f Date: Tue, 27 Dec 2011 23:22:38 +0400 From: Gleb Smirnoff To: Sami Halabi Message-ID: <20111227192238.GV8035@glebius.int.ru> References: <20111227044754.GK8035@FreeBSD.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="wq9mPyueHGvFACwf" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Cc: net@freebsd.org Subject: Re: ng_mppc_decompress: too many (4094) packets dropped, disabling node X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Dec 2011 19:22:40 -0000 --wq9mPyueHGvFACwf Content-Type: text/plain; charset=koi8-r Content-Disposition: inline On Tue, Dec 27, 2011 at 09:44:23AM +0200, Sami Halabi wrote: S> >1) Is the number always 4094? S> S> No, i see 4092, 4093 also: S> Dec 24 09:17:04 mpd2 kernel: ng_mppc_decompress: too many (4092) packets S> dropped S> , disabling node 0xffffff003051e400! S> Dec 24 09:17:04 mpd2 kernel: S> Dec 24 14:22:45 mpd2 kernel: ng_mppc_decompress: too many (4093) packets S> dropped S> , disabling node 0xffffff003d53db00! S> Dec 24 14:22:45 mpd2 kernel: S> Dec 24 19:28:45 mpd2 kernel: ng_mppc_decompress: too many (4092) packets S> dropped S> , disabling node 0xffffff00304e8500! Well, here is my histrogram of probability: 38 4094 5 4093 3 4095 1 4092 1 4091 1 4087 1 4083 1 3275 1 2173 1 2172 1 2171 1 2170 1 2169 1 2137 1 2135 1 2132 1 2122 1 2121 1 2120 1 1130 1 1013 I believe that problem is caused by re-ordering of packets that may happen on the Internet. We definitely didn't lose 4094 packets so often. Shift of 4095 means that we received a packet that should be the previous one. Shift of 4094 means that we received a packet, that should have been two packets ago. I have no idea why this condition is much more probable :( Today I thought of some patch that would detect and fix reordering, but failed to find any elegant way on fixing this MPPE poor design. So, I have decided to remove the protection at all. The decision is based on the following facts: 1) Our current limit of 1000 isn't by an order of magnitude greater than maximum possible rekeying number - 4095. So, the DoS protection is quite not really a noticable one. 2) Since ng_mppc was developed CPUs got faster by more than an order of magnitude. 3) Linux implementations do as much rekeying as needed. 4) It looks like Windows does too. Not very clear from the article, but out of ordering is mentioned here: http://technet.microsoft.com/en-us/library/cc958061.aspx I suggest the attached patch. Can you please test it for a period of time and report how it goes? I am going to try it, too. -- Totus tuus, Glebius. --wq9mPyueHGvFACwf Content-Type: text/x-diff; charset=koi8-r Content-Disposition: attachment; filename="ng_mppc.c.diff" Index: ng_mppc.c =================================================================== --- ng_mppc.c (revision 228916) +++ ng_mppc.c (working copy) @@ -98,15 +98,6 @@ /* Key length */ #define KEYLEN(b) (((b) & MPPE_128) ? 16 : 8) -/* - * When packets are lost with MPPE, we may have to re-key arbitrarily - * many times to 'catch up' to the new jumped-ahead sequence number. - * Since this can be expensive, we pose a limit on how many re-keyings - * we will do at one time to avoid a possible D.O.S. vulnerability. - * This should instead be a configurable parameter. - */ -#define MPPE_MAX_REKEY 1000 - /* MPPC packet header bits */ #define MPPC_FLAG_FLUSHED 0x8000 /* xmitter reset state */ #define MPPC_FLAG_RESTART 0x4000 /* compress history restart */ @@ -641,20 +632,13 @@ #endif #ifdef NETGRAPH_MPPC_ENCRYPTION if ((d->cfg.bits & MPPE_BITS) != 0) { - u_int rekey; - /* How many times are we going to have to re-key? */ - rekey = ((d->cfg.bits & MPPE_STATELESS) != 0) ? - numLost : (numLost / (MPPE_UPDATE_MASK + 1)); - if (rekey > MPPE_MAX_REKEY) { - log(LOG_ERR, "%s: too many (%d) packets" - " dropped, disabling node %p!", - __func__, numLost, node); - priv->recv.cfg.enable = 0; - goto failed; - } - - /* Re-key as necessary to catch up to peer */ + /* + * When packets are lost or re-ordered with MPPE, + * we may have to re-key up to 0xfff times to 'catch + * up' to the new jumped-ahead sequence number. Yep, + * this is heavy, but what else can we do? + */ while (d->cc != cc) { if ((d->cfg.bits & MPPE_STATELESS) != 0 || (d->cc & MPPE_UPDATE_MASK) --wq9mPyueHGvFACwf--