[PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface
Hi,
> Well, your opinions are welcome. Plus any hints as to how to fix this.
> I'd tend to simply(?) add some more fields to the
> {hash,get,set,delete}_item() functions in drivers/net/pppoe.c.
> But maybe there is some better way?
As noone seems to have an opinion on this: Here is a patch that does
work for me and that should solve the problem as far as that is easily
possible. It is based on the assumption that an interface's ifindex is
basically an alias for a local MAC address, so incoming packets now are
matched to sockets based on remote MAC, session id, and ifindex of the
interface the packet came in on/the socket was bound to by connect().
For relayed packets, the socket that's used for relaying is selected
based on destination MAC, session ID and the interface index of the
interface whose name currently matches the name requested by userspace
as the relaying source interface. The relaying part of the patch is
untested.
Please note that I'd consider this a security fix for reasons outlined
in previous mails.
Florian
--- linux-2.6.20/drivers/net/pppoe.c.orig 2007-02-25 19:23:51.0
+0100
+++ linux-2.6.20/drivers/net/pppoe.c2007-02-28 12:56:05.0 +0100
@@ -7,6 +7,12 @@
*
* Version:0.7.0
*
+ * 070228 :Fix to allow multiple sessions with same remote MAC and same
+ * session id by including the local device ifindex in the
+ * tuple identifying a session. This also ensures packets can't
+ * be injected into a session from interfaces other than the one
+ * specified by userspace. Florian Zumbiehl <[EMAIL PROTECTED]>
+ * (Oh, BTW, this one is YYMMDD, in case you were wondering ...)
* 220102 :Fix module use count on failure in pppoe_create, pppox_sk -acme
* 030700 :Fixed connect logic to allow for disconnect.
* 270700 :Fixed potential SMP problems; we must protect against
@@ -127,14 +133,14 @@
* Set/get/delete/rehash items (internal versions)
*
**/
-static struct pppox_sock *__get_item(unsigned long sid, unsigned char *addr)
+static struct pppox_sock *__get_item(unsigned long sid, unsigned char *addr,
int ifindex)
{
int hash = hash_item(sid, addr);
struct pppox_sock *ret;
ret = item_hash_table[hash];
- while (ret && !cmp_addr(&ret->pppoe_pa, sid, addr))
+ while (ret && !(cmp_addr(&ret->pppoe_pa, sid, addr) &&
ret->pppoe_dev->ifindex == ifindex))
ret = ret->next;
return ret;
@@ -147,21 +153,19 @@
ret = item_hash_table[hash];
while (ret) {
- if (cmp_2_addr(&ret->pppoe_pa, &po->pppoe_pa))
+ if (cmp_2_addr(&ret->pppoe_pa, &po->pppoe_pa) &&
ret->pppoe_dev->ifindex == po->pppoe_dev->ifindex)
return -EALREADY;
ret = ret->next;
}
- if (!ret) {
- po->next = item_hash_table[hash];
- item_hash_table[hash] = po;
- }
+ po->next = item_hash_table[hash];
+ item_hash_table[hash] = po;
return 0;
}
-static struct pppox_sock *__delete_item(unsigned long sid, char *addr)
+static struct pppox_sock *__delete_item(unsigned long sid, char *addr, int
ifindex)
{
int hash = hash_item(sid, addr);
struct pppox_sock *ret, **src;
@@ -170,7 +174,7 @@
src = &item_hash_table[hash];
while (ret) {
- if (cmp_addr(&ret->pppoe_pa, sid, addr)) {
+ if (cmp_addr(&ret->pppoe_pa, sid, addr) &&
ret->pppoe_dev->ifindex == ifindex) {
*src = ret->next;
break;
}
@@ -188,12 +192,12 @@
*
**/
static inline struct pppox_sock *get_item(unsigned long sid,
-unsigned char *addr)
+unsigned char *addr, int ifindex)
{
struct pppox_sock *po;
read_lock_bh(&pppoe_hash_lock);
- po = __get_item(sid, addr);
+ po = __get_item(sid, addr, ifindex);
if (po)
sock_hold(sk_pppox(po));
read_unlock_bh(&pppoe_hash_lock);
@@ -203,7 +207,15 @@
static inline struct pppox_sock *get_item_by_addr(struct sockaddr_pppox *sp)
{
- return get_item(sp->sa_addr.pppoe.sid, sp->sa_addr.pppoe.remote);
+ struct net_device *dev = NULL;
+ int ifindex;
+
+ dev = dev_get_by_name(sp->sa_addr.pppoe.dev);
+ if(!dev)
+ return NULL;
+ ifindex = dev->ifindex;
+ dev_put(dev);
+ return get_item(sp->sa_addr.pppoe.sid, sp->sa_addr.pppoe.remote,
ifindex);
}
static inline int set_item(struct pppox_sock *po)
@@ -220,12 +232,12 @@
return i;
}
-static inline struct pppox_sock *delete_item(unsigned long sid, char *addr)
+static inline struct pppox_sock *delete_item(unsigned lo
Re: [PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface
From: Florian Zumbiehl <[EMAIL PROTECTED]> Date: Wed, 28 Feb 2007 13:38:44 +0100 > As noone seems to have an opinion on this: Here is a patch that does > work for me and that should solve the problem as far as that is easily > possible. It is based on the assumption that an interface's ifindex is > basically an alias for a local MAC address, so incoming packets now are > matched to sockets based on remote MAC, session id, and ifindex of the > interface the packet came in on/the socket was bound to by connect(). I agree with your analysis and have applied your patch. Another way to implement this would have been to store the pre-computed ifindex on the kernel side sockaddr. Thanks. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface
Hi,
> From: Florian Zumbiehl <[EMAIL PROTECTED]>
> Date: Wed, 28 Feb 2007 13:38:44 +0100
>
> > As noone seems to have an opinion on this: Here is a patch that does
> > work for me and that should solve the problem as far as that is easily
> > possible. It is based on the assumption that an interface's ifindex is
> > basically an alias for a local MAC address, so incoming packets now are
> > matched to sockets based on remote MAC, session id, and ifindex of the
> > interface the packet came in on/the socket was bound to by connect().
>
> I agree with your analysis and have applied your patch.
Below you find a slightly changed version of the patch that avoids
a possible NULL pointer dereference in case pppoe_device_event()/
pppoe_flush_dev() dev_put()s dev and sets it to NULL before pppoe_connect()
tries to unbind from the previous address, in which case it would
dereference the NULL pointer in dev.
It now saves the ifindex in the socket's data structure upon connect(),
so that it's still available for finding the entry to remove from the
hash table in case pppoe_device_event() should have dropped the socket's
reference to dev.
> Another way to implement this would have been to store the
> pre-computed ifindex on the kernel side sockaddr.
Well, that probably depends on the intended semantics. There isn't any
documentation somewhere that specifies what the intended behaviour is,
is there?!
Florian
--- linux-2.6.20/drivers/net/pppoe.c.orig 2007-02-25 19:23:51.0
+0100
+++ linux-2.6.20/drivers/net/pppoe.c2007-03-04 02:11:51.0 +0100
@@ -7,6 +7,12 @@
*
* Version:0.7.0
*
+ * 070228 :Fix to allow multiple sessions with same remote MAC and same
+ * session id by including the local device ifindex in the
+ * tuple identifying a session. This also ensures packets can't
+ * be injected into a session from interfaces other than the one
+ * specified by userspace. Florian Zumbiehl <[EMAIL PROTECTED]>
+ * (Oh, BTW, this one is YYMMDD, in case you were wondering ...)
* 220102 :Fix module use count on failure in pppoe_create, pppox_sk -acme
* 030700 :Fixed connect logic to allow for disconnect.
* 270700 :Fixed potential SMP problems; we must protect against
@@ -127,14 +133,14 @@
* Set/get/delete/rehash items (internal versions)
*
**/
-static struct pppox_sock *__get_item(unsigned long sid, unsigned char *addr)
+static struct pppox_sock *__get_item(unsigned long sid, unsigned char *addr,
int ifindex)
{
int hash = hash_item(sid, addr);
struct pppox_sock *ret;
ret = item_hash_table[hash];
- while (ret && !cmp_addr(&ret->pppoe_pa, sid, addr))
+ while (ret && !(cmp_addr(&ret->pppoe_pa, sid, addr) &&
ret->pppoe_ifindex == ifindex))
ret = ret->next;
return ret;
@@ -147,21 +153,19 @@
ret = item_hash_table[hash];
while (ret) {
- if (cmp_2_addr(&ret->pppoe_pa, &po->pppoe_pa))
+ if (cmp_2_addr(&ret->pppoe_pa, &po->pppoe_pa) &&
ret->pppoe_ifindex == po->pppoe_ifindex)
return -EALREADY;
ret = ret->next;
}
- if (!ret) {
- po->next = item_hash_table[hash];
- item_hash_table[hash] = po;
- }
+ po->next = item_hash_table[hash];
+ item_hash_table[hash] = po;
return 0;
}
-static struct pppox_sock *__delete_item(unsigned long sid, char *addr)
+static struct pppox_sock *__delete_item(unsigned long sid, char *addr, int
ifindex)
{
int hash = hash_item(sid, addr);
struct pppox_sock *ret, **src;
@@ -170,7 +174,7 @@
src = &item_hash_table[hash];
while (ret) {
- if (cmp_addr(&ret->pppoe_pa, sid, addr)) {
+ if (cmp_addr(&ret->pppoe_pa, sid, addr) && ret->pppoe_ifindex
== ifindex) {
*src = ret->next;
break;
}
@@ -188,12 +192,12 @@
*
**/
static inline struct pppox_sock *get_item(unsigned long sid,
-unsigned char *addr)
+unsigned char *addr, int ifindex)
{
struct pppox_sock *po;
read_lock_bh(&pppoe_hash_lock);
- po = __get_item(sid, addr);
+ po = __get_item(sid, addr, ifindex);
if (po)
sock_hold(sk_pppox(po));
read_unlock_bh(&pppoe_hash_lock);
@@ -203,7 +207,15 @@
static inline struct pppox_sock *get_item_by_addr(struct sockaddr_pppox *sp)
{
- return get_item(sp->sa_addr.pppoe.sid, sp->sa_addr.pppoe.remote);
+ struct net_device *dev = NULL;
+ int ifindex;
+
+ dev = dev_get_by_name(sp->sa_addr.pppoe.dev);
+ if(!dev)
+ return NULL;
+ ifin
Re: [PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface
From: Florian Zumbiehl <[EMAIL PROTECTED]> Date: Sun, 4 Mar 2007 02:55:16 +0100 > Below you find a slightly changed version of the patch I already applied your first patch, so if you have any fixes to submit please provide them as relative patches to your original change. Thank you. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface
> From: Florian Zumbiehl <[EMAIL PROTECTED]>
> Date: Sun, 4 Mar 2007 02:55:16 +0100
>
> > Below you find a slightly changed version of the patch
>
> I already applied your first patch, so if you have any
> fixes to submit please provide them as relative patches
> to your original change.
>
> Thank you.
Here you go ...
--- linux-2.6.20/drivers/net/pppoe.c.orig 2007-03-04 13:06:01.0
+0100
+++ linux-2.6.20/drivers/net/pppoe.c2007-03-04 02:11:51.0 +0100
@@ -140,7 +140,7 @@
ret = item_hash_table[hash];
- while (ret && !(cmp_addr(&ret->pppoe_pa, sid, addr) &&
ret->pppoe_dev->ifindex == ifindex))
+ while (ret && !(cmp_addr(&ret->pppoe_pa, sid, addr) &&
ret->pppoe_ifindex == ifindex))
ret = ret->next;
return ret;
@@ -153,7 +153,7 @@
ret = item_hash_table[hash];
while (ret) {
- if (cmp_2_addr(&ret->pppoe_pa, &po->pppoe_pa) &&
ret->pppoe_dev->ifindex == po->pppoe_dev->ifindex)
+ if (cmp_2_addr(&ret->pppoe_pa, &po->pppoe_pa) &&
ret->pppoe_ifindex == po->pppoe_ifindex)
return -EALREADY;
ret = ret->next;
@@ -174,7 +174,7 @@
src = &item_hash_table[hash];
while (ret) {
- if (cmp_addr(&ret->pppoe_pa, sid, addr) &&
ret->pppoe_dev->ifindex == ifindex) {
+ if (cmp_addr(&ret->pppoe_pa, sid, addr) && ret->pppoe_ifindex
== ifindex) {
*src = ret->next;
break;
}
@@ -529,7 +529,7 @@
po = pppox_sk(sk);
if (po->pppoe_pa.sid) {
- delete_item(po->pppoe_pa.sid, po->pppoe_pa.remote,
po->pppoe_dev->ifindex);
+ delete_item(po->pppoe_pa.sid, po->pppoe_pa.remote,
po->pppoe_ifindex);
}
if (po->pppoe_dev)
@@ -577,7 +577,7 @@
pppox_unbind_sock(sk);
/* Delete the old binding */
-
delete_item(po->pppoe_pa.sid,po->pppoe_pa.remote,po->pppoe_dev->ifindex);
+
delete_item(po->pppoe_pa.sid,po->pppoe_pa.remote,po->pppoe_ifindex);
if(po->pppoe_dev)
dev_put(po->pppoe_dev);
@@ -597,6 +597,7 @@
goto end;
po->pppoe_dev = dev;
+ po->pppoe_ifindex = dev->ifindex;
if (!(dev->flags & IFF_UP))
goto err_put;
--- linux-2.6.20/include/linux/if_pppox.h.orig 2007-02-09 10:21:19.0
+0100
+++ linux-2.6.20/include/linux/if_pppox.h 2007-03-04 02:14:24.0
+0100
@@ -114,6 +114,7 @@
#ifdef __KERNEL__
struct pppoe_opt {
struct net_device *dev; /* device associated with socket*/
+ int ifindex; /* ifindex of device associated with
socket */
struct pppoe_addr pa; /* what this socket is bound to*/
struct sockaddr_pppox relay;/* what socket data will be
relayed to (PPPoE relaying) */
@@ -132,6 +133,7 @@
unsigned short num;
};
#define pppoe_dev proto.pppoe.dev
+#define pppoe_ifindex proto.pppoe.ifindex
#define pppoe_pa proto.pppoe.pa
#define pppoe_relayproto.pppoe.relay
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface
Sorry for the late reply I've been on the road the past few days. I ACK the patch. I'll need to think about it some more, but we could probably go a step further and eliminate the MAC address from the hash as well. -- Michal Ostrowski <[EMAIL PROTECTED]> On Sat, 2007-03-03 at 21:08 -0800, David Miller wrote: > From: Florian Zumbiehl <[EMAIL PROTECTED]> > Date: Sun, 4 Mar 2007 02:55:16 +0100 > > > Below you find a slightly changed version of the patch > > I already applied your first patch, so if you have any > fixes to submit please provide them as relative patches > to your original change. > > Thank you. > - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][BUG][SECURITY] Re: Weird problem with PPPoE on tap interface
From: Florian Zumbiehl <[EMAIL PROTECTED]> Date: Sun, 4 Mar 2007 13:09:39 +0100 > > From: Florian Zumbiehl <[EMAIL PROTECTED]> > > Date: Sun, 4 Mar 2007 02:55:16 +0100 > > > > > Below you find a slightly changed version of the patch > > > > I already applied your first patch, so if you have any > > fixes to submit please provide them as relative patches > > to your original change. > > > > Thank you. > > Here you go ... Applied, thanks a lot Florian. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
