SMB1 fixes by mmakassikis · Pull Request #399 · namjaejeon/ksmbd

mmakassikis · 2022-05-05T00:04:45Z

A few fixes SMB1 fixes:

ksmbd: smb1: remove unused parameter in find_next()
ksmbd: smb1: use kasprintf() instead of kzalloc+snprintf+strncat
ksmbd: smb1: Fix potential memleak in smb_nt_create_andx
ksmbd: smb1: add buffer validation

The first 3 ones small fixes. The last one is big, but it is essentially the same pattern repeated over and over again.

I tried sending to the linux-cifsd-devel list, but it looks like the project has been deleted from sourceforge.

namjaejeon · 2022-05-10T08:20:29Z

auth.c

 * @pw_buf:	NTLM challenge response
- * @passkey:	user password
+ * @pw_len:	buffer length
+ * @cryptkey:	buffer length


- * @cryptkey: buffer length + * @cryptkey: server challenge

auth.c

namjaejeon · 2022-05-10T22:51:24Z

I have applied 3 patches first. I need more time to review "ksmbd: smb1: add buffer validation" patch.

namjaejeon · 2022-06-24T07:05:29Z

@mmakassikis Sorry for checking "ksmbd: smb1: add buffer validation" patch. I will check it on weekend.
@hclee Hyunchul, please join to review it together.

hclee · 2022-06-24T07:18:00Z

@mmakassikis Sorry for checking "ksmbd: smb1: add buffer validation" patch. I will check it on weekend. @hclee Hyunchul, please join to review it together.

Okay, I will review this patch on weekend.

namjaejeon · 2022-06-26T12:36:10Z

smb1pdu.c

-		pr_err("Unable to strdup() treename or devtype uid %d\n",
-		       rsp_hdr->Uid);
+	offset += offsetof(struct smb_com_tconx_req, Password);
+	offset += le16_to_cpu(req->PasswordLength);


Have you tested if req->PasswordLength is zero ?

If req->PasswordLength is zero, offset is unchanged (incidentally, the current code works because req->PasswordLength == le16_to_cpu(req->PasswordLength)).

I don't see what problems can arise with req->PasswordLength.

smb1pdu.c

namjaejeon · 2022-06-26T12:45:42Z

smb1pdu.c

 	sz = le16_to_cpu(req->SecurityBlobLength);

+	if (offsetof(struct smb_com_session_setup_req, SecurityBlob) + sz >
+		get_rfc1002_len(req)) {


This check can be moved to smb1misc.c ?

namjaejeon · 2022-06-26T14:35:44Z

@mmakassikis When I checked smb1 buffer validation patch, You need to move some of validation codes to smb1misc.c. I think that you can check codes in smb2misc.c

hclee · 2022-06-26T23:51:15Z

smb1pdu.c

 	struct andx_block *next;

+	/* AndXOffset does not include 4 byte RFC1002 header */
+	len -= 4;


Does the "len" include 4-byte RFC1002 header?

buf is the full request (RFC1002 header, followed by SMB2 header and payload). len should those 4 bytes.

The code here is correct, but the caller code is wrong as it uses get_rfc1002_len() which is the buffer length minus 4.
This mistake is present in most if not all changes in this patch.

hclee · 2022-06-27T00:13:07Z

smb1pdu.c

 	}

+	offset += oldname_len + 2;
+	if (offset > maxlen) {


instead of offset > maxlen, offset >= maxlen?

Yes. Actually, I thought it was ok as we passed a calculated length to smb_get_name(), but if the request is malformed, it's possible to have offset > maxlen, in which case the negative "maxlen - offset" will cause an infinite loop in smb_utf16_bytes().

I will update the patch to check offset >= maxlen before calling smb_get_name().

smb1pdu.c

hclee · 2022-06-27T01:33:50Z

smb1pdu.c

 		setup_bytes_count = 2 * req->SetupCount;

+	maxlen = get_rfc1002_len(req);
+	offset = offsetof(struct smb_com_trans_req, Data);


Doesn't 1-byte, smb_com_trans_req.Data[1] need to be considered?

I don't understand.

Currently, the code parses up to 256 bytes starting from req->Data + setup_bytes_count. My change initializes the offset to req->Data. Whether the Data field is Data[0] or Data[1] doesn't matter, no ?

buf_len is never used anywhere. Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>

- Unset spnego bit in SMB negotiate response. - Include NativeOS / Native LAN Manager / Primary Domain fields in session setup response. Without these, smbclient fails with NT_STATUS_BUFFER_TOO_SMALL. Tested with smbclient: smbclient //127.0.0.1/testshare -U user%password \ --option "client min protocol = NT1" \ --option "client use spnego = no" \ -m NT1 Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>

flock is leaked in an error happens before smb_lock_init(). Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>

Many functions assume the client sends well formed packets, or are optimistic about the buffer length (using PATH_MAX or a fixed '256' value) instead of the actual length. The pattern followed to fix this boils down to extracting the total buffer length and calculate how much data has been consumed. Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>

mmakassikis · 2022-11-29T12:20:36Z

Updated "ksmbd: smb1: Fix resource leak in smb_locking_andx()" as there were other paths were flock was leaked

namjaejeon · 2022-12-08T14:09:30Z

@mmakassikis I have applied 4 patches first. Could you please tell me how you test "ksmbd: smb1: add buffer validation" patch ? and this patch was already applied to your product ?

mmakassikis · 2022-12-08T17:02:26Z

The buffer validation patch has not yet been applied for our product. The changes themselves are quite repetitive but it's easy to make mistakes / forget a check (as shown by the previous version of the patch that you and Hyunchul reviewed). An extra pair of eyes is always welcome.

I tested the patchset manually for the most part (which is how I saw non-spnego session setup was broken). I tested the following operations with smbclient / cifs.ko:

create dir/file
list directory contents
read/write/rename file
delete dir

namjaejeon · 2022-12-20T23:19:11Z

@mmakassikis Okay, Let me check it:) Thanks!

