Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not set SMB2_GLOBAL_CAP_ENCRYPTION for SMB 3.1.1. Fixes #550 #551

Closed
wants to merge 1 commit into from
Closed

Do not set SMB2_GLOBAL_CAP_ENCRYPTION for SMB 3.1.1. Fixes #550 #551

wants to merge 1 commit into from

Conversation

socram8888
Copy link

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 caused Windows 10 to detect a mistake in the protocol, and disable
encryption despite being enabled in the settings.

@hclee
Copy link
Member

hclee commented Dec 15, 2021

Could you send this patch to linux-cifs@vger.kernel.org?
And the prefix, "ksmbd: " has to be added to the commit title.

@@ -323,9 +323,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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the root cause of your encryption problem?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, Looks great!

smb2pdu.c Outdated
* SMB 3.1.1 uses the cipher_type field.
*/
encrequested = conn->cipher_type ||
(conn->vals->capabilities & SMB2_GLOBAL_CAP_ENCRYPTION);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need to use encrequested variable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created the variable to make clearer the conditions on which the encryption applied, since there were already a sizeable amount of operands in the if statement.

Mixing complex and, ors and bit-twiddling in the same if would be a big no for me since it requires thinking about C's operand priorities, but if you'd prefer I can change it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the logic into a function, which makes it IMHO clearer and reduces duplicate code.

I'll send it now to the LKML for further review.

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.
*
* Return: true if should be encrypted, else false
*/
static bool should_encrypt(struct ksmbd_conn *conn)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this naming sense. how about smb3_encryption_negotiated()

@socram8888
Copy link
Author

I've just sent the patch to the LKML, so I am gonna close this PR :)

@socram8888 socram8888 closed this Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants