Patchwork [4/4] cifs: verify lengths of QueryAllEAs reply

login
register
mail settings
Submitter Jeff Layton
Date Jan. 11, 2010, 9:02 p.m.
Message ID <1263243721-3782-5-git-send-email-jlayton@redhat.com>
Download mbox | patch
Permalink /patch/42665/
State New
Headers show

Comments

Jeff Layton - Jan. 11, 2010, 9:02 p.m.
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 <jlayton@redhat.com>
---
 fs/cifs/cifssmb.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)
Steve French - Jan. 11, 2010, 9:57 p.m.
The others look fine - has anyone proved that this fixes a problem in the
field (if so, I would like to push it upstream).

On Mon, Jan 11, 2010 at 3:02 PM, Jeff Layton <jlayton@redhat.com> 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 <jlayton@redhat.com>
> ---
>  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 - Jan. 12, 2010, 12:25 a.m.
On Mon, 11 Jan 2010 15:57:44 -0600
Steve French <smfrench@gmail.com> 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 <jlayton@redhat.com> 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 <jlayton@redhat.com>
> > ---
> >  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 - Jan. 12, 2010, 1 a.m.
On Mon, 11 Jan 2010 15:57:44 -0600
Steve French <smfrench@gmail.com> 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).
> 
> On Mon, Jan 11, 2010 at 3:02 PM, Jeff Layton <jlayton@redhat.com> 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 <jlayton@redhat.com>
> > ---
> >  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) {
				   ^^^^^^^^
			erm...I think I forgot to figure in the null
			terminator here on the name. I'll respin and
			resend. In case it's not obvious, please be
			sure to sanity check my pointer math in this
			patch :)

> > +                       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
> >
> >
> 
>
Steve French - Jan. 12, 2010, 1:25 a.m.
On Mon, Jan 11, 2010 at 7:00 PM, Jeff Layton <jlayton@redhat.com> wrote:

> On Mon, 11 Jan 2010 15:57:44 -0600
> Steve French <smfrench@gmail.com> wrote:
>
> > > +               }
> > > +
> > > +               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) {
>                                    ^^^^^^^^
>                        erm...I think I forgot to figure in the null
>                        terminator here on the name. I'll respin and
>                        resend. In case it's not obvious, please be
>                        sure to sanity check my pointer math in this
>                        patch :)
>
>
ok ... :)

Patch

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