mmakassikis force-pushed the marios/smb1-fixes branch from 935ba12 to 94d95ac Compare May 5, 2022 09:00

namjaejeon force-pushed the master branch from 3af1ea4 to ab578f0 Compare May 8, 2022 14:23

namjaejeon reviewed May 10, 2022

View reviewed changes

auth.c Outdated Show resolved Hide resolved

namjaejeon force-pushed the master branch 2 times, most recently from 80f76a4 to cdd2c68 Compare May 16, 2022 07:35

namjaejeon force-pushed the master branch 2 times, most recently from 598c4b4 to 9ef42e9 Compare June 19, 2022 00:10

namjaejeon reviewed Jun 26, 2022

View reviewed changes

hclee reviewed Jun 27, 2022

View reviewed changes

namjaejeon force-pushed the master branch 7 times, most recently from d055654 to 7202068 Compare July 25, 2022 04:06

namjaejeon force-pushed the master branch 9 times, most recently from c683c7d to 04bf112 Compare July 29, 2022 00:31

Marios Makassikis added 2 commits November 23, 2022 18:45

ksmbd: smb1: remove unused variable from smb_trans()

35f2337

buf_len is never used anywhere. Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>

mmakassikis force-pushed the marios/smb1-fixes branch from 87d3bda to a9a2348 Compare November 23, 2022 17:46

Marios Makassikis added 2 commits November 29, 2022 13:16

ksmbd: smb1: Fix resource leak in smb_locking_andx()

5738707

flock is leaked in an error happens before smb_lock_init(). Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>

mmakassikis force-pushed the marios/smb1-fixes branch from a9a2348 to cff4b20 Compare November 29, 2022 12:18

namjaejeon force-pushed the master branch from 7c348d5 to 978b567 Compare December 8, 2022 13:59

namjaejeon force-pushed the master branch from e0299d3 to 897c742 Compare January 4, 2023 14:14

namjaejeon force-pushed the master branch 4 times, most recently from 2fb0a29 to cdc5e9b Compare January 18, 2023 15:04

namjaejeon force-pushed the master branch 3 times, most recently from fa0ee90 to 76139ec Compare January 30, 2023 12:31

namjaejeon force-pushed the master branch from 02a5879 to 4c7db66 Compare February 16, 2023 02:55

namjaejeon force-pushed the master branch from d7da149 to bda813c Compare March 2, 2023 23:40

namjaejeon force-pushed the master branch from 5d16eda to 16163fa Compare March 13, 2023 02:39

namjaejeon force-pushed the master branch 2 times, most recently from e0e6d38 to fe3d6cd Compare March 21, 2023 01:35

namjaejeon force-pushed the master branch 2 times, most recently from 5a8106d to eccccf4 Compare March 30, 2023 09:50

namjaejeon force-pushed the master branch 4 times, most recently from 88eefd8 to ba30fd2 Compare April 29, 2023 23:57

Conversation

mmakassikis commented May 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

namjaejeon commented May 10, 2022

Uh oh!

namjaejeon commented Jun 24, 2022

Uh oh!

hclee commented Jun 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namjaejeon commented Jun 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmakassikis commented Nov 29, 2022

Uh oh!

namjaejeon commented Dec 8, 2022

Uh oh!

mmakassikis commented Dec 8, 2022

Uh oh!

namjaejeon commented Dec 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants