[PATCH] pppoe: Missed check for destination addr in PADT frame processing

[PATCH] pppoe: Missed check for destination addr in PADT frame processing

[Leonid Lisovskiy]

 One of our customer encountered sporadic disconnects problem with
his ISP when using kernel PPPoE driver. After some investigation, we
discovered that it happens due to PADT frames being received with
destination MAC not matching the client MAC even though they have
proper source MAC and session_id. It seems that these ISP use very
cheap equipment that doesn't provide generation of unique
session_id's. User-space rp-pppoe 3.10 works correctly in this case
since it has an additional check for destination MAC in PADT frame
processing.

This patch adds check for destination MAC address in pppoe_disc_rcv().

Signed-off-by: Leonid Lisovskiy <lly.dev@gmail.com>
---

Tested on one buggy ISP (R-telekom, Kirov, Russia).

 drivers/net/pppoe.c |    4 ++++
 1 file changed, 4 insertions(+)

--- a/drivers/net/pppoe.c
+++ b/drivers/net/pppoe.c
@@ -485,6 +485,10 @@ static int pppoe_disc_rcv(struct sk_buff
 	if (ph->code != PADT_CODE)
 		goto abort;

+	/* Ignore PADT packets whose destination address isn't ours */
+	if (memcmp(eth_hdr(skb)->h_dest, dev->dev_addr, ETH_ALEN))
+		goto abort;
+
 	pn = pppoe_pernet(dev_net(dev));
 	po = get_item(pn, ph->sid, eth_hdr(skb)->h_source, dev->ifindex);
 	if (po) {
--

Re: [PATCH] pppoe: Missed check for destination addr in PADT frame processing

[Leonid Lisovskiy]

On 10/15/10, Sujit K M <sjt.kar@gmail.com> wrote:
>> discovered that it happens due to PADT frames being received with
>> destination MAC not matching the client MAC even though they have
>> proper source MAC and session_id. It seems that these ISP use very
>
> This seems to be a wrong way of interpreting the scenario. I think It should
> be proper source MAC and session_id.

In case of ISP has single access point, it has one source MAC for all
network, right? If such AP is buggy and give two(or more) equal
session_id's to several clients, this cause situation I described
above.

Of course, it should be fixed in ISP hardware, but I apologize that
kernel PPPoE driver should have better foolproof.

Regards,
   Leonid

Re: [PATCH] pppoe: Missed check for destination addr in PADT frame processing

[Sujit K M]

On Wed, Oct 13, 2010 at 6:03 PM, Leonid Lisovskiy <lly.dev@gmail.com> wrote:
>  One of our customer encountered sporadic disconnects problem with
> his ISP when using kernel PPPoE driver. After some investigation, we

I am facing similar sort of disconnect at times when you have for example
having two PPPOE Connections Running, This is very specific example to
your case. My Case is that on an DSL Line When you browse and speak over
the Phone the DSL line Drop's. But once I contacted the Service Provider, They
Corrected It. I think It might have been an switch problem.


> discovered that it happens due to PADT frames being received with
> destination MAC not matching the client MAC even though they have
> proper source MAC and session_id. It seems that these ISP use very

This seems to be a wrong way of interpreting the scenario. I think It should be
proper source MAC and session_id.

> cheap equipment that doesn't provide generation of unique
> session_id's. User-space rp-pppoe 3.10 works correctly in this case
> since it has an additional check for destination MAC in PADT frame
> processing.

Above seems to be acceptable for most of the Modem's.

Re: [PATCH] pppoe: Missed check for destination addr in PADT frame processing

[Sujit K M]

> In case of ISP has single access point, it has one source MAC for all
> network, right? If such AP is buggy and give two(or more) equal
> session_id's to several clients, this cause situation I described
> above.

I am still not clear. The session_id will have an unique value for the pppoe
connection. I think It will be a part of
1. The handshake that goes on
2. Crytography for the device(specific to the modem).

Even If 2 fails, 1 Should fix it(be it single/multiple).

Re: [PATCH] pppoe: Missed check for destination addr in PADT frame processing

[Leonid Lisovskiy]

On 10/15/10, Sujit K M <sjt.kar@gmail.com> wrote:
>> In case of ISP has single access point, it has one source MAC for all
>> network, right? If such AP is buggy and give two(or more) equal
>> session_id's to several clients, this cause situation I described
>> above.
>
> I am still not clear. The session_id will have an unique value for the pppoe
> connection. I think It will be a part of
> 1. The handshake that goes on
> 2. Crytography for the device(specific to the modem).
>
> Even If 2 fails, 1 Should fix it(be it single/multiple).
>

Once more - this is how it should be done by RFC. But due to buggy ISP
hardware uniqueness of session_id violated! Client doesn't know
anything about other clients connected to the ISP, so it can't fix
anything.

Also I don't understand you - how cryptography relates to PPPoE frame header.

Re: [PATCH] pppoe: Missed check for destination addr in PADT frame

[James Carlson]

On 10/15/10 07:11, Leonid Lisovskiy wrote:
> Once more - this is how it should be done by RFC. But due to buggy ISP
> hardware uniqueness of session_id violated! Client doesn't know
> anything about other clients connected to the ISP, so it can't fix
> anything.

It might not necessarily be a bug in the ISP (well, except for the fact
that they're using PPPoE -- that's arguably a separate bug).

There's a known flaw in the way PPPoE assigns session ID numbers.
Instead of assigning a separate session number for each direction (as is
done with most competent tunneling protocols), PPPoE uses a single
session number that's assigned by the server.

This raises a problem.  If you have a single client talking to two or
more servers, there's no guarantee at all that the session IDs assigned
by those servers will be distinct.  If they're not, then the client must
match on MAC address.

Is this the problem you're running into?  Are you talking to two
different servers?

(This is also why the standards process, as cumbersome as it may seem,
is useful.  With decent review, it's possible to fix problems like this
before they become unfixable.  Unfortunately, that's not how PPPoE was
done.)

-- 
James Carlson         42.703N 71.076W         <carlsonj@workingcode.com>

Re: [PATCH] pppoe: Missed check for destination addr in PADT frame processing

[Leonid Lisovskiy]

On 10/15/10, James Carlson <carlsonj@workingcode.com> wrote:
> There's a known flaw in the way PPPoE assigns session ID numbers.
> Instead of assigning a separate session number for each direction (as is
> done with most competent tunneling protocols), PPPoE uses a single
> session number that's assigned by the server.
>
> This raises a problem.  If you have a single client talking to two or
> more servers, there's no guarantee at all that the session IDs assigned
> by those servers will be distinct.  If they're not, then the client must
> match on MAC address.
>
> Is this the problem you're running into?  Are you talking to two
> different servers?

No. Customer, who reports problem, use linux-based router with our
firmware - http://wl500g.googlecode.com to access internet resources
and pppd on it establish PPPoE session with ISP. i.e. there were only
one session from client.

One qualified user("good") of this ISP give us advice about not-unique
session_id. After blocking aliens MAC's and inserting debug printk
into pppoe_disc_rcv(), we got output:

PADT h_source\0:11:95:fc:83:f3 h_dest: 00:0e:08:2f:da:c5 session_id: 1
PADT h_source\0:11:95:fc:83:f3 h_dest: 00:14:85:03:da:49 session_id: 1
PADT h_source\0:11:95:fc:83:f3 h_dest: e0:cb:4e:37:09:66 session_id: 1
PADT h_source\0:11:95:fc:83:f3 h_dest: 00:26:5a:32:68:36 session_id: 1
PADT h_source\0:11:95:fc:83:f3 h_dest: 00:14:85:03:da:49 session_id: 1
PADT h_source\0:11:95:fc:83:f3 h_dest: 00:11:95:fc:83:f3 session_id: 1
PADT h_source\0:11:95:fc:83:f3 h_dest: 00:24:54:2a:14:d7 session_id: 1
PADT h_source\0:11:95:fc:83:f3 h_dest: 00:14:85:03:da:49 session_id: 1
PADT h_source\0:11:95:fc:83:f3 h_dest: 00:26:5a:32:68:36 session_id: 1

where only one MAC e0:cb:4e:37:09:66 belongs to our client router.
Seems to be that ISP has flat (badly segmented) network with buggy
hardware which can't provide uniqueness of session_id.

Further investigation shows that both user-space rp-pppoe & M$ windows
checks destination address in PADT frames.

After fixing problem in our firmware, I decided to push patch into
pppoe driver mainstream to add extra foolproof.

Thank you for detailed explanation!

Regards,
   Leonid

Re: [PATCH] pppoe: Missed check for destination addr in PADT frame

[James Carlson]

On 10/15/10 08:38, Leonid Lisovskiy wrote:
> PADT h_source\0:11:95:fc:83:f3 h_dest: 00:0e:08:2f:da:c5 session_id: 1
> PADT h_source\0:11:95:fc:83:f3 h_dest: 00:14:85:03:da:49 session_id: 1

Wow, that's wacky.  Thanks for the data point.  I've never seen that
behavior before.

For what it's worth, I think there's nothing specifically wrong with
that behavior, so I'd say it's a bit of a stretch calling it a bug.  The
session ID is for the server to use in demuxing; if it can demux without
it, then using 1 for everything doesn't violate anything I can see.

"Normal" clients should not be seeing unicast packets sent to anything
other than their own MAC address -- they should get filtered out at the
MAC layer before PPPoE ever has a chance to see them.  So the reuse
should be invisible.

If you're not filtering for your own MAC address on unicast reception,
I'd be a little concerned that there are other places where the
protocols go awry, and not just PADT.  This might be something to look into.

-- 
James Carlson         42.703N 71.076W         <carlsonj@workingcode.com>