[PATCH] SCTP: Fix possible memory leak while process INIT chunk with AUTH paramters

[PATCH] SCTP: Fix possible memory leak while process INIT chunk with AUTH paramters

[Wei Yongjun]

While endpoint received INIT/INIT-ACK chunk with AUTH parameters, such 
as RANDOM, HMAC_ALGO, CHUNKS parameter, if those parameters appear more 
then once, memory for store those parameters will be malloc more then 
once and not free.

This patch change to used the first parameter and ignore the others.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

--- a/net/sctp/sm_make_chunk.c	2008-02-11 17:14:05.000000000 -0500
+++ b/net/sctp/sm_make_chunk.c	2008-02-14 03:57:58.000000000 -0500
@@ -2458,6 +2458,9 @@ static int sctp_process_param(struct sct
 		if (!sctp_auth_enable)
 			goto fall_through;
 
+		if (asoc->peer.peer_random)
+			break;
+
 		/* Save peer's random parameter */
 		asoc->peer.peer_random = kmemdup(param.p,
 					    ntohs(param.p->length), gfp);
@@ -2471,6 +2474,9 @@ static int sctp_process_param(struct sct
 		if (!sctp_auth_enable)
 			goto fall_through;
 
+		if (asoc->peer.peer_hmacs) 
+			break;
+
 		/* Save peer's HMAC list */
 		asoc->peer.peer_hmacs = kmemdup(param.p,
 					    ntohs(param.p->length), gfp);
@@ -2487,6 +2493,9 @@ static int sctp_process_param(struct sct
 		if (!sctp_auth_enable)
 			goto fall_through;
 
+		if (asoc->peer.peer_chunks)
+			break;
+
 		asoc->peer.peer_chunks = kmemdup(param.p,
 					    ntohs(param.p->length), gfp);
 		if (!asoc->peer.peer_chunks)

Re: [PATCH] SCTP: Fix possible memory leak while process INIT chunk with AUTH paramters

[Vlad Yasevich]

Hi Wei

Wei Yongjun wrote:
> While endpoint received INIT/INIT-ACK chunk with AUTH parameters, such 
> as RANDOM, HMAC_ALGO, CHUNKS parameter, if those parameters appear more 
> then once, memory for store those parameters will be malloc more then 
> once and not free.
> 

All these parameters must be included only once in the packet.

If these things are included more then once, we should either ABORT or
completely ignore the packet.  I haven't decided which one makes more
sense yet.

If someone when to the trouble of violating the protocol, we should not
establish the association with them.

-vlad

> This patch change to used the first parameter and ignore the others.
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> 
> --- a/net/sctp/sm_make_chunk.c    2008-02-11 17:14:05.000000000 -0500
> +++ b/net/sctp/sm_make_chunk.c    2008-02-14 03:57:58.000000000 -0500
> @@ -2458,6 +2458,9 @@ static int sctp_process_param(struct sct
>         if (!sctp_auth_enable)
>             goto fall_through;
> 
> +        if (asoc->peer.peer_random)
> +            break;
> +
>         /* Save peer's random parameter */
>         asoc->peer.peer_random = kmemdup(param.p,
>                         ntohs(param.p->length), gfp);
> @@ -2471,6 +2474,9 @@ static int sctp_process_param(struct sct
>         if (!sctp_auth_enable)
>             goto fall_through;
> 
> +        if (asoc->peer.peer_hmacs) +            break;
> +
>         /* Save peer's HMAC list */
>         asoc->peer.peer_hmacs = kmemdup(param.p,
>                         ntohs(param.p->length), gfp);
> @@ -2487,6 +2493,9 @@ static int sctp_process_param(struct sct
>         if (!sctp_auth_enable)
>             goto fall_through;
> 
> +        if (asoc->peer.peer_chunks)
> +            break;
> +
>         asoc->peer.peer_chunks = kmemdup(param.p,
>                         ntohs(param.p->length), gfp);
>         if (!asoc->peer.peer_chunks)
> 
> 
> 

Re: [PATCH] SCTP: Fix possible memory leak while process INIT chunk with AUTH paramters

[Wei Yongjun]

Hi Vlad:

Vlad Yasevich wrote:
> Hi Wei
>
> Wei Yongjun wrote:
>> While endpoint received INIT/INIT-ACK chunk with AUTH parameters, 
>> such as RANDOM, HMAC_ALGO, CHUNKS parameter, if those parameters 
>> appear more then once, memory for store those parameters will be 
>> malloc more then once and not free.
>>
>
> All these parameters must be included only once in the packet.

RFC 4890 has the following text:

   The RANDOM parameter MUST be included once in the INIT or INIT-ACK
   chunk, if the sender wants to send or receive authenticated chunks,
   to provide a 32-byte Random Number.  For 32-byte Random Numbers, the
   Padding is empty.


It said *MUST be included once*, not *only once*, is this right?

>
> If these things are included more then once, we should either ABORT or
> completely ignore the packet.  I haven't decided which one makes more
> sense yet.
>
> If someone when to the trouble of violating the protocol, we should not
> establish the association with them.

I think do ABORT with protocol violation is better, do the same thing as 
the other protocol violation case do.

Wei Yongjun

Re: [PATCH] SCTP: Fix possible memory leak while process INIT chunk with AUTH paramters

[Vlad Yasevich]

Wei Yongjun wrote:
> Hi Vlad:
> 
> Vlad Yasevich wrote:
>> Hi Wei
>>
>> Wei Yongjun wrote:
>>> While endpoint received INIT/INIT-ACK chunk with AUTH parameters, 
>>> such as RANDOM, HMAC_ALGO, CHUNKS parameter, if those parameters 
>>> appear more then once, memory for store those parameters will be 
>>> malloc more then once and not free.
>>>
>>
>> All these parameters must be included only once in the packet.
> 
> RFC 4890 has the following text:
> 
>   The RANDOM parameter MUST be included once in the INIT or INIT-ACK
>   chunk, if the sender wants to send or receive authenticated chunks,
>   to provide a 32-byte Random Number.  For 32-byte Random Numbers, the
>   Padding is empty.
> 
> 
> It said *MUST be included once*, not *only once*, is this right?

I guess it depends on the interpretation.  If they are allowed more then
once, then which parameter should be used.  The spec leaves that undefined.

Undefined behavior on a security extension is usually treated as an exploit.
That's my take on this.

> 
>>
>> If these things are included more then once, we should either ABORT or
>> completely ignore the packet.  I haven't decided which one makes more
>> sense yet.
>>
>> If someone when to the trouble of violating the protocol, we should not
>> establish the association with them.
> 
> I think do ABORT with protocol violation is better, do the same thing as 
> the other protocol violation case do.

Yes.

-vlad
> 
> Wei Yongjun
>