MIME-Version: 1.0
In-Reply-To: <20211216003119.1609352-1-marcos@orca.pet>
References: <20211216003119.1609352-1-marcos@orca.pet>
From:   Namjae Jeon <linkinjeon@kernel.org>
Date:   Thu, 16 Dec 2021 10:14:36 +0900
Message-ID: <CAKYAXd-r14FNB-jKfCS+v69dgjxPBNdm109jRQ7BKxngN_qWTg@mail.gmail.com>
Subject: Re: [PATCH] ksmbd: disable SMB2_GLOBAL_CAP_ENCRYPTION for SMB 3.1.1
To:     Marcos Del Sol Vives <marcos@orca.pet>
Cc:     linux-cifs@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

2021-12-16 9:31 GMT+09:00, Marcos Del Sol Vives <marcos@orca.pet>:
> According to the official Microsoft MS-SMB2 document section 3.3.5.4, this
> flag should be used only for 3.0 and 3.0.2 dialects. Setting it for 3.1.1
> is a violation of the specification.
>
> This causes my Windows 10 client to detect an anomaly in the negotiation,
> and disable encryption entirely despite being explicitly enabled in ksmbd,
> causing all data transfers to go in plain text.
>
> Signed-off-by: Marcos Del Sol Vives <marcos@orca.pet>
> Cc: linux-kernel@vger.kernel.org
> Cc: Namjae Jeon <linkinjeon@kernel.org>
> ---
>  fs/ksmbd/smb2ops.c |  3 ---
>  fs/ksmbd/smb2pdu.c | 25 +++++++++++++++++++++----
>  2 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ksmbd/smb2ops.c b/fs/ksmbd/smb2ops.c
> index 0a5d8450e835..02a44d28bdaf 100644
> --- a/fs/ksmbd/smb2ops.c
> +++ b/fs/ksmbd/smb2ops.c
> @@ -271,9 +271,6 @@ int init_smb3_11_server(struct ksmbd_conn *conn)
>  	if (server_conf.flags & KSMBD_GLOBAL_FLAG_SMB2_LEASES)
>  		conn->vals->capabilities |= SMB2_GLOBAL_CAP_LEASING;
>
> -	if (conn->cipher_type)
> -		conn->vals->capabilities |= SMB2_GLOBAL_CAP_ENCRYPTION;
> -
>  	if (server_conf.flags & KSMBD_GLOBAL_FLAG_SMB3_MULTICHANNEL)
>  		conn->vals->capabilities |= SMB2_GLOBAL_CAP_MULTI_CHANNEL;
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 49c9da37315c..6193d5a1d653 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -915,6 +915,25 @@ static void decode_encrypt_ctxt(struct ksmbd_conn
> *conn,
>  	}
>  }
>
> +/**
> + * should_encrypt() - checks if connection should be encrypted
> + * @conn:	smb connection
> + *
> + * Return:	true if should be encrypted, else false
> + */
> +static bool should_encrypt(struct ksmbd_conn *conn)
Can you change function name to smb3_encryption_negotiated() ?
And need to update function description also.

Thanks for your patch!
> +{
> +	if (!conn->ops->generate_encryptionkey)
> +		return false;
> +
> +	/*
> +	 * SMB 3.0 and 3.0.2 dialects use the SMB2_GLOBAL_CAP_ENCRYPTION flag.
> +	 * SMB 3.1.1 uses the cipher_type field.
> +	 */
> +	return (conn->vals->capabilities & SMB2_GLOBAL_CAP_ENCRYPTION) ||
> +	    conn->cipher_type;
> +}
> +
>  static void decode_compress_ctxt(struct ksmbd_conn *conn,
>  				 struct smb2_compression_capabilities_context *pneg_ctxt)
>  {
> @@ -1469,8 +1488,7 @@ static int ntlm_authenticate(struct ksmbd_work *work)
>  		    (req->SecurityMode & SMB2_NEGOTIATE_SIGNING_REQUIRED))
>  			sess->sign = true;
>
> -		if (conn->vals->capabilities & SMB2_GLOBAL_CAP_ENCRYPTION &&
> -		    conn->ops->generate_encryptionkey &&
> +		if (should_encrypt(conn) &&
>  		    !(req->Flags & SMB2_SESSION_REQ_FLAG_BINDING)) {
>  			rc = conn->ops->generate_encryptionkey(sess);
>  			if (rc) {
> @@ -1559,8 +1577,7 @@ static int krb5_authenticate(struct ksmbd_work *work)
>  	    (req->SecurityMode & SMB2_NEGOTIATE_SIGNING_REQUIRED))
>  		sess->sign = true;
>
> -	if ((conn->vals->capabilities & SMB2_GLOBAL_CAP_ENCRYPTION) &&
> -	    conn->ops->generate_encryptionkey) {
> +	if (should_encrypt(conn)) {
>  		retval = conn->ops->generate_encryptionkey(sess);
>  		if (retval) {
>  			ksmbd_debug(SMB,
> --
> 2.25.1
>
>