[SECURITY] DECnet: need to validate user data and access data?

[SECURITY] DECnet: need to validate user data and access data?

[Dan Rosenberg]

In net/decnet/af_decnet.c, in the dn_access_copy() and dn_user_copy()
functions, which are called from dn_connect(), length values are
retrieved from incoming skb data and used as size values to copy
functions:

static void dn_access_copy(struct sk_buff *skb, struct accessdata_dn *acc)
{
        unsigned char *ptr = skb->data;

        acc->acc_userl = *ptr++;
        memcpy(&acc->acc_user, ptr, acc->acc_userl);
        ptr += acc->acc_userl;

        acc->acc_passl = *ptr++;
        memcpy(&acc->acc_pass, ptr, acc->acc_passl);
        ptr += acc->acc_passl;

        acc->acc_accl = *ptr++;
        memcpy(&acc->acc_acc, ptr, acc->acc_accl);

        skb_pull(skb, acc->acc_accl + acc->acc_passl + acc->acc_userl + 3);

}

static void dn_user_copy(struct sk_buff *skb, struct optdata_dn *opt)
{
        unsigned char *ptr = skb->data;
        u16 len = *ptr++; /* yes, it's 8bit on the wire */

        BUG_ON(len > 16); /* we've checked the contents earlier */
        opt->opt_optl   = cpu_to_le16(len);
        opt->opt_status = 0;
        memcpy(opt->opt_data, ptr, len);
        skb_pull(skb, len + 1);
}


Despite the BUG_ON and comment suggesting these lengths have been
validated, I don't think this is actually the case - it looks like these
fields are validated for outbound data, but I see no validation for
inbound data (unless I'm mistaken, which is entirely possible).  If this
is the case, this can allow remote attackers to cause controllable heap
corruption.  I'd appreciate it if someone who knows this protocol better
than I do took a look at this and implemented appropriate error handling
if it needs it.

Thanks,
Dan

Re: [Security] [SECURITY] DECnet: need to validate user data and access data?

[Eugene Teo]

Cc'ed the decnet list. Looks like it's still active even though the
status is orphan.

On Tue, Mar 22, 2011 at 7:41 AM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
> In net/decnet/af_decnet.c, in the dn_access_copy() and dn_user_copy()
> functions, which are called from dn_connect(), length values are
> retrieved from incoming skb data and used as size values to copy
> functions:
>
> static void dn_access_copy(struct sk_buff *skb, struct accessdata_dn *acc)
> {
>        unsigned char *ptr = skb->data;
>
>        acc->acc_userl = *ptr++;
>        memcpy(&acc->acc_user, ptr, acc->acc_userl);
>        ptr += acc->acc_userl;
>
>        acc->acc_passl = *ptr++;
>        memcpy(&acc->acc_pass, ptr, acc->acc_passl);
>        ptr += acc->acc_passl;
>
>        acc->acc_accl = *ptr++;
>        memcpy(&acc->acc_acc, ptr, acc->acc_accl);
>
>        skb_pull(skb, acc->acc_accl + acc->acc_passl + acc->acc_userl + 3);
>
> }
>
> static void dn_user_copy(struct sk_buff *skb, struct optdata_dn *opt)
> {
>        unsigned char *ptr = skb->data;
>        u16 len = *ptr++; /* yes, it's 8bit on the wire */
>
>        BUG_ON(len > 16); /* we've checked the contents earlier */
>        opt->opt_optl   = cpu_to_le16(len);
>        opt->opt_status = 0;
>        memcpy(opt->opt_data, ptr, len);
>        skb_pull(skb, len + 1);
> }
>
>
> Despite the BUG_ON and comment suggesting these lengths have been
> validated, I don't think this is actually the case - it looks like these
> fields are validated for outbound data, but I see no validation for
> inbound data (unless I'm mistaken, which is entirely possible).  If this
> is the case, this can allow remote attackers to cause controllable heap
> corruption.  I'd appreciate it if someone who knows this protocol better
> than I do took a look at this and implemented appropriate error handling
> if it needs it.

Re: [Security] [SECURITY] DECnet: need to validate user data and access data?

[Steven Whitehouse]

Hi,

On Tue, 2011-03-22 at 15:42 +0800, Eugene Teo wrote:
> Cc'ed the decnet list. Looks like it's still active even though the
> status is orphan.
> 
Well, kind of active :-) I don't think there is a lot of development
going on despite davem's recent changes to the routing code.

These functions are used in relation to conninit messages which, on the
incoming side are checked in dn_nsp_in.c:dn_find_listener() via the
calls to dn_check_idf() so that we should never queue an incorrectly
formatted message to the socket. The intent was that all messages should
be checked as early as possible on entry to the code so that we can then
rely on their content later on without needing to check again.

I hope that answers your question, but let me know if you need anything
else,

Steve.

> On Tue, Mar 22, 2011 at 7:41 AM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
> > In net/decnet/af_decnet.c, in the dn_access_copy() and dn_user_copy()
> > functions, which are called from dn_connect(), length values are
> > retrieved from incoming skb data and used as size values to copy
> > functions:
> >
> > static void dn_access_copy(struct sk_buff *skb, struct accessdata_dn *acc)
> > {
> >        unsigned char *ptr = skb->data;
> >
> >        acc->acc_userl = *ptr++;
> >        memcpy(&acc->acc_user, ptr, acc->acc_userl);
> >        ptr += acc->acc_userl;
> >
> >        acc->acc_passl = *ptr++;
> >        memcpy(&acc->acc_pass, ptr, acc->acc_passl);
> >        ptr += acc->acc_passl;
> >
> >        acc->acc_accl = *ptr++;
> >        memcpy(&acc->acc_acc, ptr, acc->acc_accl);
> >
> >        skb_pull(skb, acc->acc_accl + acc->acc_passl + acc->acc_userl + 3);
> >
> > }
> >
> > static void dn_user_copy(struct sk_buff *skb, struct optdata_dn *opt)
> > {
> >        unsigned char *ptr = skb->data;
> >        u16 len = *ptr++; /* yes, it's 8bit on the wire */
> >
> >        BUG_ON(len > 16); /* we've checked the contents earlier */
> >        opt->opt_optl   = cpu_to_le16(len);
> >        opt->opt_status = 0;
> >        memcpy(opt->opt_data, ptr, len);
> >        skb_pull(skb, len + 1);
> > }
> >
> >
> > Despite the BUG_ON and comment suggesting these lengths have been
> > validated, I don't think this is actually the case - it looks like these
> > fields are validated for outbound data, but I see no validation for
> > inbound data (unless I'm mistaken, which is entirely possible).  If this
> > is the case, this can allow remote attackers to cause controllable heap
> > corruption.  I'd appreciate it if someone who knows this protocol better
> > than I do took a look at this and implemented appropriate error handling
> > if it needs it.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Security] [SECURITY] DECnet: need to validate user data and access data?

[David Miller]

From: Steven Whitehouse <swhiteho@redhat.com>
Date: Tue, 22 Mar 2011 09:13:50 +0000

> These functions are used in relation to conninit messages which, on the
> incoming side are checked in dn_nsp_in.c:dn_find_listener() via the
> calls to dn_check_idf() so that we should never queue an incorrectly
> formatted message to the socket. The intent was that all messages should
> be checked as early as possible on entry to the code so that we can then
> rely on their content later on without needing to check again.

Ok, so we should be find here.

Thanks for the explanation Steven.

Re: [Security] [SECURITY] DECnet: need to validate user data and access data?

[Dan Rosenberg]

On Tue, 2011-03-22 at 09:13 +0000, Steven Whitehouse wrote:
> Hi,
> 
> On Tue, 2011-03-22 at 15:42 +0800, Eugene Teo wrote:
> > Cc'ed the decnet list. Looks like it's still active even though the
> > status is orphan.
> > 
> Well, kind of active :-) I don't think there is a lot of development
> going on despite davem's recent changes to the routing code.
> 
> These functions are used in relation to conninit messages which, on the
> incoming side are checked in dn_nsp_in.c:dn_find_listener() via the
> calls to dn_check_idf() so that we should never queue an incorrectly
> formatted message to the socket. The intent was that all messages should
> be checked as early as possible on entry to the code so that we can then
> rely on their content later on without needing to check again.
> 
> I hope that answers your question, but let me know if you need anything
> else,

