On Thu, Feb 17, 2011 at 8:43 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Thu, 17 Feb 2011 07:54:43 -0600
> Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
>
>> On Thu, Feb 17, 2011 at 6:59 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>> > On Thu, 17 Feb 2011 00:59:44 -0600
>> > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
>> >
>> >> On Wed, Feb 16, 2011 at 7:52 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>> >> > On Wed, 16 Feb 2011 18:05:28 -0600
>> >> > shirishpargaonkar@xxxxxxxxx wrote:
>> >> >
>> >> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> >> >>
>> >> >>
>> >> >> Fix lanman (lm) authentication code.
>> >> >>
>> >> >> Change lm response length back to 24 from 16.
>> >> >> Add code to add odd parity bit to each of the eight bytes of a DES key.
>> >> >>
>> >> >>
>> >> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> >> >> ---
>> >> >> fs/cifs/sess.c | 8 ++++----
>> >> >> fs/cifs/smbdes.c | 5 ++++-
>> >> >> 2 files changed, 8 insertions(+), 5 deletions(-)
>> >> >>
>> >> >> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
>> >> >> index 1adc962..1676570 100644
>> >> >> --- a/fs/cifs/sess.c
>> >> >> +++ b/fs/cifs/sess.c
>> >> >> @@ -656,13 +656,13 @@ ssetup_ntlmssp_authenticate:
>> >> >>
>> >> >> if (type == LANMAN) {
>> >> >> #ifdef CONFIG_CIFS_WEAK_PW_HASH
>> >> >> - char lnm_session_key[CIFS_SESS_KEY_SIZE];
>> >> >> + char lnm_session_key[CIFS_AUTH_RESP_SIZE];
>> >> >>
>> >> >> pSMB->req.hdr.Flags2 &= ~SMBFLG2_UNICODE;
>> >> >>
>> >> >> /* no capabilities flags in old lanman negotiation */
>> >> >>
>> >> >> - pSMB->old_req.PasswordLength = cpu_to_le16(CIFS_SESS_KEY_SIZE);
>> >> >> + pSMB->old_req.PasswordLength = cpu_to_le16(CIFS_AUTH_RESP_SIZE);
>> >> >>
>> >> >> /* Calculate hash with password and copy into bcc_ptr.
>> >> >> * Encryption Key (stored as in cryptkey) gets used if the
>> >> >> @@ -675,8 +675,8 @@ ssetup_ntlmssp_authenticate:
>> >> >> true : false, lnm_session_key);
>> >> >>
>> >> >> ses->flags |= CIFS_SES_LANMAN;
>> >> >> - memcpy(bcc_ptr, (char *)lnm_session_key, CIFS_SESS_KEY_SIZE);
>> >> >> - bcc_ptr += CIFS_SESS_KEY_SIZE;
>> >> >> + memcpy(bcc_ptr, (char *)lnm_session_key, CIFS_AUTH_RESP_SIZE);
>> >> >> + bcc_ptr += CIFS_AUTH_RESP_SIZE;
>> >> >>
>> >> >> /* can not sign if LANMAN negotiated so no need
>> >> >> to calculate signing key? but what if server
>> >> >> diff --git a/fs/cifs/smbdes.c b/fs/cifs/smbdes.c
>> >> >> index 0472148..0ab5def 100644
>> >> >> --- a/fs/cifs/smbdes.c
>> >> >> +++ b/fs/cifs/smbdes.c
>> >> >> @@ -312,8 +312,11 @@ str_to_key(unsigned char *str, unsigned char *key)
>> >> >> key[5] = ((str[4] & 0x1F) << 2) | (str[5] >> 6);
>> >> >> key[6] = ((str[5] & 0x3F) << 1) | (str[6] >> 7);
>> >> >> key[7] = str[6] & 0x7F;
>> >> >> - for (i = 0; i < 8; i++)
>> >> >> + for (i = 0; i < 8; i++) {
>> >> >> key[i] = (key[i] << 1);
>> >> >> + if (~(hweight8(key[i]) & 1))
>> >> >> + key[i] |= 0x1; /* add odd parity bit */
>> >> >> + }
>> >> >
>> >> > This doesn't look right:
>> >> >
>> >> > hweight8 will return an unsigned int. You & that with 1, so you'll
>> >> > either get 0x1 (for odd number of bits) or 0x0 (for even). Then take
>> >> > the one's compliment of that and you'll either have 0xfffffffe (odd) or
>> >> > 0xffffffff (even), both of which will return true.
>> >> >
>> >> > I think you'll need to do this differently -- perhaps something like
>> >> > this which also has the advantage of avoiding the if statement:
>> >> >
>> >> > /* set the odd parity bit */
>> >> > key[i] |= (~(hweight8(key[i]) & 1)) & 1;
>> >>
>> >> This should work
>> >> key[i] |= !(hweight8(key[i]) % 2);
>> >
>> > That should work if the ! always turns 0x0 into 0x1. How certain of that
>> > are you?
>> >
>> > --
>> > Jeff Layton <jlayton@xxxxxxxxxx>
>> >
>>
>> What are the various ways to confirm that? I tried with couple of different
>> passwords and printed bits in characters used in DES keys and verified it.
>
> I suppose you could look at how "bool" is typedef'ed in the kernel and
> try to determine it. My worry is that different compilers or
> architectures could do that differently. Since "not zero" becomes
> "true", then it seems plausible that "true" could be converted back to
> any value that is "not zero".
> Personally, I'd go with something closer to what I proposed before
> since it's unambiguous.
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
I think we do not need to add odd parity bit to the key (in str_to_key) .
I think crypto encryption function (ecb(des)) and locally
dohash() function do take care of that.
It is hard to trace that in dohash() function in smbdes.c but I think
it happens. ntlm auth would be broken without it.
--
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
