diff mbox series

[v2] ksmbd: add buffer validation in session setup

Message ID 20211016235715.3469969-1-mmakassikis@freebox.fr
State New
Headers show
Series [v2] ksmbd: add buffer validation in session setup | expand

Commit Message

Marios Makassikis Oct. 16, 2021, 11:57 p.m. UTC 
Make sure the security buffer's length/offset are valid with regards to
the packet length.

Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
---
Changes since v1:
 - check that securitybuffer is large enough to hold a struct
   authenticate_message in session_user() 
 - simplified check for UserName field
 - added check for NtChallengeResponse and DomainName in
   ksmbd_decode_ntlmssp_auth_blob()
 - removed check in smb2_sess_setup() that is redundant with
   ksmbd_smb2_check_message()
 
 As an aside, I find auditing the current code for missing checks to be quite
 difficult. The received buffer is cast to some struct right before usage and
 it's unclear whether the values read using the le*_to_cpu() macros have been
 validated. For example, ksmbd_decode_ntlmssp_auth_blob() has a blob_len
 parameter. Fortunately, there's a single call-site, but it looks like this:

	struct authenticate_message *authblob;

	authblob = user_authblob(conn, req);
	sz = le16_to_cpu(req->SecurityBufferLength);
	rc = ksmbd_decode_ntlmssp_auth_blob(authblob, sz, sess);

It's not immediately obvious that 'sz' here is a valid value. The actual check
is done in ksmbd_smb2_check_message().

I wonder if we wouldn't be better off carrying a size and offset variable
alongside the request buffer and decode the request header into a struct as we
go. I'm afraid there are checks missing -- or that future changes may not
properly validate the input buffer.

 fs/ksmbd/auth.c    | 18 +++++++++++------
 fs/ksmbd/smb2pdu.c | 50 +++++++++++++++++++++++++++-------------------
 2 files changed, 42 insertions(+), 26 deletions(-)

Comments

Namjae Jeon Oct. 18, 2021, 2:04 p.m. UTC | #1 
Hi Marios,
> +	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))
> +		return -EINVAL;
Like the following code, negblob is still used without buffer check.
We need to add buffer check for it here ?

if (negblob->MessageType == NtLmNegotiate) {

} else if (negblob->MessageType == NtLmAuthenticate) {

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 +1746,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
>
>
Marios Makassikis Oct. 18, 2021, 2:49 p.m. UTC | #2 
On Mon, Oct 18, 2021 at 4:04 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> Hi Marios,
> > +     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))
> > +             return -EINVAL;
> Like the following code, negblob is still used without buffer check.
> We need to add buffer check for it here ?
>
> if (negblob->MessageType == NtLmNegotiate) {
>
> } else if (negblob->MessageType == NtLmAuthenticate) {
>
> Thanks!
>
Hello Namjae,

I'm not sure I understand what you mean. Should I change the check to
something like this ?

+       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) ||
+           negblob_len < sizeof(struct negotiate_message))

Marios

> > +
> >       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 +1746,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
> >
> >
Namjae Jeon Oct. 18, 2021, 11:17 p.m. UTC | #3 
2021-10-18 23:49 GMT+09:00, Marios Makassikis <mmakassikis@freebox.fr>:
> On Mon, Oct 18, 2021 at 4:04 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
>>
>> Hi Marios,
>> > +     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))
>> > +             return -EINVAL;
>> Like the following code, negblob is still used without buffer check.
>> We need to add buffer check for it here ?
>>
>> if (negblob->MessageType == NtLmNegotiate) {
>>
>> } else if (negblob->MessageType == NtLmAuthenticate) {
>>
>> Thanks!
>>
> Hello Namjae,
>
> I'm not sure I understand what you mean. Should I change the check to
> something like this ?
>
> +       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)
> ||
> +           negblob_len < sizeof(struct negotiate_message))
negblob_len < offsetof(struct negotiate_message, NegotiateFlags))

Thanks!

>
> Marios
>
>> > +
>> >       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 +1746,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
>> >
>> >
>
diff mbox series

Patch

