* [PATCH 1/2] cifs: use CIFS_MAX_DOMAINNAME_LEN when converting the domain name
@ 2016-05-26 9:52 Jerome Marchand
2016-05-26 9:52 ` [PATCH 2/2] cifs: dynamic allocation of ntlmssp blob Jerome Marchand
0 siblings, 1 reply; 4+ messages in thread
From: Jerome Marchand @ 2016-05-26 9:52 UTC (permalink / raw)
To: Steve French
Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA,
samba-technical-w/Ol4Ecudpl8XjKLYN78aQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Currently in build_ntlmssp_auth_blob(), when converting the domain
name to UTF16, CIFS_MAX_USERNAME_LEN limit is used. It should be
CIFS_MAX_DOMAINNAME_LEN. This patch fixes this.
Signed-off-by: Jerome Marchand <jmarchan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
fs/cifs/sess.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index af0ec2d..c3d086e 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -430,7 +430,7 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
} else {
int len;
len = cifs_strtoUTF16((__le16 *)tmp, ses->domainName,
- CIFS_MAX_USERNAME_LEN, nls_cp);
+ CIFS_MAX_DOMAINNAME_LEN, nls_cp);
len *= 2; /* unicode is 2 bytes each */
sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - pbuffer);
sec_blob->DomainName.Length = cpu_to_le16(len);
--
2.5.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] cifs: dynamic allocation of ntlmssp blob
2016-05-26 9:52 [PATCH 1/2] cifs: use CIFS_MAX_DOMAINNAME_LEN when converting the domain name Jerome Marchand
@ 2016-05-26 9:52 ` Jerome Marchand
[not found] ` <1464256345-26131-2-git-send-email-jmarchan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Jerome Marchand @ 2016-05-26 9:52 UTC (permalink / raw)
To: Steve French; +Cc: linux-cifs, samba-technical, linux-kernel
In sess_auth_rawntlmssp_authenticate(), the ntlmssp blob is allocated
statically and its size is an "empirical" 5*sizeof(struct
_AUTHENTICATE_MESSAGE) (320B on x86_64). I don't know where this value
comes from or if it was ever appropriate, but it is currently
insufficient: the user and domain name in UTF16 could take 1kB by
themselves. Because of that, build_ntlmssp_auth_blob() might corrupt
memory (out-of-bounds write). The size of ntlmssp_blob in
SMB2_sess_setup() is too small too (sizeof(struct _NEGOTIATE_MESSAGE)
+ 500).
This patch allocates the blob dynamically in
build_ntlmssp_auth_blob().
Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
fs/cifs/ntlmssp.h | 2 +-
fs/cifs/sess.c | 76 ++++++++++++++++++++++++++++++-------------------------
fs/cifs/smb2pdu.c | 10 ++------
3 files changed, 45 insertions(+), 43 deletions(-)
diff --git a/fs/cifs/ntlmssp.h b/fs/cifs/ntlmssp.h
index 848249f..3079b38 100644
--- a/fs/cifs/ntlmssp.h
+++ b/fs/cifs/ntlmssp.h
@@ -133,6 +133,6 @@ typedef struct _AUTHENTICATE_MESSAGE {
int decode_ntlmssp_challenge(char *bcc_ptr, int blob_len, struct cifs_ses *ses);
void build_ntlmssp_negotiate_blob(unsigned char *pbuffer, struct cifs_ses *ses);
-int build_ntlmssp_auth_blob(unsigned char *pbuffer, u16 *buflen,
+int build_ntlmssp_auth_blob(unsigned char **pbuffer, u16 *buflen,
struct cifs_ses *ses,
const struct nls_table *nls_cp);
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index c3d086e..463bed7 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -364,19 +364,43 @@ void build_ntlmssp_negotiate_blob(unsigned char *pbuffer,
sec_blob->DomainName.MaximumLength = 0;
}
-/* We do not malloc the blob, it is passed in pbuffer, because its
- maximum possible size is fixed and small, making this approach cleaner.
- This function returns the length of the data in the blob */
-int build_ntlmssp_auth_blob(unsigned char *pbuffer,
+int size_of_ntlmssp_blob(struct cifs_ses *ses)
+{
+ int sz = sizeof(AUTHENTICATE_MESSAGE) + ses->auth_key.len
+ - CIFS_SESS_KEY_SIZE + CIFS_CPHTXT_SIZE + 2;
+
+ if (ses->domainName)
+ sz += 2 * strnlen(ses->domainName, CIFS_MAX_DOMAINNAME_LEN);
+ else
+ sz += 2;
+
+ if (ses->user_name)
+ sz += 2 * strnlen(ses->user_name, CIFS_MAX_USERNAME_LEN);
+ else
+ sz += 2;
+
+ return sz;
+}
+
+int build_ntlmssp_auth_blob(unsigned char **pbuffer,
u16 *buflen,
struct cifs_ses *ses,
const struct nls_table *nls_cp)
{
int rc;
- AUTHENTICATE_MESSAGE *sec_blob = (AUTHENTICATE_MESSAGE *)pbuffer;
+ AUTHENTICATE_MESSAGE *sec_blob;
__u32 flags;
unsigned char *tmp;
+ rc = setup_ntlmv2_rsp(ses, nls_cp);
+ if (rc) {
+ cifs_dbg(VFS, "Error %d during NTLMSSP authentication\n", rc);
+ *buflen = 0;
+ goto setup_ntlmv2_ret;
+ }
+ *pbuffer = kmalloc(size_of_ntlmssp_blob(ses), GFP_KERNEL);
+ sec_blob = (AUTHENTICATE_MESSAGE *)*pbuffer;
+
memcpy(sec_blob->Signature, NTLMSSP_SIGNATURE, 8);
sec_blob->MessageType = NtLmAuthenticate;
@@ -391,7 +415,7 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
flags |= NTLMSSP_NEGOTIATE_KEY_XCH;
}
- tmp = pbuffer + sizeof(AUTHENTICATE_MESSAGE);
+ tmp = *pbuffer + sizeof(AUTHENTICATE_MESSAGE);
sec_blob->NegotiateFlags = cpu_to_le32(flags);
sec_blob->LmChallengeResponse.BufferOffset =
@@ -399,13 +423,9 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
sec_blob->LmChallengeResponse.Length = 0;
sec_blob->LmChallengeResponse.MaximumLength = 0;
- sec_blob->NtChallengeResponse.BufferOffset = cpu_to_le32(tmp - pbuffer);
+ sec_blob->NtChallengeResponse.BufferOffset =
+ cpu_to_le32(tmp - *pbuffer);
if (ses->user_name != NULL) {
- rc = setup_ntlmv2_rsp(ses, nls_cp);
- if (rc) {
- cifs_dbg(VFS, "Error %d during NTLMSSP authentication\n", rc);
- goto setup_ntlmv2_ret;
- }
memcpy(tmp, ses->auth_key.response + CIFS_SESS_KEY_SIZE,
ses->auth_key.len - CIFS_SESS_KEY_SIZE);
tmp += ses->auth_key.len - CIFS_SESS_KEY_SIZE;
@@ -423,7 +443,7 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
}
if (ses->domainName == NULL) {
- sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - pbuffer);
+ sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
sec_blob->DomainName.Length = 0;
sec_blob->DomainName.MaximumLength = 0;
tmp += 2;
@@ -432,14 +452,14 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
len = cifs_strtoUTF16((__le16 *)tmp, ses->domainName,
CIFS_MAX_DOMAINNAME_LEN, nls_cp);
len *= 2; /* unicode is 2 bytes each */
- sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - pbuffer);
+ sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
sec_blob->DomainName.Length = cpu_to_le16(len);
sec_blob->DomainName.MaximumLength = cpu_to_le16(len);
tmp += len;
}
if (ses->user_name == NULL) {
- sec_blob->UserName.BufferOffset = cpu_to_le32(tmp - pbuffer);
+ sec_blob->UserName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
sec_blob->UserName.Length = 0;
sec_blob->UserName.MaximumLength = 0;
tmp += 2;
@@ -448,13 +468,13 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
len = cifs_strtoUTF16((__le16 *)tmp, ses->user_name,
CIFS_MAX_USERNAME_LEN, nls_cp);
len *= 2; /* unicode is 2 bytes each */
- sec_blob->UserName.BufferOffset = cpu_to_le32(tmp - pbuffer);
+ sec_blob->UserName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
sec_blob->UserName.Length = cpu_to_le16(len);
sec_blob->UserName.MaximumLength = cpu_to_le16(len);
tmp += len;
}
- sec_blob->WorkstationName.BufferOffset = cpu_to_le32(tmp - pbuffer);
+ sec_blob->WorkstationName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
sec_blob->WorkstationName.Length = 0;
sec_blob->WorkstationName.MaximumLength = 0;
tmp += 2;
@@ -463,19 +483,19 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
(ses->ntlmssp->server_flags & NTLMSSP_NEGOTIATE_EXTENDED_SEC))
&& !calc_seckey(ses)) {
memcpy(tmp, ses->ntlmssp->ciphertext, CIFS_CPHTXT_SIZE);
- sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - pbuffer);
+ sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - *pbuffer);
sec_blob->SessionKey.Length = cpu_to_le16(CIFS_CPHTXT_SIZE);
sec_blob->SessionKey.MaximumLength =
cpu_to_le16(CIFS_CPHTXT_SIZE);
tmp += CIFS_CPHTXT_SIZE;
} else {
- sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - pbuffer);
+ sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - *pbuffer);
sec_blob->SessionKey.Length = 0;
sec_blob->SessionKey.MaximumLength = 0;
}
+ *buflen = tmp - *pbuffer;
setup_ntlmv2_ret:
- *buflen = tmp - pbuffer;
return rc;
}
@@ -1266,7 +1286,7 @@ sess_auth_rawntlmssp_authenticate(struct sess_data *sess_data)
struct cifs_ses *ses = sess_data->ses;
__u16 bytes_remaining;
char *bcc_ptr;
- char *ntlmsspblob = NULL;
+ unsigned char *ntlmsspblob = NULL;
u16 blob_len;
cifs_dbg(FYI, "rawntlmssp session setup authenticate phase\n");
@@ -1279,19 +1299,7 @@ sess_auth_rawntlmssp_authenticate(struct sess_data *sess_data)
/* Build security blob before we assemble the request */
pSMB = (SESSION_SETUP_ANDX *)sess_data->iov[0].iov_base;
smb_buf = (struct smb_hdr *)pSMB;
- /*
- * 5 is an empirical value, large enough to hold
- * authenticate message plus max 10 of av paris,
- * domain, user, workstation names, flags, etc.
- */
- ntlmsspblob = kzalloc(5*sizeof(struct _AUTHENTICATE_MESSAGE),
- GFP_KERNEL);
- if (!ntlmsspblob) {
- rc = -ENOMEM;
- goto out;
- }
-
- rc = build_ntlmssp_auth_blob(ntlmsspblob,
+ rc = build_ntlmssp_auth_blob(&ntlmsspblob,
&blob_len, ses, sess_data->nls_cp);
if (rc)
goto out_free_ntlmsspblob;
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 8f38e33..c3e61a7 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -588,7 +588,7 @@ SMB2_sess_setup(const unsigned int xid, struct cifs_ses *ses,
u16 blob_length = 0;
struct key *spnego_key = NULL;
char *security_blob = NULL;
- char *ntlmssp_blob = NULL;
+ unsigned char *ntlmssp_blob = NULL;
bool use_spnego = false; /* else use raw ntlmssp */
cifs_dbg(FYI, "Session Setup\n");
@@ -713,13 +713,7 @@ ssetup_ntlmssp_authenticate:
iov[1].iov_len = blob_length;
} else if (phase == NtLmAuthenticate) {
req->hdr.SessionId = ses->Suid;
- ntlmssp_blob = kzalloc(sizeof(struct _NEGOTIATE_MESSAGE) + 500,
- GFP_KERNEL);
- if (ntlmssp_blob == NULL) {
- rc = -ENOMEM;
- goto ssetup_exit;
- }
- rc = build_ntlmssp_auth_blob(ntlmssp_blob, &blob_length, ses,
+ rc = build_ntlmssp_auth_blob(&ntlmssp_blob, &blob_length, ses,
nls_cp);
if (rc) {
cifs_dbg(FYI, "build_ntlmssp_auth_blob failed %d\n",
--
2.5.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] cifs: dynamic allocation of ntlmssp blob
[not found] ` <1464256345-26131-2-git-send-email-jmarchan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2016-05-26 18:30 ` Steve French
[not found] ` <CAH2r5mtCoqUWfRz9CF5jg6QdFy_sFgd5-2ZomLs_xaDDC_uydg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Steve French @ 2016-05-26 18:30 UTC (permalink / raw)
To: Jerome Marchand
Cc: Steve French, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
samba-technical, LKML
Your patch makes code a little clearer, so I don't mind merging it,
but the two values are the same so this patch should have no effect.
On Thu, May 26, 2016 at 4:52 AM, Jerome Marchand <jmarchan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> In sess_auth_rawntlmssp_authenticate(), the ntlmssp blob is allocated
> statically and its size is an "empirical" 5*sizeof(struct
> _AUTHENTICATE_MESSAGE) (320B on x86_64). I don't know where this value
> comes from or if it was ever appropriate, but it is currently
> insufficient: the user and domain name in UTF16 could take 1kB by
> themselves. Because of that, build_ntlmssp_auth_blob() might corrupt
> memory (out-of-bounds write). The size of ntlmssp_blob in
> SMB2_sess_setup() is too small too (sizeof(struct _NEGOTIATE_MESSAGE)
> + 500).
>
> This patch allocates the blob dynamically in
> build_ntlmssp_auth_blob().
>
> Signed-off-by: Jerome Marchand <jmarchan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> fs/cifs/ntlmssp.h | 2 +-
> fs/cifs/sess.c | 76 ++++++++++++++++++++++++++++++-------------------------
> fs/cifs/smb2pdu.c | 10 ++------
> 3 files changed, 45 insertions(+), 43 deletions(-)
>
> diff --git a/fs/cifs/ntlmssp.h b/fs/cifs/ntlmssp.h
> index 848249f..3079b38 100644
> --- a/fs/cifs/ntlmssp.h
> +++ b/fs/cifs/ntlmssp.h
> @@ -133,6 +133,6 @@ typedef struct _AUTHENTICATE_MESSAGE {
>
> int decode_ntlmssp_challenge(char *bcc_ptr, int blob_len, struct cifs_ses *ses);
> void build_ntlmssp_negotiate_blob(unsigned char *pbuffer, struct cifs_ses *ses);
> -int build_ntlmssp_auth_blob(unsigned char *pbuffer, u16 *buflen,
> +int build_ntlmssp_auth_blob(unsigned char **pbuffer, u16 *buflen,
> struct cifs_ses *ses,
> const struct nls_table *nls_cp);
> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> index c3d086e..463bed7 100644
> --- a/fs/cifs/sess.c
> +++ b/fs/cifs/sess.c
> @@ -364,19 +364,43 @@ void build_ntlmssp_negotiate_blob(unsigned char *pbuffer,
> sec_blob->DomainName.MaximumLength = 0;
> }
>
> -/* We do not malloc the blob, it is passed in pbuffer, because its
> - maximum possible size is fixed and small, making this approach cleaner.
> - This function returns the length of the data in the blob */
> -int build_ntlmssp_auth_blob(unsigned char *pbuffer,
> +int size_of_ntlmssp_blob(struct cifs_ses *ses)
> +{
> + int sz = sizeof(AUTHENTICATE_MESSAGE) + ses->auth_key.len
> + - CIFS_SESS_KEY_SIZE + CIFS_CPHTXT_SIZE + 2;
> +
> + if (ses->domainName)
> + sz += 2 * strnlen(ses->domainName, CIFS_MAX_DOMAINNAME_LEN);
> + else
> + sz += 2;
> +
> + if (ses->user_name)
> + sz += 2 * strnlen(ses->user_name, CIFS_MAX_USERNAME_LEN);
> + else
> + sz += 2;
> +
> + return sz;
> +}
> +
> +int build_ntlmssp_auth_blob(unsigned char **pbuffer,
> u16 *buflen,
> struct cifs_ses *ses,
> const struct nls_table *nls_cp)
> {
> int rc;
> - AUTHENTICATE_MESSAGE *sec_blob = (AUTHENTICATE_MESSAGE *)pbuffer;
> + AUTHENTICATE_MESSAGE *sec_blob;
> __u32 flags;
> unsigned char *tmp;
>
> + rc = setup_ntlmv2_rsp(ses, nls_cp);
> + if (rc) {
> + cifs_dbg(VFS, "Error %d during NTLMSSP authentication\n", rc);
> + *buflen = 0;
> + goto setup_ntlmv2_ret;
> + }
> + *pbuffer = kmalloc(size_of_ntlmssp_blob(ses), GFP_KERNEL);
> + sec_blob = (AUTHENTICATE_MESSAGE *)*pbuffer;
> +
> memcpy(sec_blob->Signature, NTLMSSP_SIGNATURE, 8);
> sec_blob->MessageType = NtLmAuthenticate;
>
> @@ -391,7 +415,7 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
> flags |= NTLMSSP_NEGOTIATE_KEY_XCH;
> }
>
> - tmp = pbuffer + sizeof(AUTHENTICATE_MESSAGE);
> + tmp = *pbuffer + sizeof(AUTHENTICATE_MESSAGE);
> sec_blob->NegotiateFlags = cpu_to_le32(flags);
>
> sec_blob->LmChallengeResponse.BufferOffset =
> @@ -399,13 +423,9 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
> sec_blob->LmChallengeResponse.Length = 0;
> sec_blob->LmChallengeResponse.MaximumLength = 0;
>
> - sec_blob->NtChallengeResponse.BufferOffset = cpu_to_le32(tmp - pbuffer);
> + sec_blob->NtChallengeResponse.BufferOffset =
> + cpu_to_le32(tmp - *pbuffer);
> if (ses->user_name != NULL) {
> - rc = setup_ntlmv2_rsp(ses, nls_cp);
> - if (rc) {
> - cifs_dbg(VFS, "Error %d during NTLMSSP authentication\n", rc);
> - goto setup_ntlmv2_ret;
> - }
> memcpy(tmp, ses->auth_key.response + CIFS_SESS_KEY_SIZE,
> ses->auth_key.len - CIFS_SESS_KEY_SIZE);
> tmp += ses->auth_key.len - CIFS_SESS_KEY_SIZE;
> @@ -423,7 +443,7 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
> }
>
> if (ses->domainName == NULL) {
> - sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - pbuffer);
> + sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
> sec_blob->DomainName.Length = 0;
> sec_blob->DomainName.MaximumLength = 0;
> tmp += 2;
> @@ -432,14 +452,14 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
> len = cifs_strtoUTF16((__le16 *)tmp, ses->domainName,
> CIFS_MAX_DOMAINNAME_LEN, nls_cp);
> len *= 2; /* unicode is 2 bytes each */
> - sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - pbuffer);
> + sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
> sec_blob->DomainName.Length = cpu_to_le16(len);
> sec_blob->DomainName.MaximumLength = cpu_to_le16(len);
> tmp += len;
> }
>
> if (ses->user_name == NULL) {
> - sec_blob->UserName.BufferOffset = cpu_to_le32(tmp - pbuffer);
> + sec_blob->UserName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
> sec_blob->UserName.Length = 0;
> sec_blob->UserName.MaximumLength = 0;
> tmp += 2;
> @@ -448,13 +468,13 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
> len = cifs_strtoUTF16((__le16 *)tmp, ses->user_name,
> CIFS_MAX_USERNAME_LEN, nls_cp);
> len *= 2; /* unicode is 2 bytes each */
> - sec_blob->UserName.BufferOffset = cpu_to_le32(tmp - pbuffer);
> + sec_blob->UserName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
> sec_blob->UserName.Length = cpu_to_le16(len);
> sec_blob->UserName.MaximumLength = cpu_to_le16(len);
> tmp += len;
> }
>
> - sec_blob->WorkstationName.BufferOffset = cpu_to_le32(tmp - pbuffer);
> + sec_blob->WorkstationName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
> sec_blob->WorkstationName.Length = 0;
> sec_blob->WorkstationName.MaximumLength = 0;
> tmp += 2;
> @@ -463,19 +483,19 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
> (ses->ntlmssp->server_flags & NTLMSSP_NEGOTIATE_EXTENDED_SEC))
> && !calc_seckey(ses)) {
> memcpy(tmp, ses->ntlmssp->ciphertext, CIFS_CPHTXT_SIZE);
> - sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - pbuffer);
> + sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - *pbuffer);
> sec_blob->SessionKey.Length = cpu_to_le16(CIFS_CPHTXT_SIZE);
> sec_blob->SessionKey.MaximumLength =
> cpu_to_le16(CIFS_CPHTXT_SIZE);
> tmp += CIFS_CPHTXT_SIZE;
> } else {
> - sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - pbuffer);
> + sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - *pbuffer);
> sec_blob->SessionKey.Length = 0;
> sec_blob->SessionKey.MaximumLength = 0;
> }
>
> + *buflen = tmp - *pbuffer;
> setup_ntlmv2_ret:
> - *buflen = tmp - pbuffer;
> return rc;
> }
>
> @@ -1266,7 +1286,7 @@ sess_auth_rawntlmssp_authenticate(struct sess_data *sess_data)
> struct cifs_ses *ses = sess_data->ses;
> __u16 bytes_remaining;
> char *bcc_ptr;
> - char *ntlmsspblob = NULL;
> + unsigned char *ntlmsspblob = NULL;
> u16 blob_len;
>
> cifs_dbg(FYI, "rawntlmssp session setup authenticate phase\n");
> @@ -1279,19 +1299,7 @@ sess_auth_rawntlmssp_authenticate(struct sess_data *sess_data)
> /* Build security blob before we assemble the request */
> pSMB = (SESSION_SETUP_ANDX *)sess_data->iov[0].iov_base;
> smb_buf = (struct smb_hdr *)pSMB;
> - /*
> - * 5 is an empirical value, large enough to hold
> - * authenticate message plus max 10 of av paris,
> - * domain, user, workstation names, flags, etc.
> - */
> - ntlmsspblob = kzalloc(5*sizeof(struct _AUTHENTICATE_MESSAGE),
> - GFP_KERNEL);
> - if (!ntlmsspblob) {
> - rc = -ENOMEM;
> - goto out;
> - }
> -
> - rc = build_ntlmssp_auth_blob(ntlmsspblob,
> + rc = build_ntlmssp_auth_blob(&ntlmsspblob,
> &blob_len, ses, sess_data->nls_cp);
> if (rc)
> goto out_free_ntlmsspblob;
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 8f38e33..c3e61a7 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -588,7 +588,7 @@ SMB2_sess_setup(const unsigned int xid, struct cifs_ses *ses,
> u16 blob_length = 0;
> struct key *spnego_key = NULL;
> char *security_blob = NULL;
> - char *ntlmssp_blob = NULL;
> + unsigned char *ntlmssp_blob = NULL;
> bool use_spnego = false; /* else use raw ntlmssp */
>
> cifs_dbg(FYI, "Session Setup\n");
> @@ -713,13 +713,7 @@ ssetup_ntlmssp_authenticate:
> iov[1].iov_len = blob_length;
> } else if (phase == NtLmAuthenticate) {
> req->hdr.SessionId = ses->Suid;
> - ntlmssp_blob = kzalloc(sizeof(struct _NEGOTIATE_MESSAGE) + 500,
> - GFP_KERNEL);
> - if (ntlmssp_blob == NULL) {
> - rc = -ENOMEM;
> - goto ssetup_exit;
> - }
> - rc = build_ntlmssp_auth_blob(ntlmssp_blob, &blob_length, ses,
> + rc = build_ntlmssp_auth_blob(&ntlmssp_blob, &blob_length, ses,
> nls_cp);
> if (rc) {
> cifs_dbg(FYI, "build_ntlmssp_auth_blob failed %d\n",
> --
> 2.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] cifs: dynamic allocation of ntlmssp blob
[not found] ` <CAH2r5mtCoqUWfRz9CF5jg6QdFy_sFgd5-2ZomLs_xaDDC_uydg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-05-26 18:31 ` Steve French
0 siblings, 0 replies; 4+ messages in thread
From: Steve French @ 2016-05-26 18:31 UTC (permalink / raw)
To: Jerome Marchand
Cc: Steve French, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
samba-technical, LKML
Sorry about the previous note, what I meant was that your first patch
makes the code a little clearer (this patch obviously fixes a bug).
On Thu, May 26, 2016 at 1:30 PM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Your patch makes code a little clearer, so I don't mind merging it,
> but the two values are the same so this patch should have no effect.
>
> On Thu, May 26, 2016 at 4:52 AM, Jerome Marchand <jmarchan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> In sess_auth_rawntlmssp_authenticate(), the ntlmssp blob is allocated
>> statically and its size is an "empirical" 5*sizeof(struct
>> _AUTHENTICATE_MESSAGE) (320B on x86_64). I don't know where this value
>> comes from or if it was ever appropriate, but it is currently
>> insufficient: the user and domain name in UTF16 could take 1kB by
>> themselves. Because of that, build_ntlmssp_auth_blob() might corrupt
>> memory (out-of-bounds write). The size of ntlmssp_blob in
>> SMB2_sess_setup() is too small too (sizeof(struct _NEGOTIATE_MESSAGE)
>> + 500).
>>
>> This patch allocates the blob dynamically in
>> build_ntlmssp_auth_blob().
>>
>> Signed-off-by: Jerome Marchand <jmarchan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> fs/cifs/ntlmssp.h | 2 +-
>> fs/cifs/sess.c | 76 ++++++++++++++++++++++++++++++-------------------------
>> fs/cifs/smb2pdu.c | 10 ++------
>> 3 files changed, 45 insertions(+), 43 deletions(-)
>>
>> diff --git a/fs/cifs/ntlmssp.h b/fs/cifs/ntlmssp.h
>> index 848249f..3079b38 100644
>> --- a/fs/cifs/ntlmssp.h
>> +++ b/fs/cifs/ntlmssp.h
>> @@ -133,6 +133,6 @@ typedef struct _AUTHENTICATE_MESSAGE {
>>
>> int decode_ntlmssp_challenge(char *bcc_ptr, int blob_len, struct cifs_ses *ses);
>> void build_ntlmssp_negotiate_blob(unsigned char *pbuffer, struct cifs_ses *ses);
>> -int build_ntlmssp_auth_blob(unsigned char *pbuffer, u16 *buflen,
>> +int build_ntlmssp_auth_blob(unsigned char **pbuffer, u16 *buflen,
>> struct cifs_ses *ses,
>> const struct nls_table *nls_cp);
>> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
>> index c3d086e..463bed7 100644
>> --- a/fs/cifs/sess.c
>> +++ b/fs/cifs/sess.c
>> @@ -364,19 +364,43 @@ void build_ntlmssp_negotiate_blob(unsigned char *pbuffer,
>> sec_blob->DomainName.MaximumLength = 0;
>> }
>>
>> -/* We do not malloc the blob, it is passed in pbuffer, because its
>> - maximum possible size is fixed and small, making this approach cleaner.
>> - This function returns the length of the data in the blob */
>> -int build_ntlmssp_auth_blob(unsigned char *pbuffer,
>> +int size_of_ntlmssp_blob(struct cifs_ses *ses)
>> +{
>> + int sz = sizeof(AUTHENTICATE_MESSAGE) + ses->auth_key.len
>> + - CIFS_SESS_KEY_SIZE + CIFS_CPHTXT_SIZE + 2;
>> +
>> + if (ses->domainName)
>> + sz += 2 * strnlen(ses->domainName, CIFS_MAX_DOMAINNAME_LEN);
>> + else
>> + sz += 2;
>> +
>> + if (ses->user_name)
>> + sz += 2 * strnlen(ses->user_name, CIFS_MAX_USERNAME_LEN);
>> + else
>> + sz += 2;
>> +
>> + return sz;
>> +}
>> +
>> +int build_ntlmssp_auth_blob(unsigned char **pbuffer,
>> u16 *buflen,
>> struct cifs_ses *ses,
>> const struct nls_table *nls_cp)
>> {
>> int rc;
>> - AUTHENTICATE_MESSAGE *sec_blob = (AUTHENTICATE_MESSAGE *)pbuffer;
>> + AUTHENTICATE_MESSAGE *sec_blob;
>> __u32 flags;
>> unsigned char *tmp;
>>
>> + rc = setup_ntlmv2_rsp(ses, nls_cp);
>> + if (rc) {
>> + cifs_dbg(VFS, "Error %d during NTLMSSP authentication\n", rc);
>> + *buflen = 0;
>> + goto setup_ntlmv2_ret;
>> + }
>> + *pbuffer = kmalloc(size_of_ntlmssp_blob(ses), GFP_KERNEL);
>> + sec_blob = (AUTHENTICATE_MESSAGE *)*pbuffer;
>> +
>> memcpy(sec_blob->Signature, NTLMSSP_SIGNATURE, 8);
>> sec_blob->MessageType = NtLmAuthenticate;
>>
>> @@ -391,7 +415,7 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
>> flags |= NTLMSSP_NEGOTIATE_KEY_XCH;
>> }
>>
>> - tmp = pbuffer + sizeof(AUTHENTICATE_MESSAGE);
>> + tmp = *pbuffer + sizeof(AUTHENTICATE_MESSAGE);
>> sec_blob->NegotiateFlags = cpu_to_le32(flags);
>>
>> sec_blob->LmChallengeResponse.BufferOffset =
>> @@ -399,13 +423,9 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
>> sec_blob->LmChallengeResponse.Length = 0;
>> sec_blob->LmChallengeResponse.MaximumLength = 0;
>>
>> - sec_blob->NtChallengeResponse.BufferOffset = cpu_to_le32(tmp - pbuffer);
>> + sec_blob->NtChallengeResponse.BufferOffset =
>> + cpu_to_le32(tmp - *pbuffer);
>> if (ses->user_name != NULL) {
>> - rc = setup_ntlmv2_rsp(ses, nls_cp);
>> - if (rc) {
>> - cifs_dbg(VFS, "Error %d during NTLMSSP authentication\n", rc);
>> - goto setup_ntlmv2_ret;
>> - }
>> memcpy(tmp, ses->auth_key.response + CIFS_SESS_KEY_SIZE,
>> ses->auth_key.len - CIFS_SESS_KEY_SIZE);
>> tmp += ses->auth_key.len - CIFS_SESS_KEY_SIZE;
>> @@ -423,7 +443,7 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
>> }
>>
>> if (ses->domainName == NULL) {
>> - sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - pbuffer);
>> + sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
>> sec_blob->DomainName.Length = 0;
>> sec_blob->DomainName.MaximumLength = 0;
>> tmp += 2;
>> @@ -432,14 +452,14 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
>> len = cifs_strtoUTF16((__le16 *)tmp, ses->domainName,
>> CIFS_MAX_DOMAINNAME_LEN, nls_cp);
>> len *= 2; /* unicode is 2 bytes each */
>> - sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - pbuffer);
>> + sec_blob->DomainName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
>> sec_blob->DomainName.Length = cpu_to_le16(len);
>> sec_blob->DomainName.MaximumLength = cpu_to_le16(len);
>> tmp += len;
>> }
>>
>> if (ses->user_name == NULL) {
>> - sec_blob->UserName.BufferOffset = cpu_to_le32(tmp - pbuffer);
>> + sec_blob->UserName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
>> sec_blob->UserName.Length = 0;
>> sec_blob->UserName.MaximumLength = 0;
>> tmp += 2;
>> @@ -448,13 +468,13 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
>> len = cifs_strtoUTF16((__le16 *)tmp, ses->user_name,
>> CIFS_MAX_USERNAME_LEN, nls_cp);
>> len *= 2; /* unicode is 2 bytes each */
>> - sec_blob->UserName.BufferOffset = cpu_to_le32(tmp - pbuffer);
>> + sec_blob->UserName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
>> sec_blob->UserName.Length = cpu_to_le16(len);
>> sec_blob->UserName.MaximumLength = cpu_to_le16(len);
>> tmp += len;
>> }
>>
>> - sec_blob->WorkstationName.BufferOffset = cpu_to_le32(tmp - pbuffer);
>> + sec_blob->WorkstationName.BufferOffset = cpu_to_le32(tmp - *pbuffer);
>> sec_blob->WorkstationName.Length = 0;
>> sec_blob->WorkstationName.MaximumLength = 0;
>> tmp += 2;
>> @@ -463,19 +483,19 @@ int build_ntlmssp_auth_blob(unsigned char *pbuffer,
>> (ses->ntlmssp->server_flags & NTLMSSP_NEGOTIATE_EXTENDED_SEC))
>> && !calc_seckey(ses)) {
>> memcpy(tmp, ses->ntlmssp->ciphertext, CIFS_CPHTXT_SIZE);
>> - sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - pbuffer);
>> + sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - *pbuffer);
>> sec_blob->SessionKey.Length = cpu_to_le16(CIFS_CPHTXT_SIZE);
>> sec_blob->SessionKey.MaximumLength =
>> cpu_to_le16(CIFS_CPHTXT_SIZE);
>> tmp += CIFS_CPHTXT_SIZE;
>> } else {
>> - sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - pbuffer);
>> + sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - *pbuffer);
>> sec_blob->SessionKey.Length = 0;
>> sec_blob->SessionKey.MaximumLength = 0;
>> }
>>
>> + *buflen = tmp - *pbuffer;
>> setup_ntlmv2_ret:
>> - *buflen = tmp - pbuffer;
>> return rc;
>> }
>>
>> @@ -1266,7 +1286,7 @@ sess_auth_rawntlmssp_authenticate(struct sess_data *sess_data)
>> struct cifs_ses *ses = sess_data->ses;
>> __u16 bytes_remaining;
>> char *bcc_ptr;
>> - char *ntlmsspblob = NULL;
>> + unsigned char *ntlmsspblob = NULL;
>> u16 blob_len;
>>
>> cifs_dbg(FYI, "rawntlmssp session setup authenticate phase\n");
>> @@ -1279,19 +1299,7 @@ sess_auth_rawntlmssp_authenticate(struct sess_data *sess_data)
>> /* Build security blob before we assemble the request */
>> pSMB = (SESSION_SETUP_ANDX *)sess_data->iov[0].iov_base;
>> smb_buf = (struct smb_hdr *)pSMB;
>> - /*
>> - * 5 is an empirical value, large enough to hold
>> - * authenticate message plus max 10 of av paris,
>> - * domain, user, workstation names, flags, etc.
>> - */
>> - ntlmsspblob = kzalloc(5*sizeof(struct _AUTHENTICATE_MESSAGE),
>> - GFP_KERNEL);
>> - if (!ntlmsspblob) {
>> - rc = -ENOMEM;
>> - goto out;
>> - }
>> -
>> - rc = build_ntlmssp_auth_blob(ntlmsspblob,
>> + rc = build_ntlmssp_auth_blob(&ntlmsspblob,
>> &blob_len, ses, sess_data->nls_cp);
>> if (rc)
>> goto out_free_ntlmsspblob;
>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>> index 8f38e33..c3e61a7 100644
>> --- a/fs/cifs/smb2pdu.c
>> +++ b/fs/cifs/smb2pdu.c
>> @@ -588,7 +588,7 @@ SMB2_sess_setup(const unsigned int xid, struct cifs_ses *ses,
>> u16 blob_length = 0;
>> struct key *spnego_key = NULL;
>> char *security_blob = NULL;
>> - char *ntlmssp_blob = NULL;
>> + unsigned char *ntlmssp_blob = NULL;
>> bool use_spnego = false; /* else use raw ntlmssp */
>>
>> cifs_dbg(FYI, "Session Setup\n");
>> @@ -713,13 +713,7 @@ ssetup_ntlmssp_authenticate:
>> iov[1].iov_len = blob_length;
>> } else if (phase == NtLmAuthenticate) {
>> req->hdr.SessionId = ses->Suid;
>> - ntlmssp_blob = kzalloc(sizeof(struct _NEGOTIATE_MESSAGE) + 500,
>> - GFP_KERNEL);
>> - if (ntlmssp_blob == NULL) {
>> - rc = -ENOMEM;
>> - goto ssetup_exit;
>> - }
>> - rc = build_ntlmssp_auth_blob(ntlmssp_blob, &blob_length, ses,
>> + rc = build_ntlmssp_auth_blob(&ntlmssp_blob, &blob_length, ses,
>> nls_cp);
>> if (rc) {
>> cifs_dbg(FYI, "build_ntlmssp_auth_blob failed %d\n",
>> --
>> 2.5.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Thanks,
>
> Steve
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-05-26 18:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-26 9:52 [PATCH 1/2] cifs: use CIFS_MAX_DOMAINNAME_LEN when converting the domain name Jerome Marchand
2016-05-26 9:52 ` [PATCH 2/2] cifs: dynamic allocation of ntlmssp blob Jerome Marchand
[not found] ` <1464256345-26131-2-git-send-email-jmarchan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-05-26 18:30 ` Steve French
[not found] ` <CAH2r5mtCoqUWfRz9CF5jg6QdFy_sFgd5-2ZomLs_xaDDC_uydg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-05-26 18:31 ` Steve French
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).