On Tue, Sep 7, 2010 at 6:56 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> On Mon, 6 Sep 2010 22:34:57 -0500
> shirishpargaonkar@xxxxxxxxx wrote:
>
>> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>>
>> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> ---
>> fs/cifs/cifsencrypt.c | 68 +++++++++++++++++++++++++++++++++++++++++++++---
>> fs/cifs/cifspdu.h | 1 -
>> fs/cifs/cifsproto.h | 2 +-
>> fs/cifs/connect.c | 2 +
>> fs/cifs/sess.c | 24 ++++++++++++++++-
>> 5 files changed, 88 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
>> index fe1e4c9..5f0fc6b 100644
>> --- a/fs/cifs/cifsencrypt.c
>> +++ b/fs/cifs/cifsencrypt.c
>> @@ -27,6 +27,7 @@
>> #include "md5.h"
>> #include "cifs_unicode.h"
>> #include "cifsproto.h"
>> +#include "ntlmssp.h"
>> #include <linux/ctype.h>
>> #include <linux/random.h>
>>
>> @@ -263,6 +264,56 @@ void calc_lanman_hash(const char *password, const char *cryptkey, bool encrypt,
>> }
>> #endif /* CIFS_WEAK_PW_HASH */
>>
>> +static int
>> +find_domain_name(struct cifsSesInfo *ses)
>> +{
>> + int rc = 0;
>> + unsigned int attrsize;
>> + unsigned int type;
>> + unsigned char *blobptr;
>> + struct ntlmssp2_name *attrptr;
>> +
>> + if (ses->server->tiblob) {
>> + blobptr = ses->server->tiblob;
>> + attrptr = (struct ntlmssp2_name *) blobptr;
>> +
>> + while ((type = attrptr->type) != 0) {
>> + blobptr += 2; /* advance attr type */
>> + attrsize = attrptr->length;
>> + blobptr += 2; /* advance attr size */
>> + if (type == NTLMSSP_AV_NB_DOMAIN_NAME) {
>> + if (!ses->domainName) {
>> + ses->domainName =
>> + kmalloc(attrptr->length + 1,
>> + GFP_KERNEL);
>> + if (!ses->domainName)
>> + return -ENOMEM;
>> + cifs_from_ucs2(ses->domainName,
>> + (__le16 *)blobptr,
>> + attrptr->length,
>> + attrptr->length,
>> + load_nls_default(), false);
> ^^^^^^^^
> Doesn't
> "load_nls_default" need
> an "unload_nls" when
> you're done with it?
>> + }
>> + }
>> + blobptr += attrsize; /* advance attr value */
>> + attrptr = (struct ntlmssp2_name *) blobptr;
>> + }
>> + } else {
>> + ses->server->tilen = 2 * sizeof(struct ntlmssp2_name);
>> + ses->server->tiblob = kmalloc(ses->server->tilen, GFP_KERNEL);
>> + if (!ses->server->tiblob) {
>> + ses->server->tilen = 0;
>> + cERROR(1, "Challenge target info allocation failure");
>> + return -ENOMEM;
>> + }
>> + memset(ses->server->tiblob, 0x0, ses->server->tilen);
> ^^^^^^^^^^
> This memset can be eliminated if you use kzalloc
> instead.
OK, will change the call.
>
>> + attrptr = (struct ntlmssp2_name *) ses->server->tiblob;
>> + attrptr->type = cpu_to_le16(NTLMSSP_DOMAIN_TYPE);
>> + }
>> +
>> + return rc;
>> +}
>> +
>
> A comment describing what the above function is intended to do would be
> nice. I can sort of guess that it's intended to parse the domain name
> out of one of the NTLMSSP blobs, but I don't quite understand what the
> stuff in the big "else" clause is for. Why allocate the buffer for the
> tiblob in "find_domain_name" ? It seems like that ought to be done in a
> more well-defined fashion.
Because av pairs were removed from struct ntlmv2_resp, if we are using a
a non ntlmssp sec mech e.g. sec=ntlmv2/ntlmv2i, the very same tiblob is
created as it was done before. Servers like Windows 2003 are not very
particular about this blob and so this blob does not need all the Target Info/
AV pairs such as dns/netbios names of the domain and server, timestamp etc.
But you need that (else part) blob nonetheless for sec=ntlmv2i like of security
mech options.
>
>> static int calc_ntlmv2_hash(struct cifsSesInfo *ses,
>> const struct nls_table *nls_cp)
>> {
>> @@ -322,7 +373,8 @@ calc_exit_2:
>> return rc;
>> }
>>
>> -void setup_ntlmv2_rsp(struct cifsSesInfo *ses, char *resp_buf,
>> +int
>> +setup_ntlmv2_rsp(struct cifsSesInfo *ses, char *resp_buf,
>> const struct nls_table *nls_cp)
>> {
>> int rc;
>> @@ -334,10 +386,14 @@ void setup_ntlmv2_rsp(struct cifsSesInfo *ses, char *resp_buf,
>> buf->time = cpu_to_le64(cifs_UnixTimeToNT(CURRENT_TIME));
>> get_random_bytes(&buf->client_chal, sizeof(buf->client_chal));
>> buf->reserved2 = 0;
>> - buf->names[0].type = cpu_to_le16(NTLMSSP_DOMAIN_TYPE);
>> - buf->names[0].length = 0;
>> - buf->names[1].type = 0;
>> - buf->names[1].length = 0;
>> +
>> + if (!ses->domainName) {
>> + rc = find_domain_name(ses);
>> + if (rc) {
>> + cERROR(1, "could not get domain/server name rc %d", rc);
>> + return rc;
>> + }
>> + }
>>
>> /* calculate buf->ntlmv2_hash */
>> rc = calc_ntlmv2_hash(ses, nls_cp);
>> @@ -353,6 +409,8 @@ void setup_ntlmv2_rsp(struct cifsSesInfo *ses, char *resp_buf,
>> memcpy(&ses->server->mac_signing_key.data.ntlmv2.resp, resp_buf,
>> sizeof(struct ntlmv2_resp));
>> ses->server->mac_signing_key.len = 16 + sizeof(struct ntlmv2_resp);
>> +
>> + return rc;
>> }
>>
>> void CalcNTLMv2_response(const struct cifsSesInfo *ses,
>> diff --git a/fs/cifs/cifspdu.h b/fs/cifs/cifspdu.h
>> index f5c78fb..dc90a36 100644
>> --- a/fs/cifs/cifspdu.h
>> +++ b/fs/cifs/cifspdu.h
>> @@ -670,7 +670,6 @@ struct ntlmv2_resp {
>> __le64 time;
>> __u64 client_chal; /* random */
>> __u32 reserved2;
>> - struct ntlmssp2_name names[2];
>> /* array of name entries could follow ending in minimum 4 byte struct */
>> } __attribute__((packed));
>>
>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> index f3ffa69..ccc6d57 100644
>> --- a/fs/cifs/cifsproto.h
>> +++ b/fs/cifs/cifsproto.h
>> @@ -368,7 +368,7 @@ extern int cifs_calculate_mac_key(struct session_key *key, const char *rn,
>> extern int CalcNTLMv2_partial_mac_key(struct cifsSesInfo *,
>> const struct nls_table *);
>> extern void CalcNTLMv2_response(const struct cifsSesInfo *, char *);
>> -extern void setup_ntlmv2_rsp(struct cifsSesInfo *, char *,
>> +extern int setup_ntlmv2_rsp(struct cifsSesInfo *, char *,
>> const struct nls_table *);
>> extern int cifs_crypto_shash_allocate(struct TCP_Server_Info *);
>> extern void cifs_crypto_shash_release(struct TCP_Server_Info *);
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index f5369e7..47b51eb 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -1580,6 +1580,8 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
>> cERROR(1, "could not setup hash structures rc %d", rc);
>> goto out_err;
>> }
>> + tcp_ses->tilen = 0;
>> + tcp_ses->tiblob = NULL;
>>
>> tcp_ses->hostname = extract_hostname(volume_info->UNC);
>> if (IS_ERR(tcp_ses->hostname)) {
>> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
>> index 0a57cb7..6551b17 100644
>> --- a/fs/cifs/sess.c
>> +++ b/fs/cifs/sess.c
>> @@ -383,6 +383,9 @@ static int decode_ascii_ssetup(char **pbcc_area, int bleft,
>> static int decode_ntlmssp_challenge(char *bcc_ptr, int blob_len,
>> struct cifsSesInfo *ses)
>> {
>> + unsigned int tioffset; /* challenge message target info area */
>> + unsigned int tilen; /* challenge message target info area length */
>> +
>> CHALLENGE_MESSAGE *pblob = (CHALLENGE_MESSAGE *)bcc_ptr;
>>
>> if (blob_len < sizeof(CHALLENGE_MESSAGE)) {
>> @@ -405,6 +408,20 @@ static int decode_ntlmssp_challenge(char *bcc_ptr, int blob_len,
>> /* BB spec says that if AvId field of MsvAvTimestamp is populated then
>> we must set the MIC field of the AUTHENTICATE_MESSAGE */
>>
>> + ses->server->ntlmssp.server_flags = le32_to_cpu(pblob->NegotiateFlags);
>> +
>> + tioffset = cpu_to_le16(pblob->TargetInfoArray.BufferOffset);
>> + tilen = cpu_to_le16(pblob->TargetInfoArray.Length);
>> + ses->server->tilen = tilen;
>> + if (tilen) {
>> + ses->server->tiblob = kmalloc(tilen, GFP_KERNEL);
>> + if (!ses->server->tiblob) {
>> + cERROR(1, "Challenge target info allocation failure");
>> + return -ENOMEM;
>> + }
>> + memcpy(ses->server->tiblob, bcc_ptr + tioffset, tilen);
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -533,6 +550,10 @@ static int build_ntlmssp_auth_blob(unsigned char *pbuffer,
>> sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - pbuffer);
>> sec_blob->SessionKey.Length = 0;
>> sec_blob->SessionKey.MaximumLength = 0;
>> +
>> + if (ses->server->tilen > 0)
>> + kfree(ses->server->tiblob);
>> +
>
> I'm not sure I get this...why are you freeing the blob if tilen is 0?
> That seems like you could end up with a leak in corner cases. Why not
> just unconditionally kfree it and make sure it's set to NULL if there
> is no buffer?
Not ure I get it, freeing it only if tilen is greater than 0 i.e. we
did allocate
the blob.
>
>> return tmp - pbuffer;
>> }
>>
>> @@ -729,8 +750,7 @@ ssetup_ntlmssp_authenticate:
>> cpu_to_le16(sizeof(struct ntlmv2_resp));
>>
>> /* calculate session key */
>> - setup_ntlmv2_rsp(ses, v2_sess_key, nls_cp);
>> - /* FIXME: calculate MAC key */
>> + rc = setup_ntlmv2_rsp(ses, v2_sess_key, nls_cp);
> ^^^^^^^^^^^
> You're getting the return code from this function now, but it'll just
> get clobbered when SendReceive2 is called. What's the point? Shouldn't
> you do something different if it comes back with an error? What's going
> to happen if find_domain_name fails and the session setup call proceeds?
In that case, authentication would fail.
>
>> memcpy(bcc_ptr, (char *)v2_sess_key,
>> sizeof(struct ntlmv2_resp));
>> bcc_ptr += sizeof(struct ntlmv2_resp);
>
>
> --
> Jeff Layton <jlayton@xxxxxxxxx>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
