On Fri, Oct 15, 2010 at 10:08 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> On Fri, 15 Oct 2010 09:51:59 -0500
> Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
>
>> On Fri, Oct 15, 2010 at 9:36 AM, Shirish Pargaonkar
>> <shirishpargaonkar@xxxxxxxxx> wrote:
>> > On Fri, Oct 15, 2010 at 9:02 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
>> >> On Tue, 12 Oct 2010 14:45:19 -0500
>> >> shirishpargaonkar@xxxxxxxxx wrote:
>> >>
>> >>> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> >>>
>> >>>
>> >>> Have to move av_pair/ti blob to session key as part of ntlmv2 response.
>> >>> This entire blob is part of the response and is use to calculate
>> >>> signature for cases like ntlmv2 (without extended security) authentication.
>> >>> The size of the blob is hard coded to 1024 byte which should be
>> >>> sufficient to accomodate all 11 types of av pair types and their
>> >>> lenghts/values.
>> >>>
>> >>>
>> >>> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> >>
>> >> [...]
>> >>
>> >>> hmac_md5_final(v2_session_response, &context);
>> >>> /* cifs_dump_mem("v2_sess_rsp: ", v2_session_response, 32); */
>> >>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> >>> index 7d834b1..5dd5659 100644
>> >>> --- a/fs/cifs/cifsglob.h
>> >>> +++ b/fs/cifs/cifsglob.h
>> >>> @@ -105,6 +105,7 @@ struct session_key {
>> >>> struct {
>> >>> char key[16];
>> >>> struct ntlmv2_resp resp;
>> >>> + unsigned char tiblob[CIFS_MAX_NTLMV2_AVPAIR_SIZE];
>> >>
>> >> ^^^^^
>> >> Is this really the best way to do this? We're increasing the size of
>> >> this struct substantially, thereby increasing the size of every
>> >> TCP_Server_Info and cifsSesInfo struct by 1k.
>> >>
>> >> Would it be better to keep the tiblob dynamically allocated, and hand
>> >> off the pointer to it? If it really is necessary to add this field to
>> >> the session_key struct in this way, would it be better to change how
>> >> session keys are managed? i.e. not embed them inside of TCP_Server_Info
>> >> and cifsSesInfo?
>> >
>> > Jeff, we need to have a contiguous block of ntlmv2 client response i.e.
>> > First part of client response (CR1), the 16 bytes and the variable second part
>> > of client response which consists of struct ntlmv2_resp and tiblob.
>> > Without contiguous blob, i.e. if tiblob is a pointer pointing to a dynamically
>> > allocated memory, when it comes to calculating signature, this code,
>> >
>> > crypto_shash_update(&server->secmech.sdescmd5->shash,
>> > (char *)&server->session_key.data, len);
>> >
>> > we end up copying incorrect contents.
>> >
>> > I agree 1K is a lot but I can reduce it. av pair/target info blob can vary
>> > in size, we can only guestimate how large it could.
>> >
>> > Perhaps I can keep the way it is, and do two updates, one update with fixed
>> > part, key + ntlmv2_resp structure and another with tiblob.
>> > Let me try that, I think that will work and if it works, we do not
>> > need this patch,
>> > I can make that change as part of current 4/4 patch.
>>
>> In which case we have to keep tiblob and tilen for the life of the smb
>> connection.
>> This is an issue for ntlmv2 authentication without extended security only
>> since the entire client response is used in signature calculation.
>> For ntlmv2 in ntlmssp, only first 16 bytes of the client response are needed for
>> signature calculation.
>>
>
>
> We only need to keep one of these for the life of the connection,
> right? After the initial authentication is done, only the first
> session_key established on the socket will ever be used again.
>
Jeff, yes.
> Instead of memcpy'ing this key around between embedded session_key
> structs, wouldn't it make more sense to allocate them dynamically, and
> "hand off" the first session key to the socket. Then you could just
> free the others.
>
> It also sort of sucks that this session_key is a union instead of
> having its size dynamically determined. That's quite a bit of wasted
> space for the other cases that don't need a session_key this large.
>
> Maybe the session key should just be a pointer and a length field, that
> gets set up whenever the thing is created?
>
> --
> 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
