On Wed, Oct 20, 2010 at 10:31 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> On Wed, 20 Oct 2010 10:15:27 -0500
> Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
>
>> On Wed, Oct 20, 2010 at 9:45 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
>> > On Tue, 19 Oct 2010 19:13:57 -0500
>> > shirishpargaonkar@xxxxxxxxx wrote:
>> >
>> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> >>
>> >>
>> >> Start calculation auth response within a session. Move/Add pertinet
>> >> data structures like session key, server challenge and ntlmv2_hash in
>> >> a session structure. We should do the calculations within a session
>> >> before copying session key and response over to server data
>> >> structures because a session setup can fail.
>> >>
>> >> Only after a very first smb session succeeds, it copy/make its
>> >> session key, session key of smb connection. This key stays with
>> >> the smb connection throughout its life.
>> >>
>> >> The authentication response field within structure session_key
>> >> is now dynamic. Every authentication type allocates the response sized
>> >> memory within its session structure and later frees it once the
>> >> client response is sent and if session's session key becomes
>> >> connetion's session key.
>> >>
>> >> ntlm/ntlmi authentication functions are rearranged. A function
>> >> named setup_ntlm_resp(), similar to setup_ntlmv2_resp(), replaces
>> >> function cifs_calculate_session_key().
>> >>
>> >> size of CIFS_SESS_KEY_SIZE is changed to 16, to reflect the byte size
>> >> of the key it holds.
>> >>
>> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> >> ---
>> >> fs/cifs/cifsencrypt.c | 99 +++++++++++++++++++++++++----------------------
>> >> fs/cifs/cifsglob.h | 10 +----
>> >> fs/cifs/cifspdu.h | 7 +++-
>> >> fs/cifs/cifsproto.h | 11 ++---
>> >> fs/cifs/connect.c | 33 ++++++++++++++--
>> >> fs/cifs/sess.c | 103 +++++++++++++++++++------------------------------
>> >> fs/cifs/transport.c | 6 +-
>> >> 7 files changed, 137 insertions(+), 132 deletions(-)
>> >>
>> >
>> > [...]
>> >
>> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> >> index 18ee0ad..c374859 100644
>> >> --- a/fs/cifs/cifsglob.h
>> >> +++ b/fs/cifs/cifsglob.h
>> >> @@ -98,15 +98,9 @@ enum protocolEnum {
>> >> };
>> >>
>> >> struct session_key {
>> >> + char key[16];
>> >> + char *response;
>> > ^^^^^^^
>> > What's the point of keeping the key separate from the response?
>> > If you'll always need to allocate memory for the response, why
>> > not allocate the key too as part of the same allocation?
>>
>> The key (session key) is different than response. We need both when
>> generating signature. The key is always fixed in size, 16 byte whereas
>> response can vary, 24 bytes long for ntlm, variable length for ntlmv2.
>>
>
> The key and response are separate pieces, sure but there's no reason
> that they need to be allocated separately. That design decision is what
> causes some of the special casing in later parts of the patch.
>
>> >> mutex_lock(&ses->server->srv_mutex);
>> >> if (!server->session_estab) {
>> >> - memcpy(&server->session_key.data,
>> >> - &ses->auth_key.data, ses->auth_key.len);
>> >> - server->session_key.len = ses->auth_key.len;
>> >> - ses->server->session_estab = true;
>> >> + if (server->secType == RawNTLMSSP)
>> >> + len = 16;
>> > ^^^^^^^^^
>> > Why is this special-cased here? Does the
>> > auth_key have an incorrect len field in the
>> > RawNTLMSSP case?
>>
>> I could take this out. But for NTLMv2 within NTLMSSP, only key gets used
>> in the signature, so for NTLMv2 in NTLMSSP (RawNTLMSSP), I do not even
>> need to copy the response.
>>
>
> Again, the fact that these fields are separate seems to be causing a
> lot of special-casing. That's generally a bad sign. I encourage you to
> rethink this approach.
>
>> >
>> >> + else
>> >> + len = ses->auth_key.len;
>> >> + server->session_key.response = kmalloc(len, GFP_KERNEL);
>> >> + if (!server->session_key.response) {
>> >> + rc = -ENOMEM;
>> >> + mutex_unlock(&server->srv_mutex);
>> >> + goto cifs_setup_session_ret;
>> >> + }
>> >> + memcpy(server->session_key.response,
>> >> + ses->auth_key.response, len);
>> >> + memcpy(server->session_key.key,
>> >> + ses->auth_key.key, 16);
>> >
>> > Hmmm...you're going to be freeing ses->auth_key.response below.
>> > Wouldn't it make more sense to just "hand-off" the pointer to the
>> > response field instead? For instance:
>> >
>> > server->session_key.response = ses->auth_key.response;
>> > ses->auth_key.response = NULL;
>> > ses->auth_key.len = 0;
>> >
>> > Then you don't have to deal with an error condition, etc...
>> >
>> >> + server->session_key.len = len;
>> >> + server->session_estab = true;
>>
>> Only for the first session do we do this copy. For rest of the sessions,
>> the auth_key response is freed. So when we go to free response down here,
>>
>> >> }
>> >> mutex_unlock(&server->srv_mutex);
>> >>
>> >> @@ -3192,6 +3212,11 @@ int cifs_setup_session(unsigned int xid, struct cifsSesInfo *ses,
>> >> spin_unlock(&GlobalMid_Lock);
>> >> }
>> >>
>> >> +cifs_setup_session_ret:
>> >> + kfree(ses->auth_key.response);
>>
>> we have to do a check whether this respose for a that has become part
>> of smb connection.
>>
>
> That's why you zero out the original pointer when you hand it off. The
> kfree will be a no-op in that case. That will be far simpler and more
> efficient then allocating more memory and copying it.
>
>> >> + ses->auth_key.response = NULL;
>> >> + ses->auth_key.len = 0;
>> >> +
>> >> return rc;
>> >> }
>> >>
>> >> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
>> >> index e35dc60..8bb4518 100644
>> >> --- a/fs/cifs/sess.c
>> >> +++ b/fs/cifs/sess.c
>> >
>> > [...]
>> >
>> >> @@ -785,16 +756,24 @@ ssetup_ntlmssp_authenticate:
>> >> rc = -EKEYREJECTED;
>> >> goto ssetup_exit;
>> >> }
>> >> +
>> >> + resplen = CIFS_SESS_KEY_SIZE + CIFS_AUTH_RESP_SIZE;
>> >> /* bail out if key is too long */
>> >> - if (msg->sesskey_len >
>> >> - sizeof(ses->auth_key.data.krb5)) {
>> >> + if (msg->sesskey_len > resplen) {
>> >> cERROR(1, "Kerberos signing key too long (%u bytes)",
>> >> msg->sesskey_len);
>> >> rc = -EOVERFLOW;
>> >> goto ssetup_exit;
>> >> }
>> >> + ses->auth_key.response = kmalloc(resplen, GFP_KERNEL);
>> >> + if (!ses->auth_key.response) {
>> >> + cERROR(1, "Kerberos can't allocate (%u bytes) memory",
>> >> + resplen);
>> >> + rc = -ENOMEM;
>> >> + goto ssetup_exit;
>> >> + }
>> >> ses->auth_key.len = msg->sesskey_len;
>> >> - memcpy(ses->auth_key.data.krb5, msg->data, msg->sesskey_len);
>> >> + memcpy(ses->auth_key.response, msg->data, msg->sesskey_len);
>> >
>> > ^^^^^^^^^^
>> > This looks questionable. Previously we had a fixed 40
>> > byte field for the krb5 key (CIFS_SESS_KEY_SIZE (24) +
>> > 16). Now, we have a 16 byte key and a 40-byte response field
>> > (CIFS_SESS_KEY_SIZE (16) + CIFS_AUTH_RESP_SIZE (24)),
>> > You're copying the data into the auth_key.response but
>> > the auth_key.key seems to be unused. Won't that mean
>> > that signing is broken?
>> >
>> > I suppose you could copy the first 16 bytes into the
>> > key part and then stick the rest into the response
>> > field, but that seems sort of kludgey. It would make
>> > more sense to have the session_key just be a length and
>> > a pointer to memory allocated off the slab and do away
>> > with the fixed-length key[16] field.
>> >
>> > Then when you want to access the key/response, just
>> > cast that pointer to a struct that has the fields you
>> > need.
>>
>> As I stated above, response and key can't be mixed. At least for
>> ntlm and ntlmv2 authentications. For Kerberos signing, we would
>> just use response and not the key.
>>
>
> Ok, but the signature functions don't seem to do that. They seem to do
> this unconditionally:
>
> cifs_MD5_update(&context, server->session_key.key, 16);
>
> ...since there's no way for them to know whether the key field is
> actually valid or not.
>
> In fact, the signing functions don't look correct either. You have two
> functions for calculating the signature now (cifs_calculate_signature
> and cifs_calc_signature2), but cifs_verify_signature only calls
> cifs_calculate_signature. Shouldn't it call cifs_calc_signature2 when
> that sort of key is in use?
>
> Another idea to consider: It might be worthwhile to track the type of
> key that you have in the session_key struct so that you don't have to
> refer back to the TCP_Server_Info->secType field in order to determine
> it.
>
> --
> Jeff Layton <jlayton@xxxxxxxxx>
>
yes, I will rework the code. special casing should be avoided as much
as possible.
--
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
