On Tue, Sep 7, 2010 at 8:00 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> On Tue, 7 Sep 2010 07:52:09 -0500
> Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
>
>> 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.
>>
>
> Is it possible for tilen to end up being set to 0, and the tiblob to be
> a valid pointer? If so, then this buffer will be leaked here. Right?
That would not be possible. In decode_ntlmssp_challenge, blob is
not allocated unless tilen is greater than 0.
In find_domain_name if blob allocation (tilen long) failure is
handled.
>
>> >
>> >> 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.
>>
>
> So why not return with a valid error code instead of leaving us
> wondering what the heck went wrong?
Could loging a meaningful error suffice?
>
>> >
>> >> memcpy(bcc_ptr, (char *)v2_sess_key,
>> >> sizeof(struct ntlmv2_resp));
>> >> bcc_ptr += sizeof(struct ntlmv2_resp);
>> >
>> >
>> > --
>> > Jeff Layton <jlayton@xxxxxxxxx>
>> >
>>
>
>
> --
> 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
