From: Jeff Layton <jlayton@redhat.com> Date: Fri, 17 Apr 2009 06:48:04 -0400 Subject: [fs] cifs: unicode alignment and buffer sizing problems Message-id: 1239965284-29229-1-git-send-email-jlayton@redhat.com O-Subject: [RHEL5.4 PATCH] BZ#494280: cifs: fix unicode alignment and buffer sizing problems Bugzilla: 494280 RH-Acked-by: Peter Staubach <staubach@redhat.com> CVE: CVE-2009-1439 This patch depends on the larger CIFS update for 5.4 that was recently posted. At session setup and tree connect, CIFS will save off some strings that it gets from the server during the session setup and tree connect. There are several places in this code that size the buffers for these strings too small. The server will send strings in UTF-16 format (all chars are 2 bytes), and these can translate to being up to 4 bytes in UTF-8. Part of the patch just ensures that we size the buffer appropriately for this -- 4 times the number of wide chars, plus 2 bytes for the NULL terminator. The string area of these responses is also word-aligned. The current scheme that CIFS uses to compensate for the buffer alignment assumes that the buffer will always be preceded by a 1 byte pad, but that's not always the case. This patch fixes this by changing this to properly check for the alignment by making sure that we start on a word boundary. It also fixes a workaround for buggy windows servers that don't properly NULL terminate the last string in the packet, and cleans up some unneeded NULL termination. Finally, it adds some "cFYI" debug printks that that show these strings when a debug switch is set. This may help detect problems in this area in the future. The patch is a rollup of 5 patches from upstream. Some of these are not yet in Linus' tree but are in Steve French's tree and should make their way to Linus in the near future. This patch was tested by mounting and unmounting different servers with the cFYI switch enabled, and making sure that the resulting strings looked correct. Signed-off-by: Jeff Layton <jlayton@redhat.com> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index ffa57e6..dc4417d 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -3750,16 +3750,15 @@ CIFSTCon(unsigned int xid, struct cifsSesInfo *ses, BCC(smb_buffer_response)) { kfree(tcon->nativeFileSystem); tcon->nativeFileSystem = - kzalloc(2*(length + 1), GFP_KERNEL); - if (tcon->nativeFileSystem) + kzalloc((4 * length) + 2, GFP_KERNEL); + if (tcon->nativeFileSystem) { cifs_strfromUCS_le( tcon->nativeFileSystem, (__le16 *) bcc_ptr, length, nls_codepage); - bcc_ptr += 2 * length; - bcc_ptr[0] = 0; /* null terminate the string */ - bcc_ptr[1] = 0; - bcc_ptr += 2; + cFYI(1, ("nativeFileSystem=%s", + tcon->nativeFileSystem)); + } } /* else do not bother copying these information fields*/ } else { diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c index 3fd5519..5a4cd21 100644 --- a/fs/cifs/sess.c +++ b/fs/cifs/sess.c @@ -294,35 +294,36 @@ 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; kfree(ses->serverOS); /* UTF-8 string will not grow more than four times as big as UCS-16 */ ses->serverOS = kzalloc((4 * len) + 2 /* trailing null */, GFP_KERNEL); - if (ses->serverOS != NULL) + if (ses->serverOS != NULL) { cifs_strfromUCS_le(ses->serverOS, (__le16 *)data, len, nls_cp); + cFYI(1, ("serverOS=%s", ses->serverOS)); + } data += 2 * (len + 1); words_left -= len + 1; @@ -337,6 +338,7 @@ static int decode_unicode_ssetup(char **pbcc_area, int bleft, if (ses->serverNOS != NULL) { cifs_strfromUCS_le(ses->serverNOS, (__le16 *)data, len, nls_cp); + cFYI(1, ("serverNOS=%s", ses->serverNOS)); if (strncmp(ses->serverNOS, "NT LAN Manager 4", 16) == 0) { cFYI(1, ("NT4 server")); ses->flags |= CIFS_SES_NT4; @@ -352,12 +354,11 @@ 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 */ + 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; + cFYI(1, ("serverDomain=%s", ses->serverDomain)); } data += 2 * (len + 1); words_left -= len + 1; @@ -711,12 +712,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) {