On Fri, 10 Dec 2010 15:55:52 -0600
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Fri, Dec 10, 2010 at 3:48 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> > On Fri, 10 Dec 2010 15:43:17 -0600
> > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> >
> >> On Fri, Dec 10, 2010 at 3:13 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> >> > On Fri, 10 Dec 2010 10:30:47 -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).
> >> >>
> >> >> Remove function setup_ntlmssp_neg_req() and allocate zero'ed buffers
> >> >> to prepare ntlmssp negotiate and authenticat requests.
> >> >>
> >> >>
> >> >> Reported-and-Tested-by: Robbert Kouprie <robbert@xxxxxx>
> >> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >> >> ---
> >> >> fs/cifs/sess.c | 46 +++++++++++++++++++++++++---------------------
> >> >> 1 files changed, 25 insertions(+), 21 deletions(-)
> >> >>
> >> >> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> >> >> index 7b01d3f..56e945a 100644
> >> >> --- a/fs/cifs/sess.c
> >> >> +++ b/fs/cifs/sess.c
> >> >> @@ -425,7 +425,7 @@ static int decode_ntlmssp_challenge(char *bcc_ptr, int blob_len,
> >> >>
> >> >> /* We do not malloc the blob, it is passed in pbuffer, because
> >> >> it is fixed size, and small, making this approach cleaner */
> >> >> -static void build_ntlmssp_negotiate_blob(unsigned char *pbuffer,
> >> >> +static void build_ntlmssp_negotiate_blob(char *pbuffer,
> >> >> struct cifsSesInfo *ses)
> >> >> {
> >> >> NEGOTIATE_MESSAGE *sec_blob = (NEGOTIATE_MESSAGE *)pbuffer;
> >> >> @@ -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);
> >> >>
> >> >> 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);
> >> >> @@ -562,16 +563,6 @@ setup_ntlmv2_ret:
> >> >> *buflen = tmp - pbuffer;
> >> >> return rc;
> >> >> }
> >> >> -
> >> >> -
> >> >> -static void setup_ntlmssp_neg_req(SESSION_SETUP_ANDX *pSMB,
> >> >> - struct cifsSesInfo *ses)
> >> >> -{
> >> >> - build_ntlmssp_negotiate_blob(&pSMB->req.SecurityBlob[0], ses);
> >> >> - pSMB->req.SecurityBlobLength = cpu_to_le16(sizeof(NEGOTIATE_MESSAGE));
> >> >> -
> >> >> - return;
> >> >> -}
> >> > ^^^^^
> >> > I like that you removed the above function. It was useless indirection.
> >> >
> >> >> #endif
> >> >>
> >> >> int
> >> >> @@ -595,6 +586,7 @@ CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses,
> >> >> __le32 phase = NtLmNegotiate; /* NTLMSSP, if needed, is multistage */
> >> >> u16 blob_len;
> >> >> char *ntlmsspblob = NULL;
> >> >> + char *ntlmsspnblob = NULL;
> >> >>
> >> >> if (ses == NULL)
> >> >> return -EINVAL;
> >> >> @@ -828,16 +820,26 @@ ssetup_ntlmssp_authenticate:
> >> >> capabilities |= CAP_EXTENDED_SECURITY;
> >> >> pSMB->req.Capabilities |= cpu_to_le32(capabilities);
> >> >> if (phase == NtLmNegotiate) {
> >> >> - setup_ntlmssp_neg_req(pSMB, ses);
> >> >> + ntlmsspnblob = kzalloc(
> >> >> + sizeof(struct _NEGOTIATE_MESSAGE),
> >> >> + GFP_KERNEL);
> >> >> + if (!ntlmsspnblob) {
> >> >> + cERROR(1, "Can't allocate NTLMSSPN");
> >> >> + rc = -ENOMEM;
> >> >> + goto ssetup_exit;
> >> >> + }
> >> >> + build_ntlmssp_negotiate_blob(ntlmsspnblob, ses);
> >> >
> >> > Why did you decide to allocate a new buffer here? Was the SecurityBlob
> >> > not big enough? If not, then I think it would make more sense to just
> >> > memset the existing buffer to 0.
> >>
> >> No the buffer allocated by small_smb_init is sufficient to hold the
> >> ntlmssp negotiate request. I do not need to allocate a new buffer.
> >> And header_assemble memsets that buffer to 0, so no need to redo it again.
> >>
> >> So only ntlmsspblob needs to be kzalloc'ed.
> >>
> >
> > header_assemble does this:
> >
> > memset(temp, 0, 256); /* bigger than MAX_CIFS_HDR_SIZE */
> >
> > ...is that enough? (I haven't measured it).
>
> I think so. That is what it had been all along (till I changed the code here).
> So small_int_buf allocated 256 byte long buf should be enough to carry
> ntlmssp negotiate request with session setup request headed by
> SMB header. So I think we can keep the same code, no need to
> allocate additional buffer for ntlmssp negotiate request.
>
>
I agree that we don't need a new buffer. I'm just wondering whether
header_assemble is zeroing enough of the old one. The safe thing to do
would be to do this in build_ntlmssp_negotiate_blob:
memset(pbuffer, 0, sizeof(NEGOTIATE_MESSAGE));
...or even better would be to consider zeroing all of the buffer in
smb_init or small_smb_init. We do need to be extra careful to not ever
send unintialized data on the wire.
--
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
