On Fri, Dec 10, 2010 at 4:06 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> 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));
This change looks safe to me. Will incorporate in the patch.
>
> ...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.
I do not mind undertaking this but would really prefer to do this in a
separate patch. And can get rid of above memet and the one in
header_assemble().
>
> --
> 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
>
--
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
