* [PATCH net v3] sctp: asconf's process should verify address parameter is in the beginning
@ 2015-08-26 20:35 Xin Long
2015-08-26 20:42 ` Vlad Yasevich
0 siblings, 1 reply; 5+ messages in thread
From: Xin Long @ 2015-08-26 20:35 UTC (permalink / raw)
To: network dev; +Cc: mleitner, davem, vyasevic
in sctp_process_asconf(), we get address parameter from the beginning of
the addip params. but we never check if it's really there. if the addr
param is not there, it still can pass sctp_verify_asconf(), then to be
handled by sctp_process_asconf(), it will not be safe.
so add a code in sctp_verify_asconf() to check the address parameter is in
the beginning, or return false to send abort.
v2->v3:
* put the check in the loop, add the check for multiple address parameters.
v1->v2:
* put the check behind the params' length verify.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
net/sctp/sm_make_chunk.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 06320c8..4068fe1 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -3130,14 +3130,24 @@ bool sctp_verify_asconf(const struct sctp_association *asoc,
case SCTP_PARAM_ERR_CAUSE:
break;
case SCTP_PARAM_IPV4_ADDRESS:
+ if (addr_param_seen) {
+ /* peer placed multiple address parameters into
+ * the same asconf. reject it.
+ */
+ return false;
+ }
if (length != sizeof(sctp_ipv4addr_param_t))
return false;
- addr_param_seen = true;
+ if (param.v == addip->addip_hdr.params)
+ addr_param_seen = true;
break;
case SCTP_PARAM_IPV6_ADDRESS:
+ if (addr_param_seen)
+ return false;
if (length != sizeof(sctp_ipv6addr_param_t))
return false;
- addr_param_seen = true;
+ if (param.v == addip->addip_hdr.params)
+ addr_param_seen = true;
break;
case SCTP_PARAM_ADD_IP:
case SCTP_PARAM_DEL_IP:
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net v3] sctp: asconf's process should verify address parameter is in the beginning
2015-08-26 20:35 [PATCH net v3] sctp: asconf's process should verify address parameter is in the beginning Xin Long
@ 2015-08-26 20:42 ` Vlad Yasevich
2015-08-26 20:59 ` Marcelo Ricardo Leitner
0 siblings, 1 reply; 5+ messages in thread
From: Vlad Yasevich @ 2015-08-26 20:42 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: mleitner, davem
On 08/26/2015 04:35 PM, Xin Long wrote:
> in sctp_process_asconf(), we get address parameter from the beginning of
> the addip params. but we never check if it's really there. if the addr
> param is not there, it still can pass sctp_verify_asconf(), then to be
> handled by sctp_process_asconf(), it will not be safe.
>
> so add a code in sctp_verify_asconf() to check the address parameter is in
> the beginning, or return false to send abort.
>
> v2->v3:
> * put the check in the loop, add the check for multiple address parameters.
Please split the multiple address detection from first address detection.
They are 2 different bugs and each one deserves a separate commit and
changelog.
Thanks
-vlad
> v1->v2:
> * put the check behind the params' length verify.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
> net/sctp/sm_make_chunk.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 06320c8..4068fe1 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -3130,14 +3130,24 @@ bool sctp_verify_asconf(const struct sctp_association *asoc,
> case SCTP_PARAM_ERR_CAUSE:
> break;
> case SCTP_PARAM_IPV4_ADDRESS:
> + if (addr_param_seen) {
> + /* peer placed multiple address parameters into
> + * the same asconf. reject it.
> + */
> + return false;
> + }
> if (length != sizeof(sctp_ipv4addr_param_t))
> return false;
> - addr_param_seen = true;
> + if (param.v == addip->addip_hdr.params)
> + addr_param_seen = true;
> break;
> case SCTP_PARAM_IPV6_ADDRESS:
> + if (addr_param_seen)
> + return false;
> if (length != sizeof(sctp_ipv6addr_param_t))
> return false;
> - addr_param_seen = true;
> + if (param.v == addip->addip_hdr.params)
> + addr_param_seen = true;
> break;
> case SCTP_PARAM_ADD_IP:
> case SCTP_PARAM_DEL_IP:
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net v3] sctp: asconf's process should verify address parameter is in the beginning
2015-08-26 20:42 ` Vlad Yasevich
@ 2015-08-26 20:59 ` Marcelo Ricardo Leitner
2015-08-26 21:09 ` lucien xin
0 siblings, 1 reply; 5+ messages in thread
From: Marcelo Ricardo Leitner @ 2015-08-26 20:59 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: Xin Long, network dev, davem
On Wed, Aug 26, 2015 at 04:42:21PM -0400, Vlad Yasevich wrote:
> On 08/26/2015 04:35 PM, Xin Long wrote:
> > in sctp_process_asconf(), we get address parameter from the beginning of
> > the addip params. but we never check if it's really there. if the addr
> > param is not there, it still can pass sctp_verify_asconf(), then to be
> > handled by sctp_process_asconf(), it will not be safe.
> >
> > so add a code in sctp_verify_asconf() to check the address parameter is in
> > the beginning, or return false to send abort.
> >
> > v2->v3:
> > * put the check in the loop, add the check for multiple address parameters.
>
>
> Please split the multiple address detection from first address detection.
> They are 2 different bugs and each one deserves a separate commit and
> changelog.
See below, thx.
>
> Thanks
> -vlad
>
> > v1->v2:
> > * put the check behind the params' length verify.
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> > ---
> > net/sctp/sm_make_chunk.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> > index 06320c8..4068fe1 100644
> > --- a/net/sctp/sm_make_chunk.c
> > +++ b/net/sctp/sm_make_chunk.c
> > @@ -3130,14 +3130,24 @@ bool sctp_verify_asconf(const struct sctp_association *asoc,
> > case SCTP_PARAM_ERR_CAUSE:
> > break;
> > case SCTP_PARAM_IPV4_ADDRESS:
> > + if (addr_param_seen) {
> > + /* peer placed multiple address parameters into
> > + * the same asconf. reject it.
> > + */
> > + return false;
> > + }
> > if (length != sizeof(sctp_ipv4addr_param_t))
> > return false;
> > - addr_param_seen = true;
> > + if (param.v == addip->addip_hdr.params)
> > + addr_param_seen = true;
> > break;
I know I had suggested using addr_param_seen to check for multiple
occurrences, but now realized we can simplify this with something like:
+ if (param.v != addip->addip_hdr.params)
+ return false;
addr_param_seen = true;
Then the check against addr_param_seen is not needed and do both checks
at once.
Thanks,
Marcelo
> > case SCTP_PARAM_IPV6_ADDRESS:
> > + if (addr_param_seen)
> > + return false;
> > if (length != sizeof(sctp_ipv6addr_param_t))
> > return false;
> > - addr_param_seen = true;
> > + if (param.v == addip->addip_hdr.params)
> > + addr_param_seen = true;
> > break;
> > case SCTP_PARAM_ADD_IP:
> > case SCTP_PARAM_DEL_IP:
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net v3] sctp: asconf's process should verify address parameter is in the beginning
2015-08-26 20:59 ` Marcelo Ricardo Leitner
@ 2015-08-26 21:09 ` lucien xin
2015-08-26 21:37 ` Vlad Yasevich
0 siblings, 1 reply; 5+ messages in thread
From: lucien xin @ 2015-08-26 21:09 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: Vlad Yasevich, network dev, davem
On Thu, Aug 27, 2015 at 4:59 AM, Marcelo Ricardo Leitner
<mleitner@redhat.com> wrote:
> On Wed, Aug 26, 2015 at 04:42:21PM -0400, Vlad Yasevich wrote:
>> On 08/26/2015 04:35 PM, Xin Long wrote:
>> > in sctp_process_asconf(), we get address parameter from the beginning of
>> > the addip params. but we never check if it's really there. if the addr
>> > param is not there, it still can pass sctp_verify_asconf(), then to be
>> > handled by sctp_process_asconf(), it will not be safe.
>> >
>> > so add a code in sctp_verify_asconf() to check the address parameter is in
>> > the beginning, or return false to send abort.
>> >
>> > v2->v3:
>> > * put the check in the loop, add the check for multiple address parameters.
>>
>>
>> Please split the multiple address detection from first address detection.
>> They are 2 different bugs and each one deserves a separate commit and
>> changelog.
>
> See below, thx.
>
>>
>> Thanks
>> -vlad
>>
>> > v1->v2:
>> > * put the check behind the params' length verify.
>> >
>> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> > ---
>> > net/sctp/sm_make_chunk.c | 14 ++++++++++++--
>> > 1 file changed, 12 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>> > index 06320c8..4068fe1 100644
>> > --- a/net/sctp/sm_make_chunk.c
>> > +++ b/net/sctp/sm_make_chunk.c
>> > @@ -3130,14 +3130,24 @@ bool sctp_verify_asconf(const struct sctp_association *asoc,
>> > case SCTP_PARAM_ERR_CAUSE:
>> > break;
>> > case SCTP_PARAM_IPV4_ADDRESS:
>> > + if (addr_param_seen) {
>> > + /* peer placed multiple address parameters into
>> > + * the same asconf. reject it.
>> > + */
>> > + return false;
>> > + }
>> > if (length != sizeof(sctp_ipv4addr_param_t))
>> > return false;
>> > - addr_param_seen = true;
>> > + if (param.v == addip->addip_hdr.params)
>> > + addr_param_seen = true;
>> > break;
>
> I know I had suggested using addr_param_seen to check for multiple
> occurrences, but now realized we can simplify this with something like:
>
> + if (param.v != addip->addip_hdr.params)
> + return false;
> addr_param_seen = true;
>
> Then the check against addr_param_seen is not needed and do both checks
> at once.
>
looks nice, Vlad ?
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net v3] sctp: asconf's process should verify address parameter is in the beginning
2015-08-26 21:09 ` lucien xin
@ 2015-08-26 21:37 ` Vlad Yasevich
0 siblings, 0 replies; 5+ messages in thread
From: Vlad Yasevich @ 2015-08-26 21:37 UTC (permalink / raw)
To: lucien xin, Marcelo Ricardo Leitner; +Cc: network dev, davem
On 08/26/2015 05:09 PM, lucien xin wrote:
> On Thu, Aug 27, 2015 at 4:59 AM, Marcelo Ricardo Leitner
> <mleitner@redhat.com> wrote:
>> On Wed, Aug 26, 2015 at 04:42:21PM -0400, Vlad Yasevich wrote:
>>> On 08/26/2015 04:35 PM, Xin Long wrote:
>>>> in sctp_process_asconf(), we get address parameter from the beginning of
>>>> the addip params. but we never check if it's really there. if the addr
>>>> param is not there, it still can pass sctp_verify_asconf(), then to be
>>>> handled by sctp_process_asconf(), it will not be safe.
>>>>
>>>> so add a code in sctp_verify_asconf() to check the address parameter is in
>>>> the beginning, or return false to send abort.
>>>>
>>>> v2->v3:
>>>> * put the check in the loop, add the check for multiple address parameters.
>>>
>>>
>>> Please split the multiple address detection from first address detection.
>>> They are 2 different bugs and each one deserves a separate commit and
>>> changelog.
>>
>> See below, thx.
>>
>>>
>>> Thanks
>>> -vlad
>>>
>>>> v1->v2:
>>>> * put the check behind the params' length verify.
>>>>
>>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>>> ---
>>>> net/sctp/sm_make_chunk.c | 14 ++++++++++++--
>>>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>>>> index 06320c8..4068fe1 100644
>>>> --- a/net/sctp/sm_make_chunk.c
>>>> +++ b/net/sctp/sm_make_chunk.c
>>>> @@ -3130,14 +3130,24 @@ bool sctp_verify_asconf(const struct sctp_association *asoc,
>>>> case SCTP_PARAM_ERR_CAUSE:
>>>> break;
>>>> case SCTP_PARAM_IPV4_ADDRESS:
>>>> + if (addr_param_seen) {
>>>> + /* peer placed multiple address parameters into
>>>> + * the same asconf. reject it.
>>>> + */
>>>> + return false;
>>>> + }
>>>> if (length != sizeof(sctp_ipv4addr_param_t))
>>>> return false;
>>>> - addr_param_seen = true;
>>>> + if (param.v == addip->addip_hdr.params)
>>>> + addr_param_seen = true;
>>>> break;
>>
>> I know I had suggested using addr_param_seen to check for multiple
>> occurrences, but now realized we can simplify this with something like:
>>
>> + if (param.v != addip->addip_hdr.params)
>> + return false;
>> addr_param_seen = true;
>>
>> Then the check against addr_param_seen is not needed and do both checks
>> at once.
>>
> looks nice, Vlad ?
>
yes. This is fine too. I think this kills 2 bugs with 1 patch...
If you go this route, make sure to document this well in the change log.
-vlad
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-08-26 21:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-26 20:35 [PATCH net v3] sctp: asconf's process should verify address parameter is in the beginning Xin Long
2015-08-26 20:42 ` Vlad Yasevich
2015-08-26 20:59 ` Marcelo Ricardo Leitner
2015-08-26 21:09 ` lucien xin
2015-08-26 21:37 ` Vlad Yasevich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).