* [PATCH] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message
@ 2022-08-24 8:57 Zhang Xiaoxu
2022-08-30 7:06 ` huaweicloud
0 siblings, 1 reply; 8+ messages in thread
From: Zhang Xiaoxu @ 2022-08-24 8:57 UTC (permalink / raw)
To: linux-cifs, zhangxiaoxu5, sfrench, pc, lsahlber, sprasad, rohiths
Commit d5c7076b772a ("smb3: add smb3.1.1 to default dialect list")
extend the dialects from 3 to 4, but forget to decrease the extended
length when specific the dialect, then the message length is larger
than expected.
This maybe leak some info through network because not initialize the
message body.
After apply this patch, the VALIDATE_NEGOTIATE_INFO message length is
reduced from 28 bytes to 26 bytes.
Fixes: d5c7076b772a ("smb3: add smb3.1.1 to default dialect list")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
Cc: <stable@vger.kernel.org>
---
fs/cifs/smb2pdu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 1ecc2501c56f..3df7adc01fe5 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1162,9 +1162,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
pneg_inbuf->Dialects[0] =
cpu_to_le16(server->vals->protocol_id);
pneg_inbuf->DialectCount = cpu_to_le16(1);
- /* structure is big enough for 3 dialects, sending only 1 */
+ /* structure is big enough for 4 dialects, sending only 1 */
inbuflen = sizeof(*pneg_inbuf) -
- sizeof(pneg_inbuf->Dialects[0]) * 2;
+ sizeof(pneg_inbuf->Dialects[0]) * 3;
}
rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
--
2.31.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message 2022-08-24 8:57 [PATCH] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message Zhang Xiaoxu @ 2022-08-30 7:06 ` huaweicloud 2022-08-30 14:03 ` Tom Talpey 0 siblings, 1 reply; 8+ messages in thread From: huaweicloud @ 2022-08-30 7:06 UTC (permalink / raw) To: stfrench; +Cc: linux-cifs, sfrench, lsahlber, sprasad, rohiths, pc Hi Steve, Could you help to review this patch. Thanks, Zhang Xiaoxu 在 2022/8/24 16:57, Zhang Xiaoxu 写道: > Commit d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") > extend the dialects from 3 to 4, but forget to decrease the extended > length when specific the dialect, then the message length is larger > than expected. > > This maybe leak some info through network because not initialize the > message body. > > After apply this patch, the VALIDATE_NEGOTIATE_INFO message length is > reduced from 28 bytes to 26 bytes. > > Fixes: d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") > Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> > Cc: <stable@vger.kernel.org> > --- > fs/cifs/smb2pdu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 1ecc2501c56f..3df7adc01fe5 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -1162,9 +1162,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) > pneg_inbuf->Dialects[0] = > cpu_to_le16(server->vals->protocol_id); > pneg_inbuf->DialectCount = cpu_to_le16(1); > - /* structure is big enough for 3 dialects, sending only 1 */ > + /* structure is big enough for 4 dialects, sending only 1 */ > inbuflen = sizeof(*pneg_inbuf) - > - sizeof(pneg_inbuf->Dialects[0]) * 2; > + sizeof(pneg_inbuf->Dialects[0]) * 3; > } > > rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID, ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message 2022-08-30 7:06 ` huaweicloud @ 2022-08-30 14:03 ` Tom Talpey 2022-08-30 14:30 ` huaweicloud 0 siblings, 1 reply; 8+ messages in thread From: Tom Talpey @ 2022-08-30 14:03 UTC (permalink / raw) To: huaweicloud, stfrench; +Cc: linux-cifs, sfrench, lsahlber, sprasad, rohiths, pc Wouldn't it be safer to just set the size, instead of implicitly assuming that there are 4 array elements? inbuflen = sizeof(*pneg_inbuf) - sizeof(pneg_inbuf.Dialects) + sizeof(pneg_inbuf.Dialects[0]); Or, because it obviously works to send the extra bytes even though the DialectCount is just 1, just zero them out? memset(pneg_inbuf.Dialects, 0, sizeof(pneg_inbuf.Dialects)); pneg_inbuf.Dialects[0] = cpu_to_le16(server->vals->protocol_id); Tom. On 8/30/2022 3:06 AM, huaweicloud wrote: > Hi Steve, > > Could you help to review this patch. > > Thanks, > Zhang Xiaoxu > > 在 2022/8/24 16:57, Zhang Xiaoxu 写道: >> Commit d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") >> extend the dialects from 3 to 4, but forget to decrease the extended >> length when specific the dialect, then the message length is larger >> than expected. >> >> This maybe leak some info through network because not initialize the >> message body. >> >> After apply this patch, the VALIDATE_NEGOTIATE_INFO message length is >> reduced from 28 bytes to 26 bytes. >> >> Fixes: d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") >> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> >> Cc: <stable@vger.kernel.org> >> --- >> fs/cifs/smb2pdu.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >> index 1ecc2501c56f..3df7adc01fe5 100644 >> --- a/fs/cifs/smb2pdu.c >> +++ b/fs/cifs/smb2pdu.c >> @@ -1162,9 +1162,9 @@ int smb3_validate_negotiate(const unsigned int >> xid, struct cifs_tcon *tcon) >> pneg_inbuf->Dialects[0] = >> cpu_to_le16(server->vals->protocol_id); >> pneg_inbuf->DialectCount = cpu_to_le16(1); >> - /* structure is big enough for 3 dialects, sending only 1 */ >> + /* structure is big enough for 4 dialects, sending only 1 */ >> inbuflen = sizeof(*pneg_inbuf) - >> - sizeof(pneg_inbuf->Dialects[0]) * 2; >> + sizeof(pneg_inbuf->Dialects[0]) * 3; >> } >> rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID, > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message 2022-08-30 14:03 ` Tom Talpey @ 2022-08-30 14:30 ` huaweicloud 2022-08-30 16:19 ` Tom Talpey 0 siblings, 1 reply; 8+ messages in thread From: huaweicloud @ 2022-08-30 14:30 UTC (permalink / raw) To: Tom Talpey, stfrench; +Cc: linux-cifs, sfrench, lsahlber, sprasad, rohiths, pc I think the struct validate_negotiate_info_req is an variable-length array, just for implement simple, defind as 4 in here. According MS-SMB2, there really not define the length of the package, just define the count of the dialects, but just send the needed data maybe more appropriate. Thanks. 在 2022/8/30 22:03, Tom Talpey 写道: > Wouldn't it be safer to just set the size, instead of implicitly > assuming that there are 4 array elements? > > inbuflen = sizeof(*pneg_inbuf) - sizeof(pneg_inbuf.Dialects) + sizeof(pneg_inbuf.Dialects[0]); > > Or, because it obviously works to send the extra bytes even > though the DialectCount is just 1, just zero them out? > > memset(pneg_inbuf.Dialects, 0, sizeof(pneg_inbuf.Dialects)); > pneg_inbuf.Dialects[0] = cpu_to_le16(server->vals->protocol_id); > > Tom. > > On 8/30/2022 3:06 AM, huaweicloud wrote: >> Hi Steve, >> >> Could you help to review this patch. >> >> Thanks, >> Zhang Xiaoxu >> >> 在 2022/8/24 16:57, Zhang Xiaoxu 写道: >>> Commit d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") >>> extend the dialects from 3 to 4, but forget to decrease the extended >>> length when specific the dialect, then the message length is larger >>> than expected. >>> >>> This maybe leak some info through network because not initialize the >>> message body. >>> >>> After apply this patch, the VALIDATE_NEGOTIATE_INFO message length is >>> reduced from 28 bytes to 26 bytes. >>> >>> Fixes: d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") >>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> >>> Cc: <stable@vger.kernel.org> >>> --- >>> fs/cifs/smb2pdu.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >>> index 1ecc2501c56f..3df7adc01fe5 100644 >>> --- a/fs/cifs/smb2pdu.c >>> +++ b/fs/cifs/smb2pdu.c >>> @@ -1162,9 +1162,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) >>> pneg_inbuf->Dialects[0] = >>> cpu_to_le16(server->vals->protocol_id); >>> pneg_inbuf->DialectCount = cpu_to_le16(1); >>> - /* structure is big enough for 3 dialects, sending only 1 */ >>> + /* structure is big enough for 4 dialects, sending only 1 */ >>> inbuflen = sizeof(*pneg_inbuf) - >>> - sizeof(pneg_inbuf->Dialects[0]) * 2; >>> + sizeof(pneg_inbuf->Dialects[0]) * 3; >>> } >>> rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID, >> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message 2022-08-30 14:30 ` huaweicloud @ 2022-08-30 16:19 ` Tom Talpey 2022-08-31 2:03 ` huaweicloud 0 siblings, 1 reply; 8+ messages in thread From: Tom Talpey @ 2022-08-30 16:19 UTC (permalink / raw) To: huaweicloud, stfrench; +Cc: linux-cifs, sfrench, lsahlber, sprasad, rohiths, pc On 8/30/2022 10:30 AM, huaweicloud wrote: > I think the struct validate_negotiate_info_req is an variable-length array, > just for implement simple, defind as 4 in here. But that's my point. The code picks "4" arbitrarily, and that number can change - as you discovered, it already did since it used to be "3". smb2pdu.h: struct validate_negotiate_info_req { __le32 Capabilities; __u8 Guid[SMB2_CLIENT_GUID_SIZE]; __le16 SecurityMode; __le16 DialectCount; __le16 Dialects[4]; /* BB expand this if autonegotiate > 4 dialects */ } __packed; I repeat my suggestion. Alternatively, change the code to make it a variable array and allocate it properly. Tom. > According MS-SMB2, there really not define the length of the package, just > define the count of the dialects, but just send the needed data maybe more > appropriate. > > Thanks. > > 在 2022/8/30 22:03, Tom Talpey 写道: >> Wouldn't it be safer to just set the size, instead of implicitly >> assuming that there are 4 array elements? >> >> inbuflen = sizeof(*pneg_inbuf) - sizeof(pneg_inbuf.Dialects) + >> sizeof(pneg_inbuf.Dialects[0]); >> >> Or, because it obviously works to send the extra bytes even >> though the DialectCount is just 1, just zero them out? >> >> memset(pneg_inbuf.Dialects, 0, sizeof(pneg_inbuf.Dialects)); >> pneg_inbuf.Dialects[0] = cpu_to_le16(server->vals->protocol_id); >> >> Tom. >> >> On 8/30/2022 3:06 AM, huaweicloud wrote: >>> Hi Steve, >>> >>> Could you help to review this patch. >>> >>> Thanks, >>> Zhang Xiaoxu >>> >>> 在 2022/8/24 16:57, Zhang Xiaoxu 写道: >>>> Commit d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") >>>> extend the dialects from 3 to 4, but forget to decrease the extended >>>> length when specific the dialect, then the message length is larger >>>> than expected. >>>> >>>> This maybe leak some info through network because not initialize the >>>> message body. >>>> >>>> After apply this patch, the VALIDATE_NEGOTIATE_INFO message length is >>>> reduced from 28 bytes to 26 bytes. >>>> >>>> Fixes: d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") >>>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> >>>> Cc: <stable@vger.kernel.org> >>>> --- >>>> fs/cifs/smb2pdu.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >>>> index 1ecc2501c56f..3df7adc01fe5 100644 >>>> --- a/fs/cifs/smb2pdu.c >>>> +++ b/fs/cifs/smb2pdu.c >>>> @@ -1162,9 +1162,9 @@ int smb3_validate_negotiate(const unsigned int >>>> xid, struct cifs_tcon *tcon) >>>> pneg_inbuf->Dialects[0] = >>>> cpu_to_le16(server->vals->protocol_id); >>>> pneg_inbuf->DialectCount = cpu_to_le16(1); >>>> - /* structure is big enough for 3 dialects, sending only 1 */ >>>> + /* structure is big enough for 4 dialects, sending only 1 */ >>>> inbuflen = sizeof(*pneg_inbuf) - >>>> - sizeof(pneg_inbuf->Dialects[0]) * 2; >>>> + sizeof(pneg_inbuf->Dialects[0]) * 3; >>>> } >>>> rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID, >>> >>> > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message 2022-08-30 16:19 ` Tom Talpey @ 2022-08-31 2:03 ` huaweicloud 2022-08-31 3:39 ` huaweicloud 0 siblings, 1 reply; 8+ messages in thread From: huaweicloud @ 2022-08-31 2:03 UTC (permalink / raw) To: Tom Talpey, stfrench; +Cc: linux-cifs, sfrench, lsahlber, sprasad, rohiths, pc 在 2022/8/31 0:19, Tom Talpey 写道: > On 8/30/2022 10:30 AM, huaweicloud wrote: >> I think the struct validate_negotiate_info_req is an variable-length array, >> just for implement simple, defind as 4 in here. > > But that's my point. The code picks "4" arbitrarily, and that > number can change - as you discovered, it already did since > it used to be "3". > > smb2pdu.h: > struct validate_negotiate_info_req { > __le32 Capabilities; > __u8 Guid[SMB2_CLIENT_GUID_SIZE]; > __le16 SecurityMode; > __le16 DialectCount; > __le16 Dialects[4]; /* BB expand this if autonegotiate > 4 dialects */ > } __packed; > > I repeat my suggestion. > > Alternatively, change the code to make it a variable array and > allocate it properly. Agreed with change it to variable-length array. Since the struct validate_negotiate_info_req also used by ksmbd and it not treat it as the variable-length array: int smb2_ioctl(struct ksmbd_work *work) { case FSCTL_VALIDATE_NEGOTIATE_INFO: if (in_buf_len < sizeof(struct validate_negotiate_info_req)) return -EINVAL; } But in samba, treat it as the variable-length array: static NTSTATUS fsctl_validate_neg_info(TALLOC_CTX *mem_ctx, struct tevent_context *ev, struct smbXsrv_connection *conn, DATA_BLOB *in_input, uint32_t in_max_output, DATA_BLOB *out_output, bool *disconnect) { in_num_dialects = SVAL(in_input->data, 0x16); if (in_input->length < (0x18 + in_num_dialects*2)) { return NT_STATUS_INVALID_PARAMETER; } } now, I'm trying to make it to variable-length array, but this patch, just fix the wrong message length sent by cifs client. Thanks. > > Tom. > >> According MS-SMB2, there really not define the length of the package, just >> define the count of the dialects, but just send the needed data maybe more >> appropriate. >> >> Thanks. >> >> 在 2022/8/30 22:03, Tom Talpey 写道: >>> Wouldn't it be safer to just set the size, instead of implicitly >>> assuming that there are 4 array elements? >>> >>> inbuflen = sizeof(*pneg_inbuf) - sizeof(pneg_inbuf.Dialects) + sizeof(pneg_inbuf.Dialects[0]); >>> >>> Or, because it obviously works to send the extra bytes even >>> though the DialectCount is just 1, just zero them out? >>> >>> memset(pneg_inbuf.Dialects, 0, sizeof(pneg_inbuf.Dialects)); >>> pneg_inbuf.Dialects[0] = cpu_to_le16(server->vals->protocol_id); >>> >>> Tom. >>> >>> On 8/30/2022 3:06 AM, huaweicloud wrote: >>>> Hi Steve, >>>> >>>> Could you help to review this patch. >>>> >>>> Thanks, >>>> Zhang Xiaoxu >>>> >>>> 在 2022/8/24 16:57, Zhang Xiaoxu 写道: >>>>> Commit d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") >>>>> extend the dialects from 3 to 4, but forget to decrease the extended >>>>> length when specific the dialect, then the message length is larger >>>>> than expected. >>>>> >>>>> This maybe leak some info through network because not initialize the >>>>> message body. >>>>> >>>>> After apply this patch, the VALIDATE_NEGOTIATE_INFO message length is >>>>> reduced from 28 bytes to 26 bytes. >>>>> >>>>> Fixes: d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") >>>>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> >>>>> Cc: <stable@vger.kernel.org> >>>>> --- >>>>> fs/cifs/smb2pdu.c | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >>>>> index 1ecc2501c56f..3df7adc01fe5 100644 >>>>> --- a/fs/cifs/smb2pdu.c >>>>> +++ b/fs/cifs/smb2pdu.c >>>>> @@ -1162,9 +1162,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) >>>>> pneg_inbuf->Dialects[0] = >>>>> cpu_to_le16(server->vals->protocol_id); >>>>> pneg_inbuf->DialectCount = cpu_to_le16(1); >>>>> - /* structure is big enough for 3 dialects, sending only 1 */ >>>>> + /* structure is big enough for 4 dialects, sending only 1 */ >>>>> inbuflen = sizeof(*pneg_inbuf) - >>>>> - sizeof(pneg_inbuf->Dialects[0]) * 2; >>>>> + sizeof(pneg_inbuf->Dialects[0]) * 3; >>>>> } >>>>> rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID, >>>> >>>> >> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message 2022-08-31 2:03 ` huaweicloud @ 2022-08-31 3:39 ` huaweicloud 2022-08-31 12:49 ` Tom Talpey 0 siblings, 1 reply; 8+ messages in thread From: huaweicloud @ 2022-08-31 3:39 UTC (permalink / raw) To: Tom Talpey, stfrench; +Cc: linux-cifs, sfrench, lsahlber, sprasad, rohiths, pc 在 2022/8/31 10:03, huaweicloud 写道: > > > 在 2022/8/31 0:19, Tom Talpey 写道: >> On 8/30/2022 10:30 AM, huaweicloud wrote: >>> I think the struct validate_negotiate_info_req is an variable-length array, >>> just for implement simple, defind as 4 in here. >> >> But that's my point. The code picks "4" arbitrarily, and that >> number can change - as you discovered, it already did since >> it used to be "3". >> >> smb2pdu.h: >> struct validate_negotiate_info_req { >> __le32 Capabilities; >> __u8 Guid[SMB2_CLIENT_GUID_SIZE]; >> __le16 SecurityMode; >> __le16 DialectCount; >> __le16 Dialects[4]; /* BB expand this if autonegotiate > 4 dialects */ >> } __packed; >> >> I repeat my suggestion. >> >> Alternatively, change tqhe code to make it a variable array and >> allocate it properly. > Agreed with change it to variable-length array. > > Since the struct validate_negotiate_info_req also used by ksmbd and it not > treat it as the variable-length array: > > int smb2_ioctl(struct ksmbd_work *work) > { > case FSCTL_VALIDATE_NEGOTIATE_INFO: > > if (in_buf_len < sizeof(struct validate_negotiate_info_req)) > return -EINVAL; > } Before commit c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock break, and move its struct to smbfs_common"), ksmbd also treat it as variable-length array. > > But in samba, treat it as the variable-length array: > > static NTSTATUS fsctl_validate_neg_info(TALLOC_CTX *mem_ctx, > struct tevent_context *ev, > struct smbXsrv_connection *conn, > DATA_BLOB *in_input, > uint32_t in_max_output, > DATA_BLOB *out_output, > bool *disconnect) > { > in_num_dialects = SVAL(in_input->data, 0x16); > > if (in_input->length < (0x18 + in_num_dialects*2)) { > return NT_STATUS_INVALID_PARAMETER; > } > } > > now, I'm trying to make it to variable-length array, > but this patch, just fix the wrong message length sent by cifs client. > > Thanks. >> >> Tom. >> >>> According MS-SMB2, there really not define the length of the package, just >>> define the count of the dialects, but just send the needed data maybe more >>> appropriate. >>> >>> Thanks. >>> >>> 在 2022/8/30 22:03, Tom Talpey 写道: >>>> Wouldn't it be safer to just set the size, instead of implicitly >>>> assuming that there are 4 array elements? >>>> >>>> inbuflen = sizeof(*pneg_inbuf) - sizeof(pneg_inbuf.Dialects) + sizeof(pneg_inbuf.Dialects[0]); >>>> >>>> Or, because it obviously works to send the extra bytes even >>>> though the DialectCount is just 1, just zero them out? >>>> >>>> memset(pneg_inbuf.Dialects, 0, sizeof(pneg_inbuf.Dialects)); >>>> pneg_inbuf.Dialects[0] = cpu_to_le16(server->vals->protocol_id); >>>> >>>> Tom. >>>> >>>> On 8/30/2022 3:06 AM, huaweicloud wrote: >>>>> Hi Steve, >>>>> >>>>> Could you help to review this patch. >>>>> >>>>> Thanks, >>>>> Zhang Xiaoxu >>>>> >>>>> 在 2022/8/24 16:57, Zhang Xiaoxu 写道: >>>>>> Commit d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") >>>>>> extend the dialects from 3 to 4, but forget to decrease the extended >>>>>> length when specific the dialect, then the message length is larger >>>>>> than expected. >>>>>> >>>>>> This maybe leak some info through network because not initialize the >>>>>> message body. >>>>>> >>>>>> After apply this patch, the VALIDATE_NEGOTIATE_INFO message length is >>>>>> reduced from 28 bytes to 26 bytes. >>>>>> >>>>>> Fixes: d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") >>>>>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> >>>>>> Cc: <stable@vger.kernel.org> >>>>>> --- >>>>>> fs/cifs/smb2pdu.c | 4 ++-- >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >>>>>> index 1ecc2501c56f..3df7adc01fe5 100644 >>>>>> --- a/fs/cifs/smb2pdu.c >>>>>> +++ b/fs/cifs/smb2pdu.c >>>>>> @@ -1162,9 +1162,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon) >>>>>> pneg_inbuf->Dialects[0] = >>>>>> cpu_to_le16(server->vals->protocol_id); >>>>>> pneg_inbuf->DialectCount = cpu_to_le16(1); >>>>>> - /* structure is big enough for 3 dialects, sending only 1 */ >>>>>> + /* structure is big enough for 4 dialects, sending only 1 */ >>>>>> inbuflen = sizeof(*pneg_inbuf) - >>>>>> - sizeof(pneg_inbuf->Dialects[0]) * 2; >>>>>> + sizeof(pneg_inbuf->Dialects[0]) * 3; >>>>>> } >>>>>> rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID, >>>>> >>>>> >>> >>> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message 2022-08-31 3:39 ` huaweicloud @ 2022-08-31 12:49 ` Tom Talpey 0 siblings, 0 replies; 8+ messages in thread From: Tom Talpey @ 2022-08-31 12:49 UTC (permalink / raw) To: huaweicloud, stfrench; +Cc: linux-cifs, sfrench, lsahlber, sprasad, rohiths, pc On 8/30/2022 11:39 PM, huaweicloud wrote: > 在 2022/8/31 10:03, huaweicloud 写道: >> 在 2022/8/31 0:19, Tom Talpey 写道: >>> On 8/30/2022 10:30 AM, huaweicloud wrote: >>>> I think the struct validate_negotiate_info_req is an variable-length >>>> array, >>>> just for implement simple, defind as 4 in here. >>> >>> But that's my point. The code picks "4" arbitrarily, and that >>> number can change - as you discovered, it already did since >>> it used to be "3". >>> >>> smb2pdu.h: >>> struct validate_negotiate_info_req { >>> __le32 Capabilities; >>> __u8 Guid[SMB2_CLIENT_GUID_SIZE]; >>> __le16 SecurityMode; >>> __le16 DialectCount; >>> __le16 Dialects[4]; /* BB expand this if autonegotiate > 4 >>> dialects */ >>> } __packed; > >>> >>> I repeat my suggestion. >>> >>> Alternatively, change tqhe code to make it a variable array and >>> allocate it properly. >> Agreed with change it to variable-length array. >> >> Since the struct validate_negotiate_info_req also used by ksmbd and it >> not >> treat it as the variable-length array: >> >> int smb2_ioctl(struct ksmbd_work *work) >> { >> case FSCTL_VALIDATE_NEGOTIATE_INFO: >> >> if (in_buf_len < sizeof(struct >> validate_negotiate_info_req)) >> return -EINVAL; >> } > Before commit c7803b05f74b ("smb3: fix ksmbd bigendian bug in oplock > break, and move its struct to smbfs_common"), > ksmbd also treat it as variable-length array. Whoa, looking at the ksmbd code, these are significant interop bugs. The server should definitely not be rejecting a negotiate which does not carry this arbitrary "4" value, in addition to the client sending it properly too. I look forward to your updates, sorry they're more work now! :) Tom. >> But in samba, treat it as the variable-length array: >> >> static NTSTATUS fsctl_validate_neg_info(TALLOC_CTX *mem_ctx, >> struct tevent_context *ev, >> struct smbXsrv_connection *conn, >> DATA_BLOB *in_input, >> uint32_t in_max_output, >> DATA_BLOB *out_output, >> bool *disconnect) >> { >> in_num_dialects = SVAL(in_input->data, 0x16); >> >> if (in_input->length < (0x18 + in_num_dialects*2)) { >> return NT_STATUS_INVALID_PARAMETER; >> } >> } >> >> now, I'm trying to make it to variable-length array, >> but this patch, just fix the wrong message length sent by cifs client. >> >> Thanks. >>> >>> Tom. >>> >>>> According MS-SMB2, there really not define the length of the >>>> package, just >>>> define the count of the dialects, but just send the needed data >>>> maybe more >>>> appropriate. >>>> >>>> Thanks. >>>> >>>> 在 2022/8/30 22:03, Tom Talpey 写道: >>>>> Wouldn't it be safer to just set the size, instead of implicitly >>>>> assuming that there are 4 array elements? >>>>> >>>>> inbuflen = sizeof(*pneg_inbuf) - sizeof(pneg_inbuf.Dialects) + >>>>> sizeof(pneg_inbuf.Dialects[0]); >>>>> >>>>> Or, because it obviously works to send the extra bytes even >>>>> though the DialectCount is just 1, just zero them out? >>>>> >>>>> memset(pneg_inbuf.Dialects, 0, sizeof(pneg_inbuf.Dialects)); >>>>> pneg_inbuf.Dialects[0] = cpu_to_le16(server->vals->protocol_id); >>>>> >>>>> Tom. >>>>> >>>>> On 8/30/2022 3:06 AM, huaweicloud wrote: >>>>>> Hi Steve, >>>>>> >>>>>> Could you help to review this patch. >>>>>> >>>>>> Thanks, >>>>>> Zhang Xiaoxu >>>>>> >>>>>> 在 2022/8/24 16:57, Zhang Xiaoxu 写道: >>>>>>> Commit d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") >>>>>>> extend the dialects from 3 to 4, but forget to decrease the extended >>>>>>> length when specific the dialect, then the message length is larger >>>>>>> than expected. >>>>>>> >>>>>>> This maybe leak some info through network because not initialize the >>>>>>> message body. >>>>>>> >>>>>>> After apply this patch, the VALIDATE_NEGOTIATE_INFO message >>>>>>> length is >>>>>>> reduced from 28 bytes to 26 bytes. >>>>>>> >>>>>>> Fixes: d5c7076b772a ("smb3: add smb3.1.1 to default dialect list") >>>>>>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> >>>>>>> Cc: <stable@vger.kernel.org> >>>>>>> --- >>>>>>> fs/cifs/smb2pdu.c | 4 ++-- >>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >>>>>>> index 1ecc2501c56f..3df7adc01fe5 100644 >>>>>>> --- a/fs/cifs/smb2pdu.c >>>>>>> +++ b/fs/cifs/smb2pdu.c >>>>>>> @@ -1162,9 +1162,9 @@ int smb3_validate_negotiate(const unsigned >>>>>>> int xid, struct cifs_tcon *tcon) >>>>>>> pneg_inbuf->Dialects[0] = >>>>>>> cpu_to_le16(server->vals->protocol_id); >>>>>>> pneg_inbuf->DialectCount = cpu_to_le16(1); >>>>>>> - /* structure is big enough for 3 dialects, sending only >>>>>>> 1 */ >>>>>>> + /* structure is big enough for 4 dialects, sending only >>>>>>> 1 */ >>>>>>> inbuflen = sizeof(*pneg_inbuf) - >>>>>>> - sizeof(pneg_inbuf->Dialects[0]) * 2; >>>>>>> + sizeof(pneg_inbuf->Dialects[0]) * 3; >>>>>>> } >>>>>>> rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID, >>>>>> >>>>>> >>>> >>>> >> > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-08-31 12:50 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-24 8:57 [PATCH] cifs: Fix the error length of VALIDATE_NEGOTIATE_INFO message Zhang Xiaoxu 2022-08-30 7:06 ` huaweicloud 2022-08-30 14:03 ` Tom Talpey 2022-08-30 14:30 ` huaweicloud 2022-08-30 16:19 ` Tom Talpey 2022-08-31 2:03 ` huaweicloud 2022-08-31 3:39 ` huaweicloud 2022-08-31 12:49 ` Tom Talpey
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.