Hi Marios,
2021-10-15 22:02 GMT+09:00, Marios Makassikis <mmakassikis@xxxxxxxxxx>:
> Make sure the security buffer's length/offset are valid with regards to
> the packet length.
>
> Signed-off-by: Marios Makassikis <mmakassikis@xxxxxxxxxx>
> ---
> fs/ksmbd/smb2pdu.c | 51 ++++++++++++++++++++++++++++------------------
> 1 file changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 005aa93a49d6..5fc766439f0f 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -1274,19 +1274,13 @@ static int generate_preauth_hash(struct ksmbd_work
> *work)
> return 0;
> }
>
> -static int decode_negotiation_token(struct ksmbd_work *work,
> - struct negotiate_message *negblob)
> +static int decode_negotiation_token(struct ksmbd_conn *conn,
> + struct negotiate_message *negblob,
> + size_t sz)
> {
> - struct ksmbd_conn *conn = work->conn;
> - struct smb2_sess_setup_req *req;
> - int sz;
> -
> if (!conn->use_spnego)
> return -EINVAL;
>
> - req = work->request_buf;
> - sz = le16_to_cpu(req->SecurityBufferLength);
> -
> if (ksmbd_decode_negTokenInit((char *)negblob, sz, conn)) {
> if (ksmbd_decode_negTokenTarg((char *)negblob, sz, conn)) {
> conn->auth_mechs |= KSMBD_AUTH_NTLMSSP;
> @@ -1298,9 +1292,9 @@ static int decode_negotiation_token(struct ksmbd_work
> *work,
> }
>
> static int ntlm_negotiate(struct ksmbd_work *work,
> - struct negotiate_message *negblob)
> + struct negotiate_message *negblob,
> + size_t negblob_len)
> {
> - struct smb2_sess_setup_req *req = work->request_buf;
> struct smb2_sess_setup_rsp *rsp = work->response_buf;
> struct challenge_message *chgblob;
> unsigned char *spnego_blob = NULL;
> @@ -1309,8 +1303,7 @@ static int ntlm_negotiate(struct ksmbd_work *work,
> int sz, rc;
>
> ksmbd_debug(SMB, "negotiate phase\n");
> - sz = le16_to_cpu(req->SecurityBufferLength);
> - rc = ksmbd_decode_ntlmssp_neg_blob(negblob, sz, work->sess);
> + rc = ksmbd_decode_ntlmssp_neg_blob(negblob, negblob_len, work->sess);
> if (rc)
> return rc;
>
> @@ -1378,12 +1371,20 @@ static struct ksmbd_user *session_user(struct
> ksmbd_conn *conn,
> struct authenticate_message *authblob;
> struct ksmbd_user *user;
> char *name;
> - int sz;
> + unsigned int name_off, name_len, pdu_len;
>
> authblob = user_authblob(conn, req);
we need to validate UserName first. so..
if (le16_to_cpu(req->SecurityBufferLength) < sizeof(struct
authenticate_message)) {
ksmbd_debug(SMB, "blob len %d too small\n",
le16_to_cpu(req->SecurityBufferLength));
return NULL;
}
> - sz = le32_to_cpu(authblob->UserName.BufferOffset);
> - name = smb_strndup_from_utf16((const char *)authblob + sz,
> - le16_to_cpu(authblob->UserName.Length),
> + pdu_len = get_rfc1002_len(req) + 4;
> + name_off = le32_to_cpu(authblob->UserName.BufferOffset);
> + name_len = le16_to_cpu(authblob->UserName.Length);
> +
> + if (((char *)authblob + name_off > (char *)req + pdu_len) ||
> + (sizeof(struct smb2_sess_setup_req) +
> + sizeof(struct authenticate_message) + name_len > pdu_len))
> + return NULL;
Can we make it simpler ?
auth_msg_len = le16_to_cpu(req->SecurityBufferOffset) +
le16_to_cpu(req->SecurityBufferLength);
if (auth_msg_len < (u64)name_off + name_len)
return NULL;
And We need to validate the following ones in the same way in
ksmbd_decode_ntlmssp_auth_blob().
struct security_buffer NtChallengeResponse;
struct security_buffer DomainName;
> +
> + name = smb_strndup_from_utf16((const char *)authblob + name_off,
> + name_len,
> true,
> conn->local_nls);
> if (IS_ERR(name)) {
> @@ -1629,6 +1630,7 @@ int smb2_sess_setup(struct ksmbd_work *work)
> struct smb2_sess_setup_rsp *rsp = work->response_buf;
> struct ksmbd_session *sess;
> struct negotiate_message *negblob;
> + unsigned int pdu_length, negblob_len, negblob_off;
> int rc = 0;
>
> ksmbd_debug(SMB, "Received request for session setup\n");
> @@ -1709,10 +1711,19 @@ int smb2_sess_setup(struct ksmbd_work *work)
> if (sess->state == SMB2_SESSION_EXPIRED)
> sess->state = SMB2_SESSION_IN_PROGRESS;
>
> + pdu_length = get_rfc1002_len(req);
> + negblob_off = le16_to_cpu(req->SecurityBufferOffset);
> + negblob_len = le16_to_cpu(req->SecurityBufferLength);
> + if (negblob_off != (offsetof(struct smb2_sess_setup_req, Buffer) - 4))
if (negblob_off < (offsetof(struct smb2_sess_setup_req, Buffer) - 4))
> + return -EINVAL;
> +
> + /* account for Buffer[1] and smb2_buf_length */
> + if (sizeof(struct smb2_sess_setup_req) + negblob_len - 1 > pdu_length +
> 4)
> + return -EINVAL;
There is same check in ksmbd_smb2_check_message(). Please check it.
Thanks!
> negblob = (struct negotiate_message *)((char *)&req->hdr.ProtocolId +
> - le16_to_cpu(req->SecurityBufferOffset));
> + negblob_off);
>
> - if (decode_negotiation_token(work, negblob) == 0) {
> + if (decode_negotiation_token(conn, negblob, negblob_len) == 0) {
> if (conn->mechToken)
> negblob = (struct negotiate_message *)conn->mechToken;
> }
> @@ -1736,7 +1747,7 @@ int smb2_sess_setup(struct ksmbd_work *work)
> sess->Preauth_HashValue = NULL;
> } else if (conn->preferred_auth_mech == KSMBD_AUTH_NTLMSSP) {
> if (negblob->MessageType == NtLmNegotiate) {
> - rc = ntlm_negotiate(work, negblob);
> + rc = ntlm_negotiate(work, negblob, negblob_len);
> if (rc)
> goto out_err;
> rsp->hdr.Status =
> --
> 2.25.1
>
>
