[PATCH 0/2] ppp: mppe: fixes MPPE desync on links which don't guarantee packet ordering

[PATCH 0/2] ppp: mppe: fixes MPPE desync on links which don't guarantee packet ordering

[Sylvain Rochet]

I am currently having an issue with PPP over L2TP (UDP) and MPPE in
stateless mode (default mode), UDP does not guarantee packet ordering so
we might get out of order packet. MPPE needs to be continuously synched
so we should drop late UDP packet.

I added a printk on the number of time we rekeyed in MPPE decompressor,
this is what we currently have if we receive a slightly out of order UDP
packet:

[1731001.049206] mppe_decompress[1]: ccount 1559
[1731001.049216] mppe_decompress[1]: rekeyed 1 times

[1731001.049228] mppe_decompress[1]: ccount 1560
[1731001.049232] mppe_decompress[1]: rekeyed 1 times

[1731001.050170] mppe_decompress[1]: ccount 1562
[1731001.050182] mppe_decompress[1]: rekeyed 2 times

[1731001.050191] mppe_decompress[1]: ccount 1561
[1731001.062576] mppe_decompress[1]: rekeyed 4095 times
                                             ^^^^
This is obviously wrong, we missed packet 1561 and we already rekeyed 2
times for 1562 we previously received, we can't recover the decryption
key we need for 1561, we should drop it instead of rekeying 4095 times.

This patch series drop any packet with are not within the 4096/2 forward
window.

Sylvain Rochet (2):
  ppp: mppe: sanity error path rework
  ppp: mppe: discard late packet in stateless mode

 drivers/net/ppp/ppp_mppe.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

-- 
2.1.4

[PATCH 1/2] ppp: mppe: sanity error path rework

[Sylvain Rochet]

We are going to need sanity error path a little further, rework to be
able to use the sanity error path anywhere in decompressor.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
---
 drivers/net/ppp/ppp_mppe.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 911b216..692ee0f 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -478,7 +478,6 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
 	struct blkcipher_desc desc = { .tfm = state->arc4 };
 	unsigned ccount;
 	int flushed = MPPE_BITS(ibuf) & MPPE_BIT_FLUSHED;
-	int sanity = 0;
 	struct scatterlist sg_in[1], sg_out[1];
 
 	if (isize <= PPP_HDRLEN + MPPE_OVHD) {
@@ -514,31 +513,19 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
 		       "mppe_decompress[%d]: ENCRYPTED bit not set!\n",
 		       state->unit);
 		state->sanity_errors += 100;
-		sanity = 1;
+		goto sanity_error;
 	}
 	if (!state->stateful && !flushed) {
 		printk(KERN_DEBUG "mppe_decompress[%d]: FLUSHED bit not set in "
 		       "stateless mode!\n", state->unit);
 		state->sanity_errors += 100;
-		sanity = 1;
+		goto sanity_error;
 	}
 	if (state->stateful && ((ccount & 0xff) == 0xff) && !flushed) {
 		printk(KERN_DEBUG "mppe_decompress[%d]: FLUSHED bit not set on "
 		       "flag packet!\n", state->unit);
 		state->sanity_errors += 100;
-		sanity = 1;
-	}
-
-	if (sanity) {
-		if (state->sanity_errors < SANITY_MAX)
-			return DECOMP_ERROR;
-		else
-			/*
-			 * Take LCP down if the peer is sending too many bogons.
-			 * We don't want to do this for a single or just a few
-			 * instances since it could just be due to packet corruption.
-			 */
-			return DECOMP_FATALERROR;
+		goto sanity_error;
 	}
 
 	/*
@@ -649,6 +636,16 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
 	state->sanity_errors >>= 1;
 
 	return osize;
+
+sanity_error:
+	if (state->sanity_errors < SANITY_MAX)
+		return DECOMP_ERROR;
+	else
+		/* Take LCP down if the peer is sending too many bogons.
+		 * We don't want to do this for a single or just a few
+		 * instances since it could just be due to packet corruption.
+		 */
+		return DECOMP_FATALERROR;
 }
 
 /*
-- 
2.1.4

[PATCH 2/2] ppp: mppe: discard late packet in stateless mode

[Sylvain Rochet]

When PPP is used over a link which does not guarantee packet ordering,
we might get late MPPE packets. This is a problem because MPPE must be
kept synchronized and the current implementation does not drop them and
rekey 4095 times instead of 0, which is wrong.

In order to prevent rekeying about a whole count space times (~ 4095
times), drop packets which are not within the forward 4096/2 window and
increase sanity error counter.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
---
 drivers/net/ppp/ppp_mppe.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 692ee0f..05005c6 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -533,6 +533,13 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
 	 */
 
 	if (!state->stateful) {
+		/* Discard late packet */
+		if ((ccount - state->ccount) % MPPE_CCOUNT_SPACE
+						> MPPE_CCOUNT_SPACE / 2) {
+			state->sanity_errors++;
+			goto sanity_error;
+		}
+
 		/* RFC 3078, sec 8.1.  Rekey for every packet. */
 		while (state->ccount != ccount) {
 			mppe_rekey(state, 0);
-- 
2.1.4

Re: [PATCH 0/2] ppp: mppe: fixes MPPE desync on links which don't guarantee packet ordering

[David Miller]

From: Sylvain Rochet <sylvain.rochet@finsecur.com>
Date: Sun, 26 Apr 2015 20:40:51 +0200

> I am currently having an issue with PPP over L2TP (UDP) and MPPE in
> stateless mode (default mode), UDP does not guarantee packet ordering so
> we might get out of order packet. MPPE needs to be continuously synched
> so we should drop late UDP packet.
> 
> I added a printk on the number of time we rekeyed in MPPE decompressor,
> this is what we currently have if we receive a slightly out of order UDP
> packet:
 ...
> This is obviously wrong, we missed packet 1561 and we already rekeyed 2
> times for 1562 we previously received, we can't recover the decryption
> key we need for 1561, we should drop it instead of rekeying 4095 times.
> 
> This patch series drop any packet with are not within the 4096/2 forward
> window.

Series applied, thanks Sylvain.