cifsd-team / ksmbd

ksmbd kernel server(SMB/CIFS server)
151 stars 23 forks source link

Plaintext connection despite encryption enabled #550

Closed socram8888 closed 2 years ago

socram8888 commented 2 years ago

Hello. I am facing an error with the latest stable v3.4.2 version, running on an OpenWRT machine. My configuration file is as follows:

[global]
        netbios name = wd-casa
        server string = Ksmbd on OpenWrt
        workgroup = WORKGROUP
        ipc timeout = 20
        deadtime = 15
        smb2 max read = 64K
        smb2 max write = 64K
        smb2 max trans = 64K
        smb3 encryption = yes
        restrict anonymous = 1
        server signing = required
        server min protocol = SMB3_11

######### Dynamic written config options #########

[marcos]
        path = /mnt/data/marcos
        force user = root
        force group = root
        create mask = 0666
        directory mask = 0777
        read only = no
        guest ok = no

Despite "smb3 encryption" set to yes, communications are still being made in plain text according to Wireshark.

During the initial negotiation, both the client (a Windows 10 machine running 21H1, compilation 19043.1348) and the server agree on using 3.1.1 and AES-128-GCM: imagen

On the the session setup request, however, the client decides to no longer flag it supports encryption, and thus request a plain text connection for unknown reasons: imagen

Worse is, ksmbd agrees and goes on, ignoring the request to enforce encryption and just sending data on plain. The following screenshot displays a plain text ASCII message from opening a file which shouldn't be visible: imagen

I am not sure if being made in plain text is an error on the Windows side or not, but at the very least ksmbd should probably deny connections if the user requested encryption in the settings.

Attached here is a .zip file with a Wireshark log of the entire packet exchange, from the initial connection to the desconnection: comms.zip

namjaejeon commented 2 years ago

It work fine to me. don't know why windows client doesn't send encrypted request since session setup. I can not find any clue in your dump. Can you check it against samba ?

socram8888 commented 2 years ago

Samba is working fine: imagen This is a log of me connecting to another machine running Debian Bullseye: comms-samba.zip

I've found a small issue I think it could be the culprit, and it is that the 3.1.1 handler in smb2ops.c mistakengly sets the SMB2_GLOBAL_CAP_ENCRYPTION flag on the negotiation response which Samba does not.

According to Microsoft's SMB2 and 3 protocol standard section 2.2.4 in page 54, this flag should not be set:

When set, indicates that the server supports encryption. This flag is valid for the SMB 3.0 and 3.0.2 dialects.

namjaejeon commented 2 years ago

I've found a small issue I think it could be the culprit, and it is that the 3.1.1 handler in smb2ops.c mistakengly sets the SMB2_GLOBAL_CAP_ENCRYPTION flag on the negotiation response which Samba does not.

Okay, Can you check problem is fixed after removing this lines you found ?

socram8888 commented 2 years ago

I already thought so but that disables encryption completely, as smb2pdu.c uses it as a condition to enable AES keygen.

Further modifying the code (https://github.com/socram8888/ksmbd/tree/no-enc-flag) seems quite promising, as the client seems to indeed request encryption, but I am facing a kernel crash after these changes which I am still not sure if related or not to my changes. I'll try on a non-embedded device for easier debugging, since I can't read the dmesg to know the reason.

socram8888 commented 2 years ago

The crash seems to be caused by something else, as the modded ksmbd module loads and runs fine on an Alpine installation, so I've opened #551 to merge the changes.

I've also checked that this bug does not affect 3.4.2 if using dialects 3.0 or 3.0.2, which do use the flag and the session is properly encrypted, so as a mitigation for anyone affected you might wanna do that.

socram8888 commented 2 years ago

Closing this, as the patch will be merged with Linux kernel instead: https://lkml.org/lkml/2021/12/16/336

namjaejeon commented 2 years ago

Thanks for your work and I will apply this patch to out of tree ksmbd soon.

socram8888 commented 2 years ago

Awesome. Thank you for creating ksmbd as well :)