[PATCH next-next] ppp: change default for incoming protocol filter to NPMODE_DROP

[PATCH next-next] ppp: change default for incoming protocol filter to NPMODE_DROP

[Benjamin LaHaise]

By default, the ppp_generic code initializes the npmode array that filters
incoming packet to accept packets for all protocols.  This behaviour is
incorrect, as it results in packets for protocols that an older version
of a PPP implementation may not be aware of to be incorrectly accepted.
This behaviour is visible, for example, when sending IPv6 packets across a
ppp link where pppd has only been configured to use IPv4.

This change should be safe since pppd will correctly set the protocols it
negotiates to NPMODE_PASS as the appropriate protocols transition to an Up
state.

Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
 drivers/net/ppp/ppp_generic.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 5c05572..404ac50 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -2619,7 +2619,7 @@ ppp_create_interface(struct net *net, int unit, int *retp)
 	init_ppp_file(&ppp->file, INTERFACE);
 	ppp->file.hdrlen = PPP_HDRLEN - 2;	/* don't count proto bytes */
 	for (i = 0; i < NUM_NP; ++i)
-		ppp->npmode[i] = NPMODE_PASS;
+		ppp->npmode[i] = NPMODE_DROP;
 	INIT_LIST_HEAD(&ppp->channels);
 	spin_lock_init(&ppp->rlock);
 	spin_lock_init(&ppp->wlock);
-- 
1.7.4.1


-- 
"Thought is the essence of where you are now."

Re: [PATCH next-next] ppp: change default for incoming protocol filter to NPMODE_DROP

[David Miller]

From: Benjamin LaHaise <bcrl@kvack.org>
Date: Tue, 3 Jul 2012 21:32:58 -0400

> By default, the ppp_generic code initializes the npmode array that filters
> incoming packet to accept packets for all protocols.  This behaviour is
> incorrect, as it results in packets for protocols that an older version
> of a PPP implementation may not be aware of to be incorrectly accepted.
> This behaviour is visible, for example, when sending IPv6 packets across a
> ppp link where pppd has only been configured to use IPv4.
> 
> This change should be safe since pppd will correctly set the protocols it
> negotiates to NPMODE_PASS as the appropriate protocols transition to an Up
> state.
> 
> Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>

As far as I can tell, this has been this way for a very long time.

Therefore it is the applications responsibility to adjust the filters
to suit their needs and we really can't make such adjustments to this
behavior.

Re: [PATCH next-next] ppp: change default for incoming protocol filter to NPMODE_DROP

[Benjamin LaHaise]

On Thu, Jul 05, 2012 at 03:00:27AM -0700, David Miller wrote:
> As far as I can tell, this has been this way for a very long time.
> 
> Therefore it is the applications responsibility to adjust the filters
> to suit their needs and we really can't make such adjustments to this
> behavior.

Okay.  Clearing all the protocols the kernel may support in the future is a 
bit expensive due to a lack of a way to get the protocols supported -- the 
code would have to walk the entire protocol id space.  How about the 
following addition instead to provide a list of protocols to disable?

		-ben


[PATCH net-next] ppp: add PPPIOCGPROTOS ioctl to get the list of protocols

At present there is no means for a userspace ppp implementation to get a 
list of protocols supported by the kernel.  Add an ioctl, PPPIOCGPROTOS to 
get the protocol list array where [0] is the number of protocols in the 
array.

Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 5c05572..daf50aa 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -565,6 +565,20 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	void __user *argp = (void __user *)arg;
 	int __user *p = argp;
 
+	if (cmd == PPPIOCGPROTOS) {
+		if (get_user(val, p))
+			return err;
+		if (val <= 0)
+			return -EINVAL;
+		if (NUM_NP < val)
+			val = NUM_NP;
+		if (put_user(val, p))
+			return err;
+		if (copy_to_user(p + 1, &npindex_to_proto, sizeof(int) * val))
+			return err;
+		return 0;
+	}
+
 	if (!pf)
 		return ppp_unattached_ioctl(current->nsproxy->net_ns,
 					pf, file, cmd, arg);
diff --git a/include/linux/ppp-ioctl.h b/include/linux/ppp-ioctl.h
index 2d9a885..d2cc304 100644
--- a/include/linux/ppp-ioctl.h
+++ b/include/linux/ppp-ioctl.h
@@ -81,6 +81,7 @@ struct pppol2tp_ioc_stats {
  * Ioctl definitions.
  */
 
+#define	PPPIOCGPROTOS	_IOWR('t', 90, int)	/* get protocol list array */
 #define	PPPIOCGFLAGS	_IOR('t', 90, int)	/* get configuration flags */
 #define	PPPIOCSFLAGS	_IOW('t', 89, int)	/* set configuration flags */
 #define	PPPIOCGASYNCMAP	_IOR('t', 88, int)	/* get async map */

Re: [PATCH next-next] ppp: change default for incoming protocol filter to NPMODE_DROP

[David Miller]

From: Benjamin LaHaise <bcrl@kvack.org>
Date: Fri, 6 Jul 2012 13:28:00 -0400

> How about the following addition instead to provide a list of
> protocols to disable?

The userspace programs must accomodate all existing kernels, so
the addition of this feature is rather pointless.

Re: [PATCH next-next] ppp: change default for incoming protocol filter to NPMODE_DROP

[Benjamin LaHaise]

On Sat, Jul 07, 2012 at 04:15:04PM -0700, David Miller wrote:
> From: Benjamin LaHaise <bcrl@kvack.org>
> Date: Fri, 6 Jul 2012 13:28:00 -0400
> 
> > How about the following addition instead to provide a list of
> > protocols to disable?
> 
> The userspace programs must accomodate all existing kernels, so
> the addition of this feature is rather pointless.

It's not existing kernels that this guards against, but the use of older 
versions of the API users on new kernels that support additional protocols.  
I'm in the middle of porting a PPP stack to using the ppp_generic interface, 
and there is no way for me to prevent packet types for protocols which are 
newly added to the kernel from getting these new packet types leaked.  I 
came across this exactly because I was testing this case.  I suppose I can 
ignore the issue, but I'd prefer to get it right since it is technically a 
security hole that bypasses PPP session authentication.

		-ben
-- 
"Thought is the essence of where you are now."