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.
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
