On Wed, Oct 27, 2010 at 6:38 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Wed, 27 Oct 2010 16:20:48 -0500
> Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
>
>> On Wed, Oct 27, 2010 at 4:18 PM, Shirish Pargaonkar
>> <shirishpargaonkar@xxxxxxxxx> wrote:
>> > On Wed, Oct 27, 2010 at 3:56 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>> >> On Wed, 27 Oct 2010 15:20:36 -0500
>> >> shirishpargaonkar@xxxxxxxxx wrote:
>> >>
>> >>> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> >>>
>> >>>
>> >>> Need to have cryptkey or server challenge in smb connection
>> >>> (struct TCP_Server_Info) for ntlm and ntlmv2 auth types for which
>> >>> cryptkey (Encryption Key) is supplied just once in Negotiate Protocol
>> >>> response during an smb connection setup for all the smb sessions over
>> >>> that smb connection.
>> >>>
>> >>> For ntlmssp, cryptkey or server challenge is provided for every
>> >>> smb session in type 2 packet of ntlmssp negotiation, the cryptkey
>> >>> provided during Negotiation Protocol response before smb connection
>> >>> does not count.
>> >>>
>> >>> Rename cryptKey to cryptkey and related changes.
>> >>>
>> >>>
>> >>> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> >>> ---
>> >>> fs/cifs/cifsencrypt.c | 10 +++++++---
>> >>> fs/cifs/cifsglob.h | 3 ++-
>> >>> fs/cifs/cifssmb.c | 4 ++--
>> >>> fs/cifs/connect.c | 4 ++--
>> >>> fs/cifs/sess.c | 12 ++++++++----
>> >>> 5 files changed, 21 insertions(+), 12 deletions(-)
>> >>>
>> >>
>> >> [...]
>> >>
>> >>>
>> >>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> >>> index 67d6a22..b736951 100644
>> >>> --- a/fs/cifs/cifsglob.h
>> >>> +++ b/fs/cifs/cifsglob.h
>> >>> @@ -196,6 +196,7 @@ struct TCP_Server_Info {
>> >>> int capabilities; /* allow selective disabling of caps by smb sess */
>> >>> int timeAdj; /* Adjust for difference in server time zone in sec */
>> >>> __u16 CurrentMid; /* multiplex id - rotating counter */
>> >>> + char cryptkey[CIFS_CRYPTO_KEY_SIZE]; /* used by ntlm, ntlmv2 etc */
>> >>> /* 16th byte of RFC1001 workstation name is always null */
>> >>> char workstation_RFC1001_name[RFC1001_NAME_LEN_WITH_NULL];
>> >>> __u32 sequence_number; /* needed for CIFS PDU signature */
>> >>> @@ -240,7 +241,7 @@ struct cifsSesInfo {
>> >>> char userName[MAX_USERNAME_SIZE + 1];
>> >>> char *domainName;
>> >>> char *password;
>> >>> - char cryptKey[CIFS_CRYPTO_KEY_SIZE];
>> >>> + char cryptkey[CIFS_CRYPTO_KEY_SIZE]; /* used by ntlmssp */
>> >>
>> >> I'm trying to understand why we need ses->cryptkey at all. It holds the
>> >> cryptkey provided by the server only if we're using ntlmssp. It's
>> >> only used once, and essentially it just holds the key until we have a
>> >> auth_key.response field that we can stuff this in. Is that correct? If
>> >> so, wouldn't it make more sense to just allocate the auth_key.response
>> >> field earlier and put this directly in there?
>> >>
>> >> --
>> >> Jeff Layton <jlayton@xxxxxxxxxx>
>> >>
>> >
>> > Jeff, we could do that. We then have to allocate .response in function
>> > decode_ntlmssp_challenge(). We have to worry about freeing it if there
>> > are any errors between decode_ntlmssp_challenge() and setup_ntlmv2_response().
>> > In function setup_ntlmv2_rsp(), we have to allocate .response only if called
>> > in case of ntlmv2 without extended security, so an extra if statement in there.
>> >
>> > I think current code is simpler for the price of 16 bytes of cryptkey storage?
>> >
>>
>> Not 16, 8 bytes. CIFS_CRYPTO_KEY_SIZE is 8 bytes, I thought it was 16 bytes.
>
> I don't think that's simpler. I'm becoming concerned about the
> proliferation of fields in cifsSesInfo. All of these are recent
> additions by your patches to fix NTLMSSP support.
>
> char cryptKey[CIFS_CRYPTO_KEY_SIZE];
> struct session_key auth_key;
> char ntlmv2_hash[16];
> unsigned int tilen; /* length of the target info blob */
> unsigned char *tiblob; /* target info blob in challenge response */
> struct ntlmssp_auth ntlmssp; /* ciphertext, flags */
>
> ...it's not so much the space that they take up, even though that's not
> exactly trivial. A lot of these fields have no use at all after
> authentication is done. There's a lot of memcpy'ing going on here too
> which is a red flag.
>
> It's not clear to me why you need so many fields for this. It would
> seem to me that it would be a good idea to think about each of these
> fields and determine which ones need to stick around after
> authentication is done (i.e. for signing), and store the rest of them
> in a structure that can be freed after authentication is done.
>
> My main concern here is debugging this later. One of the main
> techniques I use when tracking down bugs is to follow the lifecycle of
> the data. Whenever you add a new field, it makes it a lot more
> difficult for myself and others to follow the code.
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
Posted a follow up patch that addresses the concerns. If that patch
is agreeable in
some or no additional variations, it should follow this patch if this
patch is good enough
because the functionality in this patch is needed for multiple smb
sessions per smb connection.
--
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
