[PATCH 1/2] ax25: integer overflows in ax25_setsockopt()

[PATCH 1/2] ax25: integer overflows in ax25_setsockopt()

[Xi Wang]

ax25_setsockopt() misses several upper-bound checks on the
user-controlled value.


Reported-by: Fan Long <longfancn@gmail.com>
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 net/ax25/af_ax25.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index e7c69f4..be6a8cf 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -571,7 +571,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case AX25_T1:
-		if (opt < 1) {
+		if (opt < 1 || opt > 30) {
 			res = -EINVAL;
 			break;
 		}
@@ -580,7 +580,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case AX25_T2:
-		if (opt < 1) {
+		if (opt < 1 || opt > 20) {
 			res = -EINVAL;
 			break;
 		}
@@ -596,7 +596,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case AX25_T3:
-		if (opt < 1) {
+		if (opt < 0 || opt > 3600) {
 			res = -EINVAL;
 			break;
 		}
@@ -604,7 +604,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case AX25_IDLE:
-		if (opt < 0) {
+		if (opt < 0 || opt > 65535) {
 			res = -EINVAL;
 			break;
 		}
-- 
1.7.5.4

Re: [PATCH 1/2] ax25: integer overflows in ax25_setsockopt()

[Alan Cox]

>  	case AX25_T1:
> -		if (opt < 1) {
> +		if (opt < 1 || opt > 30) {

Where do these values come from ? If they are from some 'standard' then
really we should avoid restricting needlessly to it, particularly as
AX.25 isn't well defined and is used for all sorts of crazy stuff where
the usual range of settings isn't useful.

Restricting to the point it would overflow makes sense however.

Alan

Re: [PATCH 1/2] ax25: integer overflows in ax25_setsockopt()

[Xi Wang]

All these magic numbers come from net/ax25/sysctl_net_ax25.c, where
min/max values of each field are set for sysctl.  Is it okay to use
them?

- xi

On Wed, Nov 23, 2011 at 5:44 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>>       case AX25_T1:
>> -             if (opt < 1) {
>> +             if (opt < 1 || opt > 30) {
>
> Where do these values come from ? If they are from some 'standard' then
> really we should avoid restricting needlessly to it, particularly as
> AX.25 isn't well defined and is used for all sorts of crazy stuff where
> the usual range of settings isn't useful.
>
> Restricting to the point it would overflow makes sense however.
>
> Alan
>
>

Re: [PATCH 1/2] ax25: integer overflows in ax25_setsockopt()

[Alan Cox]

On Wed, 23 Nov 2011 09:04:11 -0500
Xi Wang <xi.wang@gmail.com> wrote:

> All these magic numbers come from net/ax25/sysctl_net_ax25.c, where
> min/max values of each field are set for sysctl.  Is it okay to use
> them?

The sysctl range is the 'standard' range, but it's always historically
been possible to override them in apps for special cases. I'm wary of
changing that because people do insane things like AX.25 bounced off the
moon where you need very long timeouts.

Alan

RE: [PATCH 1/2] ax25: integer overflows in ax25_setsockopt()

[David Laight]

 
> > All these magic numbers come from net/ax25/sysctl_net_ax25.c, where
> > min/max values of each field are set for sysctl.  Is it okay to use
> > them?
> 
> The sysctl range is the 'standard' range, but it's always historically
> been possible to override them in apps for special cases. I'm wary of
> changing that because people do insane things like AX.25 
> bounced off the moon where you need very long timeouts.

It is a long time since I wrote any of the X.25 protocol
stack layers, but I would agree that limiting timers to the
values defined in the standard is probably not a good idea.

Even normal telco's may have decided to use values that
are outside the nominal range.

These timers are almost certainly either 'guard' timers
for missing responses or retransmit timers for 'keepalive'
messages - so allowing much larger values doesn't matter.
I'd only limit them in order to stop the code breaking.

The lower limit (1 second) will be below the limit for the
protocol - but exists to stop the code breaking.

	David

Re: [PATCH 1/2] ax25: integer overflows in ax25_setsockopt()

[Ralf Baechle]

On Tue, Nov 22, 2011 at 11:28:24PM -0500, Xi Wang wrote:

> ax25_setsockopt() misses several upper-bound checks on the
> user-controlled value.
> 
> 
> Reported-by: Fan Long <longfancn@gmail.com>
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> ---
>  net/ax25/af_ax25.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index e7c69f4..be6a8cf 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -571,7 +571,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
>  		break;
>  
>  	case AX25_T1:
> -		if (opt < 1) {
> +		if (opt < 1 || opt > 30) {
>  			res = -EINVAL;
>  			break;
>  		}

30 seconds is definitively too low.  The TCP spec assumes that a RTT
of up to 120s is possible.  In slow packet radio networks 15 minutes are
easily possible in HF networks.

How about AX.25 to the P5A mars mission?

A silly value for T1 will be caught further down the road, so no damage
but an application really should receive a sensible return value when
trying something stupid.

If an apps wants to shoot itself into the foot there is nothing wrong with
being the arms dealer so an error check should be something like

	if (val > ULONG_MAX / HZ)
		bail_out;

which will do the right thing even on a 64-bit system and prevent an
overflow in the multiplication further down.

> @@ -580,7 +580,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
>  		break;
>  
>  	case AX25_T2:
> -		if (opt < 1) {
> +		if (opt < 1 || opt > 20) {
>  			res = -EINVAL;
>  			break;
>  		}

An excessive value here could result in a timer being set to expire in
the past similar in effect to setting a very low value.  Again it's ok
if a user tries to shoot himself into the other foot as well.

> @@ -596,7 +596,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
>  		break;
>  
>  	case AX25_T3:
> -		if (opt < 1) {
> +		if (opt < 0 || opt > 3600) {
>  			res = -EINVAL;
>  			break;
>  		}

For a stable link it should be possible to set a very high T3 value,
potencially high enough to effectively disable the T3 functionality.

> @@ -604,7 +604,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
>  		break;
>  
>  	case AX25_IDLE:
> -		if (opt < 0) {
> +		if (opt < 0 || opt > 65535) {
>  			res = -EINVAL;
>  			break;
>  		}

I have an updated patch which I'm testing right now.

  Ralf

Re: [PATCH 1/2] ax25: integer overflows in ax25_setsockopt()

[Xi Wang]

Thanks a lot for your comments!  Look forward to your patch.

- xi