Re: [PATCH 2/2] ksmbd: remove NTLMv1 authentication

Tom Talpey
2021-09-29T15:10:07Z

Ok, feel to free to add my
Reviewed-By: Tom Talpey <tom@talpey.com>

On 9/28/2021 8:34 PM, Namjae Jeon wrote:
> 2021-09-28 23:32 GMT+09:00, Tom Talpey <tom@talpey.com>:
>> On 9/27/2021 8:47 AM, Namjae Jeon wrote:
>>> Remove insecure NTLMv1 authentication.
>>
>> There are some extremely confusing name overloads in this file.
>> Apparently "ksmbd_auth_ntlmv2()" and "__ksmb2_auth_ntlmv2()"
>> are entirely different things! Yes, this patch removes one,
>> but it's not easy to review.
> A long time ago, There was a mistake to rename this function to
> current name during clean-up.
>>
>>> /**
>>> * ksmbd_auth_ntlmv2() - NTLMv2 authentication handler
>>> * @sess: session of connection
>>> * @ntlmv2: NTLMv2 challenge response
>>> * @blen: NTLMv2 blob length
>>> * @domain_name: domain name
>>> *
>>> * Return: 0 on success, error number on error
>>> */
>>
>>> /**
>>> * __ksmbd_auth_ntlmv2() - NTLM2(extended security) authentication
>>> handler
>>> * @sess: session of connection
>>> * @client_nonce: client nonce from LM response.
>>> * @ntlm_resp: ntlm response data from client.
>>> *
>>> * Return: 0 on success, error number on error
>>> */
>>
>> Two questions:
>> 1) Have you tested this does not remove existing NTLMv2 support?
> Yes, tested. This is NTLM2 not NTLMv2.
>> 2) Does this fully clean up the rather insane function naming?
> Yes, This patch will do all(remove NTLM and insane fucntion name) :)
>>
>> Tom.
> Thank you for your review!
>>
>>> Cc: Tom Talpey <tom@talpey.com>
>>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>>> Cc: Ralph Böhme <slow@samba.org>
>>> Cc: Steve French <smfrench@gmail.com>
>>> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
>>> Cc: Hyunchul Lee <hyc.lee@gmail.com>
>>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>>> ---
>>> fs/ksmbd/auth.c | 205 ------------------------------------------
>>> fs/ksmbd/crypto_ctx.c | 16 ----
>>> fs/ksmbd/crypto_ctx.h | 8 --
>>> 3 files changed, 229 deletions(-)
>>>
>>> diff --git a/fs/ksmbd/auth.c b/fs/ksmbd/auth.c
>>> index de36f12070bf..71c989f1568d 100644
>>> --- a/fs/ksmbd/auth.c
>>> +++ b/fs/ksmbd/auth.c
>>> @@ -68,125 +68,6 @@ void ksmbd_copy_gss_neg_header(void *buf)
>>> memcpy(buf, NEGOTIATE_GSS_HEADER, AUTH_GSS_LENGTH);
>>> }
>>>
>>> -static void
>>> -str_to_key(unsigned char *str, unsigned char *key)
>>> -{
>>> - int i;
>>> -
>>> - key[0] = str[0] >> 1;
>>> - key[1] = ((str[0] & 0x01) << 6) | (str[1] >> 2);
>>> - key[2] = ((str[1] & 0x03) << 5) | (str[2] >> 3);
>>> - key[3] = ((str[2] & 0x07) << 4) | (str[3] >> 4);
>>> - key[4] = ((str[3] & 0x0F) << 3) | (str[4] >> 5);
>>> - 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++)
>>> - key[i] = (key[i] << 1);
>>> -}
>>> -
>>> -static int
>>> -smbhash(unsigned char *out, const unsigned char *in, unsigned char *key)
>>> -{
>>> - unsigned char key2[8];
>>> - struct des_ctx ctx;
>>> -
>>> - if (fips_enabled) {
>>> - ksmbd_debug(AUTH, "FIPS compliance enabled: DES not permitted\n");
>>> - return -ENOENT;
>>> - }
>>> -
>>> - str_to_key(key, key2);
>>> - des_expand_key(&ctx, key2, DES_KEY_SIZE);
>>> - des_encrypt(&ctx, out, in);
>>> - memzero_explicit(&ctx, sizeof(ctx));
>>> - return 0;
>>> -}
>>> -
>>> -static int ksmbd_enc_p24(unsigned char *p21, const unsigned char *c8,
>>> unsigned char *p24)
>>> -{
>>> - int rc;
>>> -
>>> - rc = smbhash(p24, c8, p21);
>>> - if (rc)
>>> - return rc;
>>> - rc = smbhash(p24 + 8, c8, p21 + 7);
>>> - if (rc)
>>> - return rc;
>>> - return smbhash(p24 + 16, c8, p21 + 14);
>>> -}
>>> -
>>> -/* produce a md4 message digest from data of length n bytes */
>>> -static int ksmbd_enc_md4(unsigned char *md4_hash, unsigned char
>>> *link_str,
>>> - int link_len)
>>> -{
>>> - int rc;
>>> - struct ksmbd_crypto_ctx *ctx;
>>> -
>>> - ctx = ksmbd_crypto_ctx_find_md4();
>>> - if (!ctx) {
>>> - ksmbd_debug(AUTH, "Crypto md4 allocation error\n");
>>> - return -ENOMEM;
>>> - }
>>> -
>>> - rc = crypto_shash_init(CRYPTO_MD4(ctx));
>>> - if (rc) {
>>> - ksmbd_debug(AUTH, "Could not init md4 shash\n");
>>> - goto out;
>>> - }
>>> -
>>> - rc = crypto_shash_update(CRYPTO_MD4(ctx), link_str, link_len);
>>> - if (rc) {
>>> - ksmbd_debug(AUTH, "Could not update with link_str\n");
>>> - goto out;
>>> - }
>>> -
>>> - rc = crypto_shash_final(CRYPTO_MD4(ctx), md4_hash);
>>> - if (rc)
>>> - ksmbd_debug(AUTH, "Could not generate md4 hash\n");
>>> -out:
>>> - ksmbd_release_crypto_ctx(ctx);
>>> - return rc;
>>> -}
>>> -
>>> -static int ksmbd_enc_update_sess_key(unsigned char *md5_hash, char
>>> *nonce,
>>> - char *server_challenge, int len)
>>> -{
>>> - int rc;
>>> - struct ksmbd_crypto_ctx *ctx;
>>> -
>>> - ctx = ksmbd_crypto_ctx_find_md5();
>>> - if (!ctx) {
>>> - ksmbd_debug(AUTH, "Crypto md5 allocation error\n");
>>> - return -ENOMEM;
>>> - }
>>> -
>>> - rc = crypto_shash_init(CRYPTO_MD5(ctx));
>>> - if (rc) {
>>> - ksmbd_debug(AUTH, "Could not init md5 shash\n");
>>> - goto out;
>>> - }
>>> -
>>> - rc = crypto_shash_update(CRYPTO_MD5(ctx), server_challenge, len);
>>> - if (rc) {
>>> - ksmbd_debug(AUTH, "Could not update with challenge\n");
>>> - goto out;
>>> - }
>>> -
>>> - rc = crypto_shash_update(CRYPTO_MD5(ctx), nonce, len);
>>> - if (rc) {
>>> - ksmbd_debug(AUTH, "Could not update with nonce\n");
>>> - goto out;
>>> - }
>>> -
>>> - rc = crypto_shash_final(CRYPTO_MD5(ctx), md5_hash);
>>> - if (rc)
>>> - ksmbd_debug(AUTH, "Could not generate md5 hash\n");
>>> -out:
>>> - ksmbd_release_crypto_ctx(ctx);
>>> - return rc;
>>> -}
>>> -
>>> /**
>>> * ksmbd_gen_sess_key() - function to generate session key
>>> * @sess: session of connection
>>> @@ -324,43 +205,6 @@ static int calc_ntlmv2_hash(struct ksmbd_session
>>> *sess, char *ntlmv2_hash,
>>> return ret;
>>> }
>>>
>>> -/**
>>> - * ksmbd_auth_ntlm() - NTLM authentication handler
>>> - * @sess: session of connection
>>> - * @pw_buf: NTLM challenge response
>>> - * @passkey: user password
>>> - *
>>> - * Return: 0 on success, error number on error
>>> - */
>>> -int ksmbd_auth_ntlm(struct ksmbd_session *sess, char *pw_buf)
>>> -{
>>> - int rc;
>>> - unsigned char p21[21];
>>> - char key[CIFS_AUTH_RESP_SIZE];
>>> -
>>> - memset(p21, '\0', 21);
>>> - memcpy(p21, user_passkey(sess->user), CIFS_NTHASH_SIZE);
>>> - rc = ksmbd_enc_p24(p21, sess->ntlmssp.cryptkey, key);
>>> - if (rc) {
>>> - pr_err("password processing failed\n");
>>> - return rc;
>>> - }
>>> -
>>> - ksmbd_enc_md4(sess->sess_key, user_passkey(sess->user),
>>> - CIFS_SMB1_SESSKEY_SIZE);
>>> - memcpy(sess->sess_key + CIFS_SMB1_SESSKEY_SIZE, key,
>>> - CIFS_AUTH_RESP_SIZE);
>>> - sess->sequence_number = 1;
>>> -
>>> - if (strncmp(pw_buf, key, CIFS_AUTH_RESP_SIZE) != 0) {
>>> - ksmbd_debug(AUTH, "ntlmv1 authentication failed\n");
>>> - return -EINVAL;
>>> - }
>>> -
>>> - ksmbd_debug(AUTH, "ntlmv1 authentication pass\n");
>>> - return 0;
>>> -}
>>> -
>>> /**
>>> * ksmbd_auth_ntlmv2() - NTLMv2 authentication handler
>>> * @sess: session of connection
>>> @@ -441,44 +285,6 @@ int ksmbd_auth_ntlmv2(struct ksmbd_session *sess,
>>> struct ntlmv2_resp *ntlmv2,
>>> return rc;
>>> }
>>>
>>> -/**
>>> - * __ksmbd_auth_ntlmv2() - NTLM2(extended security) authentication
>>> handler
>>> - * @sess: session of connection
>>> - * @client_nonce: client nonce from LM response.
>>> - * @ntlm_resp: ntlm response data from client.
>>> - *
>>> - * Return: 0 on success, error number on error
>>> - */
>>> -static int __ksmbd_auth_ntlmv2(struct ksmbd_session *sess, char
>>> *client_nonce,
>>> - char *ntlm_resp)
>>> -{
>>> - char sess_key[CIFS_SMB1_SESSKEY_SIZE] = {0};
>>> - int rc;
>>> - unsigned char p21[21];
>>> - char key[CIFS_AUTH_RESP_SIZE];
>>> -
>>> - rc = ksmbd_enc_update_sess_key(sess_key,
>>> - client_nonce,
>>> - (char *)sess->ntlmssp.cryptkey, 8);
>>> - if (rc) {
>>> - pr_err("password processing failed\n");
>>> - goto out;
>>> - }
>>> -
>>> - memset(p21, '\0', 21);
>>> - memcpy(p21, user_passkey(sess->user), CIFS_NTHASH_SIZE);
>>> - rc = ksmbd_enc_p24(p21, sess_key, key);
>>> - if (rc) {
>>> - pr_err("password processing failed\n");
>>> - goto out;
>>> - }
>>> -
>>> - if (memcmp(ntlm_resp, key, CIFS_AUTH_RESP_SIZE) != 0)
>>> - rc = -EINVAL;
>>> -out:
>>> - return rc;
>>> -}
>>> -
>>> /**
>>> * ksmbd_decode_ntlmssp_auth_blob() - helper function to construct
>>> * authenticate blob
>>> @@ -512,17 +318,6 @@ int ksmbd_decode_ntlmssp_auth_blob(struct
>>> authenticate_message *authblob,
>>> nt_off = le32_to_cpu(authblob->NtChallengeResponse.BufferOffset);
>>> nt_len = le16_to_cpu(authblob->NtChallengeResponse.Length);
>>>
>>> - /* process NTLM authentication */
>>> - if (nt_len == CIFS_AUTH_RESP_SIZE) {
>>> - if (le32_to_cpu(authblob->NegotiateFlags) &
>>> - NTLMSSP_NEGOTIATE_EXTENDED_SEC)
>>> - return __ksmbd_auth_ntlmv2(sess, (char *)authblob +
>>> - lm_off, (char *)authblob + nt_off);
>>> - else
>>> - return ksmbd_auth_ntlm(sess, (char *)authblob +
>>> - nt_off);
>>> - }
>>> -
>>> /* TODO : use domain name that imported from configuration file */
>>> domain_name = smb_strndup_from_utf16((const char *)authblob +
>>> le32_to_cpu(authblob->DomainName.BufferOffset),
>>> diff --git a/fs/ksmbd/crypto_ctx.c b/fs/ksmbd/crypto_ctx.c
>>> index 5f4b1008d17e..81488d04199d 100644
>>> --- a/fs/ksmbd/crypto_ctx.c
>>> +++ b/fs/ksmbd/crypto_ctx.c
>>> @@ -81,12 +81,6 @@ static struct shash_desc *alloc_shash_desc(int id)
>>> case CRYPTO_SHASH_SHA512:
>>> tfm = crypto_alloc_shash("sha512", 0, 0);
>>> break;
>>> - case CRYPTO_SHASH_MD4:
>>> - tfm = crypto_alloc_shash("md4", 0, 0);
>>> - break;
>>> - case CRYPTO_SHASH_MD5:
>>> - tfm = crypto_alloc_shash("md5", 0, 0);
>>> - break;
>>> default:
>>> return NULL;
>>> }
>>> @@ -214,16 +208,6 @@ struct ksmbd_crypto_ctx
>>> *ksmbd_crypto_ctx_find_sha512(void)
>>> return ____crypto_shash_ctx_find(CRYPTO_SHASH_SHA512);
>>> }
>>>
>>> -struct ksmbd_crypto_ctx *ksmbd_crypto_ctx_find_md4(void)
>>> -{
>>> - return ____crypto_shash_ctx_find(CRYPTO_SHASH_MD4);
>>> -}
>>> -
>>> -struct ksmbd_crypto_ctx *ksmbd_crypto_ctx_find_md5(void)
>>> -{
>>> - return ____crypto_shash_ctx_find(CRYPTO_SHASH_MD5);
>>> -}
>>> -
>>> static struct ksmbd_crypto_ctx *____crypto_aead_ctx_find(int id)
>>> {
>>> struct ksmbd_crypto_ctx *ctx;
>>> diff --git a/fs/ksmbd/crypto_ctx.h b/fs/ksmbd/crypto_ctx.h
>>> index ef11154b43df..4a367c62f653 100644
>>> --- a/fs/ksmbd/crypto_ctx.h
>>> +++ b/fs/ksmbd/crypto_ctx.h
>>> @@ -15,8 +15,6 @@ enum {
>>> CRYPTO_SHASH_CMACAES,
>>> CRYPTO_SHASH_SHA256,
>>> CRYPTO_SHASH_SHA512,
>>> - CRYPTO_SHASH_MD4,
>>> - CRYPTO_SHASH_MD5,
>>> CRYPTO_SHASH_MAX,
>>> };
>>>
>>> @@ -43,8 +41,6 @@ struct ksmbd_crypto_ctx {
>>> #define CRYPTO_CMACAES(c) ((c)->desc[CRYPTO_SHASH_CMACAES])
>>> #define CRYPTO_SHA256(c) ((c)->desc[CRYPTO_SHASH_SHA256])
>>> #define CRYPTO_SHA512(c) ((c)->desc[CRYPTO_SHASH_SHA512])
>>> -#define CRYPTO_MD4(c) ((c)->desc[CRYPTO_SHASH_MD4])
>>> -#define CRYPTO_MD5(c) ((c)->desc[CRYPTO_SHASH_MD5])
>>>
>>> #define CRYPTO_HMACMD5_TFM(c) ((c)->desc[CRYPTO_SHASH_HMACMD5]->tfm)
>>> #define CRYPTO_HMACSHA256_TFM(c)\
>>> @@ -52,8 +48,6 @@ struct ksmbd_crypto_ctx {
>>> #define CRYPTO_CMACAES_TFM(c) ((c)->desc[CRYPTO_SHASH_CMACAES]->tfm)
>>> #define CRYPTO_SHA256_TFM(c) ((c)->desc[CRYPTO_SHASH_SHA256]->tfm)
>>> #define CRYPTO_SHA512_TFM(c) ((c)->desc[CRYPTO_SHASH_SHA512]->tfm)
>>> -#define CRYPTO_MD4_TFM(c) ((c)->desc[CRYPTO_SHASH_MD4]->tfm)
>>> -#define CRYPTO_MD5_TFM(c) ((c)->desc[CRYPTO_SHASH_MD5]->tfm)
>>>
>>> #define CRYPTO_GCM(c) ((c)->ccmaes[CRYPTO_AEAD_AES_GCM])
>>> #define CRYPTO_CCM(c) ((c)->ccmaes[CRYPTO_AEAD_AES_CCM])
>>> @@ -64,8 +58,6 @@ struct ksmbd_crypto_ctx
>>> *ksmbd_crypto_ctx_find_hmacsha256(void);
>>> struct ksmbd_crypto_ctx *ksmbd_crypto_ctx_find_cmacaes(void);
>>> struct ksmbd_crypto_ctx *ksmbd_crypto_ctx_find_sha512(void);
>>> struct ksmbd_crypto_ctx *ksmbd_crypto_ctx_find_sha256(void);
>>> -struct ksmbd_crypto_ctx *ksmbd_crypto_ctx_find_md4(void);
>>> -struct ksmbd_crypto_ctx *ksmbd_crypto_ctx_find_md5(void);
>>> struct ksmbd_crypto_ctx *ksmbd_crypto_ctx_find_gcm(void);
>>> struct ksmbd_crypto_ctx *ksmbd_crypto_ctx_find_ccm(void);
>>> void ksmbd_crypto_destroy(void);
>>>
>>
>