Re: [PATCH] ksmbd: fix multiple out-of-bounds read during context decoding

[Hex Rabbit <h3xrabbit@xxxxxxxxx>]

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@xxxxxxxxxx> 於 2023年5月18日 週四 下午2:36寫道：
>
> 2023-05-18 15:30 GMT+09:00, Hex Rabbit <h3xrabbit@xxxxxxxxx>:
> >> 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;
> > }
> > ```
> >