[linux-cifs-client] Re: [PATCH] cifs: fix unicode string area word alignment in session setup

[Steve French]

merged into cifs-2.6.git

On Tue, Apr 14, 2009 at 10:00 AM, Jeff Layton <jlayton at redhat.com> wrote:
> The handling of unicode string area alignment is wrong.
> decode_unicode_ssetup improperly assumes that it will always be preceded
> by a pad byte. This isn't the case if the string area is already
> word-aligned.
>
> This problem, combined with the bad buffer sizing for the serverDomain
> string can cause memory corruption. The bad alignment can make it so
> that the alignment of the characters is off. This can make them
> translate to characters that are greater than 2 bytes each. If this
> happens we can overflow the allocation.
>
> Fix this by fixing the alignment in CIFS_SessSetup instead so we can
> verify it against the head of the response. Also, clean up the
> workaround for improperly terminated strings by checking for a
> odd-length unicode buffers and then forcibly terminating them.
>
> Finally, resize the buffer for serverDomain. Now that we've fixed
> the alignment, it's probably fine, but a malicious server could
> overflow it.
>
> A better solution for handling these strings is still needed, but
> this should be a suitable bandaid.
>
> Signed-off-by: Jeff Layton <jlayton at redhat.com>
> ---
>  fs/cifs/sess.c |   44 +++++++++++++++++++++++---------------------
>  1 files changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> index 5c68b42..70d04d0 100644
> --- a/fs/cifs/sess.c
> +++ b/fs/cifs/sess.c
> @@ -285,27 +285,26 @@ static int decode_unicode_ssetup(char **pbcc_area, int bleft,
>        int words_left, len;
>        char *data = *pbcc_area;
>
> -
> -
>        cFYI(1, ("bleft %d", bleft));
>
> -
> -       /* SMB header is unaligned, so cifs servers word align start of
> -          Unicode strings */
> -       data++;
> -       bleft--; /* Windows servers do not always double null terminate
> -                   their final Unicode string - in which case we
> -                   now will not attempt to decode the byte of junk
> -                   which follows it */
> +       /*
> +        * Windows servers do not always double null terminate their final
> +        * Unicode string. Check to see if there are an uneven number of bytes
> +        * left. If so, then add an extra NULL pad byte to the end of the
> +        * response.
> +        *
> +        * See section 2.7.2 in "Implementing CIFS" for details
> +        */
> +       if (bleft % 2) {
> +               data[bleft] = 0;
> +               ++bleft;
> +       }
>
>        words_left = bleft / 2;
>
>        /* save off server operating system */
>        len = UniStrnlen((wchar_t *) data, words_left);
>
> -/* We look for obvious messed up bcc or strings in response so we do not go off
> -   the end since (at least) WIN2K and Windows XP have a major bug in not null
> -   terminating last Unicode string in response  */
>        if (len >= words_left)
>                return rc;
>
> @@ -343,13 +342,10 @@ static int decode_unicode_ssetup(char **pbcc_area, int bleft,
>                return rc;
>
>        kfree(ses->serverDomain);
> -       ses->serverDomain = kzalloc(2 * (len + 1), GFP_KERNEL); /* BB FIXME wrong length */
> -       if (ses->serverDomain != NULL) {
> +       ses->serverDomain = kzalloc((4 * len) + 2, GFP_KERNEL);
> +       if (ses->serverDomain != NULL)
>                cifs_strfromUCS_le(ses->serverDomain, (__le16 *)data, len,
>                                   nls_cp);
> -               ses->serverDomain[2*len] = 0;
> -               ses->serverDomain[(2*len) + 1] = 0;
> -       }
>        data += 2 * (len + 1);
>        words_left -= len + 1;
>
> @@ -702,12 +698,18 @@ CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses, int first_time,
>        }
>
>        /* BB check if Unicode and decode strings */
> -       if (smb_buf->Flags2 & SMBFLG2_UNICODE)
> +       if (smb_buf->Flags2 & SMBFLG2_UNICODE) {
> +               /* unicode string area must be word-aligned */
> +               if (((unsigned long) bcc_ptr - (unsigned long) smb_buf) % 2) {
> +                       ++bcc_ptr;
> +                       --bytes_remaining;
> +               }
>                rc = decode_unicode_ssetup(&bcc_ptr, bytes_remaining,
> -                                                  ses, nls_cp);
> -       else
> +                                          ses, nls_cp);
> +       } else {
>                rc = decode_ascii_ssetup(&bcc_ptr, bytes_remaining,
>                                         ses, nls_cp);
> +       }
>
>  ssetup_exit:
>        if (spnego_key) {
> --
> 1.6.0.6
>
>



-- 
Thanks,

Steve