* [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
@ 2023-05-18 14:42 HexRabbit
2023-05-19 1:37 ` Namjae Jeon
0 siblings, 1 reply; 13+ messages in thread
From: HexRabbit @ 2023-05-18 14:42 UTC (permalink / raw)
To: linkinjeon; +Cc: sfrench, senozhatsky, tom, linux-cifs, Kuan-Ting Chen
From: Kuan-Ting Chen <h3xrabbit@gmail.com>
Check the remaining data length before accessing the context structure
to ensure that the entire structure is contained within the packet.
Additionally, since the context data length `ctxt_len` has already been
checked against the total packet length `len_of_ctxts`, update the
comparison to use `ctxt_len`.
Signed-off-by: Kuan-Ting Chen <h3xrabbit@gmail.com>
---
fs/ksmbd/smb2pdu.c | 52 +++++++++++++++++++++++++++++-----------------
1 file changed, 33 insertions(+), 19 deletions(-)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 972176bff..0285c3f9e 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -849,13 +849,13 @@ static void assemble_neg_contexts(struct ksmbd_conn *conn,
static __le32 decode_preauth_ctxt(struct ksmbd_conn *conn,
struct smb2_preauth_neg_context *pneg_ctxt,
- int len_of_ctxts)
+ int ctxt_len)
{
/*
* sizeof(smb2_preauth_neg_context) assumes SMB311_SALT_SIZE Salt,
* which may not be present. Only check for used HashAlgorithms[1].
*/
- if (len_of_ctxts < MIN_PREAUTH_CTXT_DATA_LEN)
+ if (ctxt_len < MIN_PREAUTH_CTXT_DATA_LEN)
return STATUS_INVALID_PARAMETER;
if (pneg_ctxt->HashAlgorithms != SMB2_PREAUTH_INTEGRITY_SHA512)
@@ -867,15 +867,23 @@ static __le32 decode_preauth_ctxt(struct ksmbd_conn *conn,
static void decode_encrypt_ctxt(struct ksmbd_conn *conn,
struct smb2_encryption_neg_context *pneg_ctxt,
- int len_of_ctxts)
+ int ctxt_len)
{
- int cph_cnt = le16_to_cpu(pneg_ctxt->CipherCount);
- int i, cphs_size = cph_cnt * sizeof(__le16);
+ int cph_cnt;
+ int i, cphs_size;
+
+ if (sizeof(struct smb2_encryption_neg_context) > ctxt_len) {
+ pr_err("Invalid SMB2_ENCRYPTION_CAPABILITIES context size\n");
+ return;
+ }
conn->cipher_type = 0;
+ cph_cnt = le16_to_cpu(pneg_ctxt->CipherCount);
+ cphs_size = cph_cnt * sizeof(__le16);
+
if (sizeof(struct smb2_encryption_neg_context) + cphs_size >
- len_of_ctxts) {
+ ctxt_len) {
pr_err("Invalid cipher count(%d)\n", cph_cnt);
return;
}
@@ -923,15 +931,22 @@ static void decode_compress_ctxt(struct ksmbd_conn *conn,
static void decode_sign_cap_ctxt(struct ksmbd_conn *conn,
struct smb2_signing_capabilities *pneg_ctxt,
- int len_of_ctxts)
+ int ctxt_len)
{
- int sign_algo_cnt = le16_to_cpu(pneg_ctxt->SigningAlgorithmCount);
- int i, sign_alos_size = sign_algo_cnt * sizeof(__le16);
+ int sign_algo_cnt;
+ int i, sign_alos_size;
+
+ if (sizeof(struct smb2_signing_capabilities) > ctxt_len) {
+ pr_err("Invalid SMB2_SIGNING_CAPABILITIES context length\n");
+ return;
+ }
conn->signing_negotiated = false;
+ sign_algo_cnt = le16_to_cpu(pneg_ctxt->SigningAlgorithmCount);
+ sign_alos_size = sign_algo_cnt * sizeof(__le16);
if (sizeof(struct smb2_signing_capabilities) + sign_alos_size >
- len_of_ctxts) {
+ ctxt_len) {
pr_err("Invalid signing algorithm count(%d)\n", sign_algo_cnt);
return;
}
@@ -969,18 +984,16 @@ static __le32 deassemble_neg_contexts(struct ksmbd_conn *conn,
len_of_ctxts = len_of_smb - offset;
while (i++ < neg_ctxt_cnt) {
- int clen;
-
- /* check that offset is not beyond end of SMB */
- if (len_of_ctxts == 0)
- break;
+ int clen, ctxt_len;
if (len_of_ctxts < sizeof(struct smb2_neg_context))
break;
pctx = (struct smb2_neg_context *)((char *)pctx + offset);
clen = le16_to_cpu(pctx->DataLength);
- if (clen + sizeof(struct smb2_neg_context) > len_of_ctxts)
+ ctxt_len = clen + sizeof(struct smb2_neg_context);
+
+ if (ctxt_len > len_of_ctxts)
break;
if (pctx->ContextType == SMB2_PREAUTH_INTEGRITY_CAPABILITIES) {
@@ -991,7 +1004,7 @@ static __le32 deassemble_neg_contexts(struct ksmbd_conn *conn,
status = decode_preauth_ctxt(conn,
(struct smb2_preauth_neg_context *)pctx,
- len_of_ctxts);
+ ctxt_len);
if (status != STATUS_SUCCESS)
break;
} else if (pctx->ContextType == SMB2_ENCRYPTION_CAPABILITIES) {
@@ -1002,7 +1015,7 @@ static __le32 deassemble_neg_contexts(struct ksmbd_conn *conn,
decode_encrypt_ctxt(conn,
(struct smb2_encryption_neg_context *)pctx,
- len_of_ctxts);
+ ctxt_len);
} else if (pctx->ContextType == SMB2_COMPRESSION_CAPABILITIES) {
ksmbd_debug(SMB,
"deassemble SMB2_COMPRESSION_CAPABILITIES context\n");
@@ -1021,9 +1034,10 @@ static __le32 deassemble_neg_contexts(struct ksmbd_conn *conn,
} else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES) {
ksmbd_debug(SMB,
"deassemble SMB2_SIGNING_CAPABILITIES context\n");
+
decode_sign_cap_ctxt(conn,
(struct smb2_signing_capabilities *)pctx,
- len_of_ctxts);
+ ctxt_len);
}
/* offsets must be 8 byte aligned */
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
2023-05-18 14:42 [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding HexRabbit
@ 2023-05-19 1:37 ` Namjae Jeon
2023-05-19 4:34 ` Hex Rabbit
0 siblings, 1 reply; 13+ messages in thread
From: Namjae Jeon @ 2023-05-19 1:37 UTC (permalink / raw)
To: HexRabbit; +Cc: sfrench, senozhatsky, tom, linux-cifs
2023-05-18 23:42 GMT+09:00, HexRabbit <h3xrabbit@gmail.com>:
> From: Kuan-Ting Chen <h3xrabbit@gmail.com>
>
> Check the remaining data length before accessing the context structure
> to ensure that the entire structure is contained within the packet.
> Additionally, since the context data length `ctxt_len` has already been
> checked against the total packet length `len_of_ctxts`, update the
> comparison to use `ctxt_len`.
>
> Signed-off-by: Kuan-Ting Chen <h3xrabbit@gmail.com>
> ---
> fs/ksmbd/smb2pdu.c | 52 +++++++++++++++++++++++++++++-----------------
> 1 file changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 972176bff..0285c3f9e 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -849,13 +849,13 @@ static void assemble_neg_contexts(struct ksmbd_conn
> *conn,
>
> static __le32 decode_preauth_ctxt(struct ksmbd_conn *conn,
> struct smb2_preauth_neg_context *pneg_ctxt,
> - int len_of_ctxts)
> + int ctxt_len)
> {
> /*
> * sizeof(smb2_preauth_neg_context) assumes SMB311_SALT_SIZE Salt,
> * which may not be present. Only check for used HashAlgorithms[1].
> */
> - if (len_of_ctxts < MIN_PREAUTH_CTXT_DATA_LEN)
> + if (ctxt_len < MIN_PREAUTH_CTXT_DATA_LEN)
if (ctxt_len <
sizeof(struct smb2_neg_context) + MIN_PREAUTH_CTXT_DATA_LEN)
You need to plus sizeof(struct smb2_neg_context) here.
MIN_PREAUTH_CTXT_DATA_LEN accounts for HashAlgorithmCount,
SaltLength, and 1 HashAlgorithm.
> return STATUS_INVALID_PARAMETER;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
2023-05-19 1:37 ` Namjae Jeon
@ 2023-05-19 4:34 ` Hex Rabbit
2023-05-19 5:16 ` Namjae Jeon
0 siblings, 1 reply; 13+ messages in thread
From: Hex Rabbit @ 2023-05-19 4:34 UTC (permalink / raw)
To: Namjae Jeon; +Cc: sfrench, senozhatsky, tom, linux-cifs
> if (ctxt_len <
> sizeof(struct smb2_neg_context) + MIN_PREAUTH_CTXT_DATA_LEN)
> You need to plus sizeof(struct smb2_neg_context) here.
> MIN_PREAUTH_CTXT_DATA_LEN accounts for HashAlgorithmCount,
> SaltLength, and 1 HashAlgorithm.
>
> > return STATUS_INVALID_PARAMETER;
Sorry, should I resend the patch?
Thanks
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
2023-05-19 4:34 ` Hex Rabbit
@ 2023-05-19 5:16 ` Namjae Jeon
0 siblings, 0 replies; 13+ messages in thread
From: Namjae Jeon @ 2023-05-19 5:16 UTC (permalink / raw)
To: Hex Rabbit; +Cc: sfrench, senozhatsky, tom, linux-cifs
2023-05-19 13:34 GMT+09:00, Hex Rabbit <h3xrabbit@gmail.com>:
>> if (ctxt_len <
>> sizeof(struct smb2_neg_context) + MIN_PREAUTH_CTXT_DATA_LEN)
>> You need to plus sizeof(struct smb2_neg_context) here.
>> MIN_PREAUTH_CTXT_DATA_LEN accounts for HashAlgorithmCount,
>> SaltLength, and 1 HashAlgorithm.
>>
>> > return STATUS_INVALID_PARAMETER;
>
> Sorry, should I resend the patch?
No, I will directly update it.
Thanks for your patch.
>
> Thanks
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
2023-05-18 8:11 ` Hex Rabbit
@ 2023-05-18 14:10 ` Namjae Jeon
0 siblings, 0 replies; 13+ messages in thread
From: Namjae Jeon @ 2023-05-18 14:10 UTC (permalink / raw)
To: Hex Rabbit; +Cc: sfrench, senozhatsky, tom, linux-cifs
2023-05-18 17:11 GMT+09:00, Hex Rabbit <h3xrabbit@gmail.com>:
> I have decided to leave the modifications within the function that handles
> the
> corresponding context. The reason for this is that I believe consolidating
> the
> checks together can improve readability, also, moving them out would
> require
> us to read the size of the flex-sized array again in the corresponding
> function.
>
> What do you think?
Looks okay. Please send the patch to test to the list.
Thanks.
>
> below is the modified patch:
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 972176bff..0285c3f9e 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -849,13 +849,13 @@ static void assemble_neg_contexts(struct ksmbd_conn
> *conn,
>
> static __le32 decode_preauth_ctxt(struct ksmbd_conn *conn,
> struct smb2_preauth_neg_context *pneg_ctxt,
> - int len_of_ctxts)
> + int ctxt_len)
> {
> /*
> * sizeof(smb2_preauth_neg_context) assumes SMB311_SALT_SIZE Salt,
> * which may not be present. Only check for used HashAlgorithms[1].
> */
> - if (len_of_ctxts < MIN_PREAUTH_CTXT_DATA_LEN)
> + if (ctxt_len < MIN_PREAUTH_CTXT_DATA_LEN)
> return STATUS_INVALID_PARAMETER;
>
> if (pneg_ctxt->HashAlgorithms != SMB2_PREAUTH_INTEGRITY_SHA512)
> @@ -867,15 +867,23 @@ static __le32 decode_preauth_ctxt(struct ksmbd_conn
> *conn,
>
> static void decode_encrypt_ctxt(struct ksmbd_conn *conn,
> struct smb2_encryption_neg_context *pneg_ctxt,
> - int len_of_ctxts)
> + int ctxt_len)
> {
> - int cph_cnt = le16_to_cpu(pneg_ctxt->CipherCount);
> - int i, cphs_size = cph_cnt * sizeof(__le16);
> + int cph_cnt;
> + int i, cphs_size;
> +
> + if (sizeof(struct smb2_encryption_neg_context) > ctxt_len) {
> + pr_err("Invalid SMB2_ENCRYPTION_CAPABILITIES context size\n");
> + return;
> + }
>
> conn->cipher_type = 0;
>
> + cph_cnt = le16_to_cpu(pneg_ctxt->CipherCount);
> + cphs_size = cph_cnt * sizeof(__le16);
> +
> if (sizeof(struct smb2_encryption_neg_context) + cphs_size >
> - len_of_ctxts) {
> + ctxt_len) {
> pr_err("Invalid cipher count(%d)\n", cph_cnt);
> return;
> }
> @@ -923,15 +931,22 @@ static void decode_compress_ctxt(struct ksmbd_conn
> *conn,
>
> static void decode_sign_cap_ctxt(struct ksmbd_conn *conn,
> struct smb2_signing_capabilities *pneg_ctxt,
> - int len_of_ctxts)
> + int ctxt_len)
> {
> - int sign_algo_cnt = le16_to_cpu(pneg_ctxt->SigningAlgorithmCount);
> - int i, sign_alos_size = sign_algo_cnt * sizeof(__le16);
> + int sign_algo_cnt;
> + int i, sign_alos_size;
> +
> + if (sizeof(struct smb2_signing_capabilities) > ctxt_len) {
> + pr_err("Invalid SMB2_SIGNING_CAPABILITIES context length\n");
> + return;
> + }
>
> conn->signing_negotiated = false;
> + sign_algo_cnt = le16_to_cpu(pneg_ctxt->SigningAlgorithmCount);
> + sign_alos_size = sign_algo_cnt * sizeof(__le16);
>
> if (sizeof(struct smb2_signing_capabilities) + sign_alos_size >
> - len_of_ctxts) {
> + ctxt_len) {
> pr_err("Invalid signing algorithm count(%d)\n", sign_algo_cnt);
> return;
> }
> @@ -969,18 +984,16 @@ static __le32 deassemble_neg_contexts(struct
> ksmbd_conn *conn,
> len_of_ctxts = len_of_smb - offset;
>
> while (i++ < neg_ctxt_cnt) {
> - int clen;
> -
> - /* check that offset is not beyond end of SMB */
> - if (len_of_ctxts == 0)
> - break;
> + int clen, ctxt_len;
>
> if (len_of_ctxts < sizeof(struct smb2_neg_context))
> break;
>
> pctx = (struct smb2_neg_context *)((char *)pctx + offset);
> clen = le16_to_cpu(pctx->DataLength);
> - if (clen + sizeof(struct smb2_neg_context) > len_of_ctxts)
> + ctxt_len = clen + sizeof(struct smb2_neg_context);
> +
> + if (ctxt_len > len_of_ctxts)
> break;
>
> if (pctx->ContextType == SMB2_PREAUTH_INTEGRITY_CAPABILITIES) {
> @@ -991,7 +1004,7 @@ static __le32 deassemble_neg_contexts(struct
> ksmbd_conn *conn,
>
> status = decode_preauth_ctxt(conn,
> (struct smb2_preauth_neg_context *)pctx,
> - len_of_ctxts);
> + ctxt_len);
> if (status != STATUS_SUCCESS)
> break;
> } else if (pctx->ContextType == SMB2_ENCRYPTION_CAPABILITIES) {
> @@ -1002,7 +1015,7 @@ static __le32 deassemble_neg_contexts(struct
> ksmbd_conn *conn,
>
> decode_encrypt_ctxt(conn,
> (struct smb2_encryption_neg_context *)pctx,
> - len_of_ctxts);
> + ctxt_len);
> } else if (pctx->ContextType == SMB2_COMPRESSION_CAPABILITIES) {
> ksmbd_debug(SMB,
> "deassemble SMB2_COMPRESSION_CAPABILITIES context\n");
> @@ -1021,9 +1034,10 @@ static __le32 deassemble_neg_contexts(struct
> ksmbd_conn *conn,
> } else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES) {
> ksmbd_debug(SMB,
> "deassemble SMB2_SIGNING_CAPABILITIES context\n");
> +
> decode_sign_cap_ctxt(conn,
> (struct smb2_signing_capabilities *)pctx,
> - len_of_ctxts);
> + ctxt_len);
> }
>
> /* offsets must be 8 byte aligned */
> ---
>
> Namjae Jeon <linkinjeon@kernel.org> 於 2023年5月18日 週四 下午2:36寫道：
>>
>> 2023-05-18 15:30 GMT+09:00, Hex Rabbit <h3xrabbit@gmail.com>:
>> >> You need to consider Ciphers flex-array size to validate ctxt_len. we
>> >> can get its size using CipherCount in smb2_encryption_neg_context.
>> >
>> > I'm not checking the flex-array size since both
>> > `decode_sign_cap_ctxt()`
>> > and `decode_encrypt_ctxt()` have done it, or should I move it out?
>> Yes, We can move it out. Thanks.
>> >
>> > ```
>> > if (sizeof(struct smb2_encryption_neg_context) + cphs_size >
>> > len_of_ctxts) {
>> > pr_err("Invalid cipher count(%d)\n", cph_cnt);
>> > return;
>> > }
>> > ```
>> >
>> > ```
>> > if (sizeof(struct smb2_signing_capabilities) + sign_alos_size >
>> > len_of_ctxts) {
>> > pr_err("Invalid signing algorithm count(%d)\n", sign_algo_cnt);
>> > return;
>> > }
>> > ```
>> >
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
2023-05-18 6:36 ` Namjae Jeon
@ 2023-05-18 8:11 ` Hex Rabbit
2023-05-18 14:10 ` Namjae Jeon
0 siblings, 1 reply; 13+ messages in thread
From: Hex Rabbit @ 2023-05-18 8:11 UTC (permalink / raw)
To: Namjae Jeon; +Cc: sfrench, senozhatsky, tom, linux-cifs
I have decided to leave the modifications within the function that handles the
corresponding context. The reason for this is that I believe consolidating the
checks together can improve readability, also, moving them out would require
us to read the size of the flex-sized array again in the corresponding function.
What do you think?
below is the modified patch:
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 972176bff..0285c3f9e 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -849,13 +849,13 @@ static void assemble_neg_contexts(struct ksmbd_conn *conn,
static __le32 decode_preauth_ctxt(struct ksmbd_conn *conn,
struct smb2_preauth_neg_context *pneg_ctxt,
- int len_of_ctxts)
+ int ctxt_len)
{
/*
* sizeof(smb2_preauth_neg_context) assumes SMB311_SALT_SIZE Salt,
* which may not be present. Only check for used HashAlgorithms[1].
*/
- if (len_of_ctxts < MIN_PREAUTH_CTXT_DATA_LEN)
+ if (ctxt_len < MIN_PREAUTH_CTXT_DATA_LEN)
return STATUS_INVALID_PARAMETER;
if (pneg_ctxt->HashAlgorithms != SMB2_PREAUTH_INTEGRITY_SHA512)
@@ -867,15 +867,23 @@ static __le32 decode_preauth_ctxt(struct ksmbd_conn *conn,
static void decode_encrypt_ctxt(struct ksmbd_conn *conn,
struct smb2_encryption_neg_context *pneg_ctxt,
- int len_of_ctxts)
+ int ctxt_len)
{
- int cph_cnt = le16_to_cpu(pneg_ctxt->CipherCount);
- int i, cphs_size = cph_cnt * sizeof(__le16);
+ int cph_cnt;
+ int i, cphs_size;
+
+ if (sizeof(struct smb2_encryption_neg_context) > ctxt_len) {
+ pr_err("Invalid SMB2_ENCRYPTION_CAPABILITIES context size\n");
+ return;
+ }
conn->cipher_type = 0;
+ cph_cnt = le16_to_cpu(pneg_ctxt->CipherCount);
+ cphs_size = cph_cnt * sizeof(__le16);
+
if (sizeof(struct smb2_encryption_neg_context) + cphs_size >
- len_of_ctxts) {
+ ctxt_len) {
pr_err("Invalid cipher count(%d)\n", cph_cnt);
return;
}
@@ -923,15 +931,22 @@ static void decode_compress_ctxt(struct ksmbd_conn *conn,
static void decode_sign_cap_ctxt(struct ksmbd_conn *conn,
struct smb2_signing_capabilities *pneg_ctxt,
- int len_of_ctxts)
+ int ctxt_len)
{
- int sign_algo_cnt = le16_to_cpu(pneg_ctxt->SigningAlgorithmCount);
- int i, sign_alos_size = sign_algo_cnt * sizeof(__le16);
+ int sign_algo_cnt;
+ int i, sign_alos_size;
+
+ if (sizeof(struct smb2_signing_capabilities) > ctxt_len) {
+ pr_err("Invalid SMB2_SIGNING_CAPABILITIES context length\n");
+ return;
+ }
conn->signing_negotiated = false;
+ sign_algo_cnt = le16_to_cpu(pneg_ctxt->SigningAlgorithmCount);
+ sign_alos_size = sign_algo_cnt * sizeof(__le16);
if (sizeof(struct smb2_signing_capabilities) + sign_alos_size >
- len_of_ctxts) {
+ ctxt_len) {
pr_err("Invalid signing algorithm count(%d)\n", sign_algo_cnt);
return;
}
@@ -969,18 +984,16 @@ static __le32 deassemble_neg_contexts(struct
ksmbd_conn *conn,
len_of_ctxts = len_of_smb - offset;
while (i++ < neg_ctxt_cnt) {
- int clen;
-
- /* check that offset is not beyond end of SMB */
- if (len_of_ctxts == 0)
- break;
+ int clen, ctxt_len;
if (len_of_ctxts < sizeof(struct smb2_neg_context))
break;
pctx = (struct smb2_neg_context *)((char *)pctx + offset);
clen = le16_to_cpu(pctx->DataLength);
- if (clen + sizeof(struct smb2_neg_context) > len_of_ctxts)
+ ctxt_len = clen + sizeof(struct smb2_neg_context);
+
+ if (ctxt_len > len_of_ctxts)
break;
if (pctx->ContextType == SMB2_PREAUTH_INTEGRITY_CAPABILITIES) {
@@ -991,7 +1004,7 @@ static __le32 deassemble_neg_contexts(struct
ksmbd_conn *conn,
status = decode_preauth_ctxt(conn,
(struct smb2_preauth_neg_context *)pctx,
- len_of_ctxts);
+ ctxt_len);
if (status != STATUS_SUCCESS)
break;
} else if (pctx->ContextType == SMB2_ENCRYPTION_CAPABILITIES) {
@@ -1002,7 +1015,7 @@ static __le32 deassemble_neg_contexts(struct
ksmbd_conn *conn,
decode_encrypt_ctxt(conn,
(struct smb2_encryption_neg_context *)pctx,
- len_of_ctxts);
+ ctxt_len);
} else if (pctx->ContextType == SMB2_COMPRESSION_CAPABILITIES) {
ksmbd_debug(SMB,
"deassemble SMB2_COMPRESSION_CAPABILITIES context\n");
@@ -1021,9 +1034,10 @@ static __le32 deassemble_neg_contexts(struct
ksmbd_conn *conn,
} else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES) {
ksmbd_debug(SMB,
"deassemble SMB2_SIGNING_CAPABILITIES context\n");
+
decode_sign_cap_ctxt(conn,
(struct smb2_signing_capabilities *)pctx,
- len_of_ctxts);
+ ctxt_len);
}
/* offsets must be 8 byte aligned */
---
Namjae Jeon <linkinjeon@kernel.org> 於 2023年5月18日 週四 下午2:36寫道：
>
> 2023-05-18 15:30 GMT+09:00, Hex Rabbit <h3xrabbit@gmail.com>:
> >> You need to consider Ciphers flex-array size to validate ctxt_len. we
> >> can get its size using CipherCount in smb2_encryption_neg_context.
> >
> > I'm not checking the flex-array size since both `decode_sign_cap_ctxt()`
> > and `decode_encrypt_ctxt()` have done it, or should I move it out?
> Yes, We can move it out. Thanks.
> >
> > ```
> > if (sizeof(struct smb2_encryption_neg_context) + cphs_size >
> > len_of_ctxts) {
> > pr_err("Invalid cipher count(%d)\n", cph_cnt);
> > return;
> > }
> > ```
> >
> > ```
> > if (sizeof(struct smb2_signing_capabilities) + sign_alos_size >
> > len_of_ctxts) {
> > pr_err("Invalid signing algorithm count(%d)\n", sign_algo_cnt);
> > return;
> > }
> > ```
> >
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
2023-05-18 6:30 ` Hex Rabbit
@ 2023-05-18 6:36 ` Namjae Jeon
2023-05-18 8:11 ` Hex Rabbit
0 siblings, 1 reply; 13+ messages in thread
From: Namjae Jeon @ 2023-05-18 6:36 UTC (permalink / raw)
To: Hex Rabbit; +Cc: sfrench, senozhatsky, tom, linux-cifs
2023-05-18 15:30 GMT+09:00, Hex Rabbit <h3xrabbit@gmail.com>:
>> You need to consider Ciphers flex-array size to validate ctxt_len. we
>> can get its size using CipherCount in smb2_encryption_neg_context.
>
> I'm not checking the flex-array size since both `decode_sign_cap_ctxt()`
> and `decode_encrypt_ctxt()` have done it, or should I move it out?
Yes, We can move it out. Thanks.
>
> ```
> if (sizeof(struct smb2_encryption_neg_context) + cphs_size >
> len_of_ctxts) {
> pr_err("Invalid cipher count(%d)\n", cph_cnt);
> return;
> }
> ```
>
> ```
> if (sizeof(struct smb2_signing_capabilities) + sign_alos_size >
> len_of_ctxts) {
> pr_err("Invalid signing algorithm count(%d)\n", sign_algo_cnt);
> return;
> }
> ```
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
2023-05-18 5:45 ` Namjae Jeon
@ 2023-05-18 6:30 ` Hex Rabbit
2023-05-18 6:36 ` Namjae Jeon
0 siblings, 1 reply; 13+ messages in thread
From: Hex Rabbit @ 2023-05-18 6:30 UTC (permalink / raw)
To: Namjae Jeon; +Cc: sfrench, senozhatsky, tom, linux-cifs
> You need to consider Ciphers flex-array size to validate ctxt_len. we
> can get its size using CipherCount in smb2_encryption_neg_context.
I'm not checking the flex-array size since both `decode_sign_cap_ctxt()`
and `decode_encrypt_ctxt()` have done it, or should I move it out?
```
if (sizeof(struct smb2_encryption_neg_context) + cphs_size >
len_of_ctxts) {
pr_err("Invalid cipher count(%d)\n", cph_cnt);
return;
}
```
```
if (sizeof(struct smb2_signing_capabilities) + sign_alos_size >
len_of_ctxts) {
pr_err("Invalid signing algorithm count(%d)\n", sign_algo_cnt);
return;
}
```
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
2023-05-17 18:58 HexRabbit
2023-05-17 19:02 ` Jeremy Allison
@ 2023-05-18 5:45 ` Namjae Jeon
2023-05-18 6:30 ` Hex Rabbit
1 sibling, 1 reply; 13+ messages in thread
From: Namjae Jeon @ 2023-05-18 5:45 UTC (permalink / raw)
To: HexRabbit; +Cc: sfrench, senozhatsky, tom, linux-cifs
2023-05-18 3:58 GMT+09:00, HexRabbit <h3xrabbit@gmail.com>:
> From: Kuan-Ting Chen <h3xrabbit@gmail.com>
>
> Ensure the context's length is valid (excluding VLAs) before casting the
> pointer to the corresponding structure pointer type, also removed
> redundant check on `len_of_ctxts`.
>
> Signed-off-by: Kuan-Ting Chen <h3xrabbit@gmail.com>
> ---
> fs/ksmbd/smb2pdu.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 972176bff..83b877254 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -969,18 +969,16 @@ static __le32 deassemble_neg_contexts(struct
> ksmbd_conn *conn,
> len_of_ctxts = len_of_smb - offset;
>
> while (i++ < neg_ctxt_cnt) {
> - int clen;
> -
> - /* check that offset is not beyond end of SMB */
> - if (len_of_ctxts == 0)
> - break;
> + int clen, ctxt_len;
>
> if (len_of_ctxts < sizeof(struct smb2_neg_context))
> break;
>
> pctx = (struct smb2_neg_context *)((char *)pctx + offset);
> clen = le16_to_cpu(pctx->DataLength);
> - if (clen + sizeof(struct smb2_neg_context) > len_of_ctxts)
> + ctxt_len = clen + sizeof(struct smb2_neg_context);
> +
> + if (ctxt_len > len_of_ctxts)
> break;
>
> if (pctx->ContextType == SMB2_PREAUTH_INTEGRITY_CAPABILITIES) {
> @@ -989,6 +987,9 @@ static __le32 deassemble_neg_contexts(struct ksmbd_conn
> *conn,
> if (conn->preauth_info->Preauth_HashId)
> break;
>
> + if (ctxt_len < sizeof(struct smb2_preauth_neg_context))
> + break;
> +
> status = decode_preauth_ctxt(conn,
> (struct smb2_preauth_neg_context *)pctx,
> len_of_ctxts);
> @@ -1000,6 +1001,9 @@ static __le32 deassemble_neg_contexts(struct
> ksmbd_conn *conn,
> if (conn->cipher_type)
> break;
>
> + if (ctxt_len < sizeof(struct smb2_encryption_neg_context))
You need to consider Ciphers flex-array size to validate ctxt_len. we
can get its size using CipherCount in smb2_encryption_neg_context.
> + break;
> +
> decode_encrypt_ctxt(conn,
> (struct smb2_encryption_neg_context *)pctx,
> len_of_ctxts);
> @@ -1009,6 +1013,9 @@ static __le32 deassemble_neg_contexts(struct
> ksmbd_conn *conn,
> if (conn->compress_algorithm)
> break;
>
> + if (ctxt_len < sizeof(struct smb2_compression_capabilities_context))
Ditto.
> + break;
> +
> decode_compress_ctxt(conn,
> (struct smb2_compression_capabilities_context *)pctx);
> } else if (pctx->ContextType == SMB2_NETNAME_NEGOTIATE_CONTEXT_ID) {
> @@ -1021,6 +1028,10 @@ static __le32 deassemble_neg_contexts(struct
> ksmbd_conn *conn,
> } else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES) {
> ksmbd_debug(SMB,
> "deassemble SMB2_SIGNING_CAPABILITIES context\n");
> +
> + if (ctxt_len < sizeof(struct smb2_signing_capabilities))
Ditto.
Thanks.
> + break;
> +
> decode_sign_cap_ctxt(conn,
> (struct smb2_signing_capabilities *)pctx,
> len_of_ctxts);
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
2023-05-18 0:37 ` Sergey Senozhatsky
@ 2023-05-18 0:46 ` Jeremy Allison
0 siblings, 0 replies; 13+ messages in thread
From: Jeremy Allison @ 2023-05-18 0:46 UTC (permalink / raw)
To: Sergey Senozhatsky; +Cc: Hex Rabbit, linkinjeon, sfrench, tom, linux-cifs
On Thu, May 18, 2023 at 09:37:48AM +0900, Sergey Senozhatsky wrote:
>On (23/05/18 03:28), Hex Rabbit wrote:
>>
>> Maybe it might be worth addressing these issues in the upcoming patches.
>>
>
>I guess that would be nice to see
More than nice. Essential IMHO.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
[not found] ` <CAF3ZFedA=Q+ghkKNR=wnbNQ63LXPwagetmez0KqZthMySBNTJA@mail.gmail.com>
@ 2023-05-18 0:37 ` Sergey Senozhatsky
2023-05-18 0:46 ` Jeremy Allison
0 siblings, 1 reply; 13+ messages in thread
From: Sergey Senozhatsky @ 2023-05-18 0:37 UTC (permalink / raw)
To: Hex Rabbit
Cc: Jeremy Allison, linkinjeon, sfrench, senozhatsky, tom, linux-cifs
On (23/05/18 03:28), Hex Rabbit wrote:
>
> Maybe it might be worth addressing these issues in the upcoming patches.
>
I guess that would be nice to see
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
2023-05-17 18:58 HexRabbit
@ 2023-05-17 19:02 ` Jeremy Allison
[not found] ` <CAF3ZFedA=Q+ghkKNR=wnbNQ63LXPwagetmez0KqZthMySBNTJA@mail.gmail.com>
2023-05-18 5:45 ` Namjae Jeon
1 sibling, 1 reply; 13+ messages in thread
From: Jeremy Allison @ 2023-05-17 19:02 UTC (permalink / raw)
To: HexRabbit; +Cc: linkinjeon, sfrench, senozhatsky, tom, linux-cifs
On Wed, May 17, 2023 at 06:58:20PM +0000, HexRabbit wrote:
>From: Kuan-Ting Chen <h3xrabbit@gmail.com>
>
>Ensure the context's length is valid (excluding VLAs) before casting the
>pointer to the corresponding structure pointer type, also removed
>redundant check on `len_of_ctxts`.
>
>Signed-off-by: Kuan-Ting Chen <h3xrabbit@gmail.com>
>---
> fs/ksmbd/smb2pdu.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
>diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>index 972176bff..83b877254 100644
>--- a/fs/ksmbd/smb2pdu.c
>+++ b/fs/ksmbd/smb2pdu.c
>@@ -969,18 +969,16 @@ static __le32 deassemble_neg_contexts(struct ksmbd_conn *conn,
> len_of_ctxts = len_of_smb - offset;
>
> while (i++ < neg_ctxt_cnt) {
>- int clen;
>-
>- /* check that offset is not beyond end of SMB */
>- if (len_of_ctxts == 0)
>- break;
>+ int clen, ctxt_len;
Just a drive-by comment here (haven't had time to look at the
underlying code).
Should lengths in protocol parsing *ever* be defined at 'int' ?
IMHO no, never. That's a disaster waiting to happen as int
overflow driven by the peer can often cause integer wrap to
negative, leaving all the nice "not greater than packet length"
to fail horribly. We excised all 'int' length representations
from the Samba parser a long time ago.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding
@ 2023-05-17 18:58 HexRabbit
2023-05-17 19:02 ` Jeremy Allison
2023-05-18 5:45 ` Namjae Jeon
0 siblings, 2 replies; 13+ messages in thread
From: HexRabbit @ 2023-05-17 18:58 UTC (permalink / raw)
To: linkinjeon; +Cc: sfrench, senozhatsky, tom, linux-cifs, Kuan-Ting Chen
From: Kuan-Ting Chen <h3xrabbit@gmail.com>
Ensure the context's length is valid (excluding VLAs) before casting the
pointer to the corresponding structure pointer type, also removed
redundant check on `len_of_ctxts`.
Signed-off-by: Kuan-Ting Chen <h3xrabbit@gmail.com>
---
fs/ksmbd/smb2pdu.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 972176bff..83b877254 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -969,18 +969,16 @@ static __le32 deassemble_neg_contexts(struct ksmbd_conn *conn,
len_of_ctxts = len_of_smb - offset;
while (i++ < neg_ctxt_cnt) {
- int clen;
-
- /* check that offset is not beyond end of SMB */
- if (len_of_ctxts == 0)
- break;
+ int clen, ctxt_len;
if (len_of_ctxts < sizeof(struct smb2_neg_context))
break;
pctx = (struct smb2_neg_context *)((char *)pctx + offset);
clen = le16_to_cpu(pctx->DataLength);
- if (clen + sizeof(struct smb2_neg_context) > len_of_ctxts)
+ ctxt_len = clen + sizeof(struct smb2_neg_context);
+
+ if (ctxt_len > len_of_ctxts)
break;
if (pctx->ContextType == SMB2_PREAUTH_INTEGRITY_CAPABILITIES) {
@@ -989,6 +987,9 @@ static __le32 deassemble_neg_contexts(struct ksmbd_conn *conn,
if (conn->preauth_info->Preauth_HashId)
break;
+ if (ctxt_len < sizeof(struct smb2_preauth_neg_context))
+ break;
+
status = decode_preauth_ctxt(conn,
(struct smb2_preauth_neg_context *)pctx,
len_of_ctxts);
@@ -1000,6 +1001,9 @@ static __le32 deassemble_neg_contexts(struct ksmbd_conn *conn,
if (conn->cipher_type)
break;
+ if (ctxt_len < sizeof(struct smb2_encryption_neg_context))
+ break;
+
decode_encrypt_ctxt(conn,
(struct smb2_encryption_neg_context *)pctx,
len_of_ctxts);
@@ -1009,6 +1013,9 @@ static __le32 deassemble_neg_contexts(struct ksmbd_conn *conn,
if (conn->compress_algorithm)
break;
+ if (ctxt_len < sizeof(struct smb2_compression_capabilities_context))
+ break;
+
decode_compress_ctxt(conn,
(struct smb2_compression_capabilities_context *)pctx);
} else if (pctx->ContextType == SMB2_NETNAME_NEGOTIATE_CONTEXT_ID) {
@@ -1021,6 +1028,10 @@ static __le32 deassemble_neg_contexts(struct ksmbd_conn *conn,
} else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES) {
ksmbd_debug(SMB,
"deassemble SMB2_SIGNING_CAPABILITIES context\n");
+
+ if (ctxt_len < sizeof(struct smb2_signing_capabilities))
+ break;
+
decode_sign_cap_ctxt(conn,
(struct smb2_signing_capabilities *)pctx,
len_of_ctxts);
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-05-19 5:17 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-18 14:42 [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding HexRabbit
2023-05-19 1:37 ` Namjae Jeon
2023-05-19 4:34 ` Hex Rabbit
2023-05-19 5:16 ` Namjae Jeon
-- strict thread matches above, loose matches on Subject: below --
2023-05-17 18:58 HexRabbit
2023-05-17 19:02 ` Jeremy Allison
[not found] ` <CAF3ZFedA=Q+ghkKNR=wnbNQ63LXPwagetmez0KqZthMySBNTJA@mail.gmail.com>
2023-05-18 0:37 ` Sergey Senozhatsky
2023-05-18 0:46 ` Jeremy Allison
2023-05-18 5:45 ` Namjae Jeon
2023-05-18 6:30 ` Hex Rabbit
2023-05-18 6:36 ` Namjae Jeon
2023-05-18 8:11 ` Hex Rabbit
2023-05-18 14:10 ` Namjae Jeon
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.