On Fri, 10 Dec 2010 07:07:23 -0600
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Thu, Dec 9, 2010 at 3:27 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> > On Thu, 9 Dec 2010 11:58:11 -0600
> > shirishpargaonkar@xxxxxxxxx wrote:
> >
> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >>
> >>
> >> Indicate to the server a capability of NTLM2 session security (NTLM2 Key)
> >> during ntlmssp protocol exchange in one of the bits of the flags field.
> >> If server supports this capability, send NTLM2 key even if signing is not
> >> required on the server.
> >>
> >> If the server requires signing, the session keys exchanged for NTLMv2
> >> and NTLM2 session security in auth packet of the nlmssp exchange are same.
> >>
> >> Send the same flags in authenticate message (type 3) that client sent in
> >> negotiate message (type 1).
> >>
> >>
> >> Reported-and-Tested-by: Robbert Kouprie <robbert@xxxxxx>
> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >> ---
> >> fs/cifs/sess.c | 13 +++++++------
> >> 1 files changed, 7 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> >> index 7b01d3f..ea39f3f 100644
> >> --- a/fs/cifs/sess.c
> >> +++ b/fs/cifs/sess.c
> >> @@ -437,7 +437,7 @@ static void build_ntlmssp_negotiate_blob(unsigned char *pbuffer,
> >> /* BB is NTLMV2 session security format easier to use here? */
> >> flags = NTLMSSP_NEGOTIATE_56 | NTLMSSP_REQUEST_TARGET |
> >> NTLMSSP_NEGOTIATE_128 | NTLMSSP_NEGOTIATE_UNICODE |
> >> - NTLMSSP_NEGOTIATE_NTLM;
> >> + NTLMSSP_NEGOTIATE_NTLM | NTLMSSP_NEGOTIATE_EXTENDED_SEC;
> >> if (ses->server->secMode &
> >> (SECMODE_SIGN_REQUIRED | SECMODE_SIGN_ENABLED)) {
> >> flags |= NTLMSSP_NEGOTIATE_SIGN;
> >> @@ -446,7 +446,7 @@ static void build_ntlmssp_negotiate_blob(unsigned char *pbuffer,
> >> NTLMSSP_NEGOTIATE_EXTENDED_SEC;
> >> }
> >>
> >> - sec_blob->NegotiateFlags |= cpu_to_le32(flags);
> >> + sec_blob->NegotiateFlags = cpu_to_le32(flags);
> >>
> >
> > Your earlier emails about seeing the POISON_INUSE value in this struct
> > worry me a bit. Maybe we ought to zero out the sec_blob before use?
> >
> > These aren't exactly hot codepaths so we can afford to do a little
> > defensive coding here. Ditto for this buffer in the other phases of
> > NTLMSSP negotiation too.
> >
> > Thoughts?
> >
> >> sec_blob->WorkstationName.BufferOffset = 0;
> >> sec_blob->WorkstationName.Length = 0;
> >> @@ -477,7 +477,7 @@ static int build_ntlmssp_auth_blob(unsigned char *pbuffer,
> >> flags = NTLMSSP_NEGOTIATE_56 |
> >> NTLMSSP_REQUEST_TARGET | NTLMSSP_NEGOTIATE_TARGET_INFO |
> >> NTLMSSP_NEGOTIATE_128 | NTLMSSP_NEGOTIATE_UNICODE |
> >> - NTLMSSP_NEGOTIATE_NTLM;
> >> + NTLMSSP_NEGOTIATE_NTLM | NTLMSSP_NEGOTIATE_EXTENDED_SEC;
> >> if (ses->server->secMode &
> >> (SECMODE_SIGN_REQUIRED | SECMODE_SIGN_ENABLED))
> >> flags |= NTLMSSP_NEGOTIATE_SIGN;
> >> @@ -485,7 +485,7 @@ static int build_ntlmssp_auth_blob(unsigned char *pbuffer,
> >> flags |= NTLMSSP_NEGOTIATE_ALWAYS_SIGN;
> >>
> >> tmp = pbuffer + sizeof(AUTHENTICATE_MESSAGE);
> >> - sec_blob->NegotiateFlags |= cpu_to_le32(flags);
> >> + sec_blob->NegotiateFlags = cpu_to_le32(flags);
> >>
> >> sec_blob->LmChallengeResponse.BufferOffset =
> >> cpu_to_le32(sizeof(AUTHENTICATE_MESSAGE));
> >> @@ -544,8 +544,9 @@ static int build_ntlmssp_auth_blob(unsigned char *pbuffer,
> >> sec_blob->WorkstationName.MaximumLength = 0;
> >> tmp += 2;
> >>
> >> - if ((ses->ntlmssp->server_flags & NTLMSSP_NEGOTIATE_KEY_XCH) &&
> >> - !calc_seckey(ses)) {
> >> + if (((ses->ntlmssp->server_flags & NTLMSSP_NEGOTIATE_KEY_XCH) ||
> >> + (ses->ntlmssp->server_flags & NTLMSSP_NEGOTIATE_EXTENDED_SEC))
> >> + && !calc_seckey(ses)) {
> >> memcpy(tmp, ses->ntlmssp->ciphertext, CIFS_CPHTXT_SIZE);
> >> sec_blob->SessionKey.BufferOffset = cpu_to_le32(tmp - pbuffer);
> >> sec_blob->SessionKey.Length = cpu_to_le16(CIFS_CPHTXT_SIZE);
> >
> >
> >
> > --
> > Jeff Layton <jlayton@xxxxxxxxx>
> >
>
> Yes. I think that would be safe thing to do. I will look into
> wireshark trace again
> just to see what other fields contained POISON_INUSE besides flags field
> when either of the requests (type 1 and type 3 of ntlmssp exchange) were
> sent (such as session key) and resubmit this patch as well.
> This code gets called may be once per smb connection so it does not
> get exercised often, as you stated.
>
We have to consider that wherever you see POISON_INUSE, it could just
as easily be leftover data from earlier use of the page. We really need
to take great care not to stick unintialized data out on the wire.
There's no telling what it might contain.
It may even be best to just to memset all buffers to zero in smb_init
and small_smb_init. Those functions look like they do part of the
buffer now, but it seems a little dangerous to leave the rest unset.
--
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
