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