Re: [PATCH 4/4] cifs: verify lengths of QueryAllEAs reply

On Mon, 11 Jan 2010 15:57:44 -0600
Steve French <[email protected]> wrote:

> The others look fine - has anyone proved that this fixes a problem in the
> field (if so, I would like to push it upstream).
> 

Unfortunately, no. I held out hope for a while that I'd get a capture
that showed one of these malformed responses, but was never able to get
one. The reporter also wasn't keen on testing patches either, so I have
no confirmation that this fixes anything.

> On Mon, Jan 11, 2010 at 3:02 PM, Jeff Layton <[email protected]> wrote:
> 
> > Make sure the lengths in a QUERY_ALL_EAS reply don't make the parser walk
> > off the end of the SMB.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> >  fs/cifs/cifssmb.c |   30 +++++++++++++++++++++++++++---
> >  1 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> > index f5e1527..fb19f43 100644
> > --- a/fs/cifs/cifssmb.c
> > +++ b/fs/cifs/cifssmb.c
> > @@ -5285,6 +5285,7 @@ CIFSSMBQAllEAs(const int xid, struct cifsTconInfo
> > *tcon,
> >        struct fealist *ea_response_data;
> >        struct fea *temp_fea;
> >        char *temp_ptr;
> > +       char *end_of_smb;
> >        __u16 params, byte_count, data_offset;
> >
> >        cFYI(1, ("In Query All EAs path %s", searchName));
> > @@ -5360,11 +5361,19 @@ QAllEAsRetry:
> >        data_offset = le16_to_cpu(pSMBr->t2.DataOffset);
> >        ea_response_data = (struct fealist *)
> >                                (((char *) &pSMBr->hdr.Protocol) +
> > data_offset);
> > -
> >        list_len = le32_to_cpu(ea_response_data->list_len);
> >        cFYI(1, ("ea length %d", list_len));
> >        if (list_len <= 8) {
> >                cFYI(1, ("empty EA list returned from server"));
> > +               rc = -EIO;
> > +               goto QAllEAsOut;
> > +       }
> > +
> > +       /* make sure list_len doesn't go past end of SMB */
> > +       end_of_smb = (char *)pByteArea(&pSMBr->hdr) + BCC(&pSMBr->hdr);
> > +       if ((char *)ea_response_data + list_len > end_of_smb) {
> > +               cFYI(1, ("list_len goes beyond SMB"));
> > +               rc = -EIO;
> >                goto QAllEAsOut;
> >        }
> >
> > @@ -5373,10 +5382,26 @@ QAllEAsRetry:
> >        temp_fea = ea_response_data->list;
> >        temp_ptr = (char *)temp_fea;
> >        while (list_len > 0) {
> > +               __u8 name_len;
> >                __u16 value_len;
> >                list_len -= 4;
> >                temp_ptr += 4;
> > -               rc += temp_fea->name_len;
> > +               /* make sure we can read name_len and value_len */
> > +               if (temp_ptr > end_of_smb) {
> > +                       cFYI(1, ("fealist goes beyond end of SMB"));
> > +                       rc = -EIO;
> > +                       goto QAllEAsOut;
> > +               }
> > +
> > +               name_len = temp_fea->name_len;
> > +               value_len = le16_to_cpu(temp_fea->value_len);
> > +               if (temp_ptr + name_len + value_len > end_of_smb) {
> > +                       cFYI(1, ("fealist goes beyond end of SMB"));
> > +                       rc = -EIO;
> > +                       goto QAllEAsOut;
> > +               }
> > +
> > +               rc += name_len;
> >                /* account for prefix user. and trailing null */
> >                rc = rc + 5 + 1;
> >                if (rc < (int) buf_size) {
> > @@ -5399,7 +5424,6 @@ QAllEAsRetry:
> >                /* account for trailing null */
> >                list_len--;
> >                temp_ptr++;
> > -               value_len = le16_to_cpu(temp_fea->value_len);
> >                list_len -= value_len;
> >                temp_ptr += value_len;
> >                /* BB check that temp_ptr is still
> > --
> > 1.6.5.2
> >
> >
> 
> 


-- 
Jeff Layton <[email protected]>