diff mbox series

ksmbd: fix multiple out-of-bounds read during context decoding

Message ID 20230518144208.2099772-1-h3xrabbit@gmail.com (mailing list archive)
State New, archived
Headers show
Series ksmbd: fix multiple out-of-bounds read during context decoding | expand

Commit Message

Hex Rabbit May 18, 2023, 2:42 p.m. UTC 
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(-)

Comments

Namjae Jeon May 19, 2023, 1:37 a.m. UTC | #1 
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;
Hex Rabbit May 19, 2023, 4:34 a.m. UTC | #2 
>          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
Namjae Jeon May 19, 2023, 5:16 a.m. UTC | #3 
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
>
diff mbox series

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 */