Thanks very much, that does clear it up.  I must have missed it because
I was expecting it to use the array size macro (DN_MAXACCL) rather than
a hard-coded value.  Glad it's a non-issue.

-Dan

> Steve.
> 
> > On Tue, Mar 22, 2011 at 7:41 AM, Dan Rosenberg <drosenberg@vsecurity.com> wrote:
> > > In net/decnet/af_decnet.c, in the dn_access_copy() and dn_user_copy()
> > > functions, which are called from dn_connect(), length values are
> > > retrieved from incoming skb data and used as size values to copy
> > > functions:
> > >
> > > static void dn_access_copy(struct sk_buff *skb, struct accessdata_dn *acc)
> > > {
> > >        unsigned char *ptr = skb->data;
> > >
> > >        acc->acc_userl = *ptr++;
> > >        memcpy(&acc->acc_user, ptr, acc->acc_userl);
> > >        ptr += acc->acc_userl;
> > >
> > >        acc->acc_passl = *ptr++;
> > >        memcpy(&acc->acc_pass, ptr, acc->acc_passl);
> > >        ptr += acc->acc_passl;
> > >
> > >        acc->acc_accl = *ptr++;
> > >        memcpy(&acc->acc_acc, ptr, acc->acc_accl);
> > >
> > >        skb_pull(skb, acc->acc_accl + acc->acc_passl + acc->acc_userl + 3);
> > >
> > > }
> > >
> > > static void dn_user_copy(struct sk_buff *skb, struct optdata_dn *opt)
> > > {
> > >        unsigned char *ptr = skb->data;
> > >        u16 len = *ptr++; /* yes, it's 8bit on the wire */
> > >
> > >        BUG_ON(len > 16); /* we've checked the contents earlier */
> > >        opt->opt_optl   = cpu_to_le16(len);
> > >        opt->opt_status = 0;
> > >        memcpy(opt->opt_data, ptr, len);
> > >        skb_pull(skb, len + 1);
> > > }
> > >
> > >
> > > Despite the BUG_ON and comment suggesting these lengths have been
> > > validated, I don't think this is actually the case - it looks like these
> > > fields are validated for outbound data, but I see no validation for
> > > inbound data (unless I'm mistaken, which is entirely possible).  If this
> > > is the case, this can allow remote attackers to cause controllable heap
> > > corruption.  I'd appreciate it if someone who knows this protocol better
> > > than I do took a look at this and implemented appropriate error handling
> > > if it needs it.
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html