diff --git a/fs/ksmbd/auth.c b/fs/ksmbd/auth.c
index 71c989f1568d..e3c54888544f 100644
--- a/fs/ksmbd/auth.c
+++ b/fs/ksmbd/auth.c
@@ -298,8 +298,9 @@  int ksmbd_decode_ntlmssp_auth_blob(struct authenticate_message *authblob,
 				   int blob_len, struct ksmbd_session *sess)
 {
 	char *domain_name;
-	unsigned int lm_off, nt_off;
-	unsigned short nt_len;
+	uintptr_t auth_msg_off;
+	unsigned int lm_off, nt_off, dn_off;
+	unsigned short nt_len, dn_len;
 	int ret;
 
 	if (blob_len < sizeof(struct authenticate_message)) {
@@ -314,15 +315,20 @@  int ksmbd_decode_ntlmssp_auth_blob(struct authenticate_message *authblob,
 		return -EINVAL;
 	}
 
+	auth_msg_off = (uintptr_t)((char *)authblob + blob_len);
 	lm_off = le32_to_cpu(authblob->LmChallengeResponse.BufferOffset);
 	nt_off = le32_to_cpu(authblob->NtChallengeResponse.BufferOffset);
 	nt_len = le16_to_cpu(authblob->NtChallengeResponse.Length);
+	dn_off = le32_to_cpu(authblob->DomainName.BufferOffset);
+	dn_len = le16_to_cpu(authblob->DomainName.Length);
+
+	if (auth_msg_off < (u64)dn_off + dn_len ||
+	    auth_msg_off < (u64)nt_off + nt_len)
+		return -EINVAL;
 
 	/* TODO : use domain name that imported from configuration file */
-	domain_name = smb_strndup_from_utf16((const char *)authblob +
-			le32_to_cpu(authblob->DomainName.BufferOffset),
-			le16_to_cpu(authblob->DomainName.Length), true,
-			sess->conn->local_nls);
+	domain_name = smb_strndup_from_utf16((const char *)authblob + dn_off,
+			dn_len, true, sess->conn->local_nls);
 	if (IS_ERR(domain_name))
 		return PTR_ERR(domain_name);
 
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 005aa93a49d6..c4e84537b5ca 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,23 @@  static struct ksmbd_user *session_user(struct ksmbd_conn *conn,
 	struct authenticate_message *authblob;
 	struct ksmbd_user *user;
 	char *name;
-	int sz;
+	unsigned int auth_msg_len, name_off, name_len, secbuf_len;
 
+	secbuf_len = le16_to_cpu(req->SecurityBufferLength);
+	if (secbuf_len < sizeof(struct authenticate_message)) {
+		ksmbd_debug(SMB, "blob len %d too small\n", secbuf_len);
+		return NULL;
+	}
 	authblob = user_authblob(conn, req);
-	sz = le32_to_cpu(authblob->UserName.BufferOffset);
-	name = smb_strndup_from_utf16((const char *)authblob + sz,
-				      le16_to_cpu(authblob->UserName.Length),
+	name_off = le32_to_cpu(authblob->UserName.BufferOffset);
+	name_len = le16_to_cpu(authblob->UserName.Length);
+	auth_msg_len = le16_to_cpu(req->SecurityBufferOffset) + secbuf_len;
+
+	if (auth_msg_len < (u64)name_off + name_len)
+		return NULL;
+
+	name = smb_strndup_from_utf16((const char *)authblob + name_off,
+				      name_len,
 				      true,
 				      conn->local_nls);
 	if (IS_ERR(name)) {
@@ -1629,6 +1633,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 negblob_len, negblob_off;
 	int rc = 0;
 
 	ksmbd_debug(SMB, "Received request for session setup\n");
@@ -1709,10 +1714,15 @@  int smb2_sess_setup(struct ksmbd_work *work)
 	if (sess->state == SMB2_SESSION_EXPIRED)
 		sess->state = SMB2_SESSION_IN_PROGRESS;
 
+	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))
+		return -EINVAL;
+
 	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 +1746,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 =