From: Stefan Metzmacher <metze@samba.org>
To: Namjae Jeon <linkinjeon@kernel.org>
Cc: Marios Makassikis <mmakassikis@freebox.fr>,
linux-cifsd-devel@lists.sourceforge.net,
"linux-cifs@vger.kernel.org" <linux-cifs@vger.kernel.org>
Subject: Re: [Linux-cifsd-devel] [PATCH] cifsd: Do not use 0 as TreeID
Date: Sat, 15 May 2021 16:27:38 +0200 [thread overview]
Message-ID: <d5f40ae9-ba89-3bad-1ed8-c4fe672bb0b9@samba.org> (raw)
In-Reply-To: <CAKYAXd_64YRcho0Qnq+ccezVL7Tu2_9Jjb8PM+GDujZ9h8x6xw@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 5164 bytes --]
Am 15.05.21 um 16:10 schrieb Namjae Jeon:
> 2021-05-15 17:57 GMT+09:00, Stefan Metzmacher <metze@samba.org>:
>>
>> Am 15.05.21 um 07:18 schrieb Namjae Jeon:
>>> 2021-05-14 22:11 GMT+09:00, Stefan Metzmacher via Linux-cifsd-devel
>>> <linux-cifsd-devel@lists.sourceforge.net>:
>>>>
>>>> Am 14.05.21 um 14:52 schrieb Marios Makassikis:
>>>>> Returning TreeID=0 is valid behaviour according to [MS-SMB2] 2.2.1.2:
>>>>>
>>>>> TreeId (4 bytes): Uniquely identifies the tree connect for the
>>>>> command.
>>>>> This MUST be 0 for the SMB2 TREE_CONNECT Request. The TreeId can be
>>>>> any unsigned 32-bit integer that is received from a previous
>>>>> SMB2 TREE_CONNECT Response. TreeId SHOULD be set to 0 for the
>>>>> following commands:
>>>>> [...]
>>>>>
>>>>> However, some client implementations reject it as invalid. Windows 7/10
>>>>> assigns ids starting from 1, and samba4 returns a random uint32_t
>>>>> which suggests there may be other clients that consider it is
>>>>> invalid behaviour.
>>>>>
>>>>> While here, simplify ksmbd_acquire_smb2_tid. 0xFFFF is a reserved value
>>>>> for CIFS/SMB1:
>>>>> [MS-CIFS] 2.2.4.50.2
>>>>>
>>>>> TID (2 bytes): The newly generated Tree ID, used in subsequent CIFS
>>>>> client requests to refer to a resource relative to the
>>>>> SMB_Data.Bytes.Path specified in the request. Most access to the
>>>>> server requires a valid TID, whether the resource is password
>>>>> protected or not. The value 0xFFFF is reserved; the server MUST NOT
>>>>> return a TID value of 0xFFFF.
>>>>>
>>>>> Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
>>>>> ---
>>>>> Example library that treats zero TreeID as invalid:
>>>>>
>>>>> https://github.com/AgNO3/jcifs-ng/blob/master/src/main/java/jcifs/internal/smb2/tree/Smb2TreeConnectResponse.java#L201
>>>>>
>>>>> mgmt/ksmbd_ida.c | 9 ++-------
>>>>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/mgmt/ksmbd_ida.c b/mgmt/ksmbd_ida.c
>>>>> index 7eb6476..34e0d2e 100644
>>>>> --- a/mgmt/ksmbd_ida.c
>>>>> +++ b/mgmt/ksmbd_ida.c
>>>>> @@ -13,19 +13,14 @@ static inline int __acquire_id(struct ida *ida, int
>>>>> from, int to)
>>>>> #ifdef CONFIG_SMB_INSECURE_SERVER
>>>>> int ksmbd_acquire_smb1_tid(struct ida *ida)
>>>>> {
>>>>> - return __acquire_id(ida, 0, 0xFFFF);
>>>>> + return __acquire_id(ida, 1, 0xFFFF);
>>>>> }
>>>>> #endif
>>>>>
>>>>> int ksmbd_acquire_smb2_tid(struct ida *ida)
>>>>> {
>>>>> - int id;
>>>>> + return __acquire_id(ida, 1, 0);
>>>>
>>>> I think that should be __acquire_id(ida, 1, 0xFFFFFFFF) (or a lower
>>>> constraint)
>>>>
>>>> 0xFFFFFFFF is used for compound requests to inherit the tree id from the
>>>> previous request.
>>> Where is it defined in the specification ? As I know,
>>> SMB2_FLAGS_RELATED_OPERATIONS flags in smb header indicate inherit
>>> tree id in previous request.
>>
>> [MS-SMB2] 3.2.4.1.4 Sending Compounded Requests
>>
>> ...
>>
>> The client MUST construct the subsequent request as it would do normally.
>> For any subsequent
>> requests the client MUST set SMB2_FLAGS_RELATED_OPERATIONS in the Flags
>> field of the SMB2
>> header to indicate that it is using the SessionId, TreeId, and FileId
>> supplied in the previous
>> request (or generated by the server in processing that request). For an
>> operation compounded
>> with an SMB2 CREATE request, the FileId field SHOULD be set to {
>> 0xFFFFFFFFFFFFFFFF,
>> 0xFFFFFFFFFFFFFFFF }.
>>
>> This only explicitly talks about FileId and I'm not any client would do
>> that, but in theory it should be possible to
>> compound, the 2nd session setup request (of an anonymous authentication)
>> with a tree connect request
>> and an open.
>>
>> Which means it's the safest behavior for a server to avoid 0 and all F as
>> valid id,
>> there're still enough ids to use....
>>
>> It also makes sure that we don't end up with very confusing network
>> captures.
> Okay, I have checked cifs client code like the following.
>
> if (request_type & CHAINED_REQUEST) {
> if (!(request_type & END_OF_CHAIN)) {
> /* next 8-byte aligned request */
> *total_len = DIV_ROUND_UP(*total_len, 8) * 8;
> shdr->NextCommand = cpu_to_le32(*total_len);
> } else /* END_OF_CHAIN */
> shdr->NextCommand = 0;
> if (request_type & RELATED_REQUEST) {
> shdr->Flags |= SMB2_FLAGS_RELATED_OPERATIONS;
> /*
> * Related requests use info from previous read request
> * in chain.
> */
> shdr->SessionId = 0xFFFFFFFF;
> shdr->TreeId = 0xFFFFFFFF;
> req->PersistentFileId = 0xFFFFFFFF;
> req->VolatileFileId = 0xFFFFFFFF;
> }
Which seems actually wrong and should be 0xFFFFFFFFFFFFFFFF for all but TreeId...
metze
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next parent reply other threads:[~2021-05-15 14:27 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20210514125201.560775-1-mmakassikis@freebox.fr>
[not found] ` <f053a063-c8e2-14c8-2c4f-a34c10c39fa1@samba.org>
[not found] ` <CAKYAXd9xWW9VJ=EPrhgVUP+ES04zOnHcy4rkboAVYeOuE=bX=w@mail.gmail.com>
[not found] ` <e03eb8d7-e964-5fa5-840d-9d3292d6d03f@samba.org>
[not found] ` <CAKYAXd_64YRcho0Qnq+ccezVL7Tu2_9Jjb8PM+GDujZ9h8x6xw@mail.gmail.com>
2021-05-15 14:27 ` Stefan Metzmacher [this message]
2021-05-15 14:48 ` [Linux-cifsd-devel] [PATCH] cifsd: Do not use 0 as TreeID Namjae Jeon
2021-05-15 14:59 ` Steve French
2021-05-19 12:02 ` Shyam Prasad N
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d5f40ae9-ba89-3bad-1ed8-c4fe672bb0b9@samba.org \
--to=metze@samba.org \
--cc=linkinjeon@kernel.org \
--cc=linux-cifs@vger.kernel.org \
--cc=linux-cifsd-devel@lists.sourceforge.net \
--cc=mmakassikis@freebox.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).