| Message ID | 20211019083641.116783-1-mmakassikis@freebox.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] ksmbd: add buffer validation in session setup | expand |
2021-10-19 17:36 GMT+09:00, Marios Makassikis <mmakassikis@freebox.fr>: > 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 v2: > - check that negblob in smb2_sess_setup() is large enough to access the > MessageType field > > fs/ksmbd/auth.c | 18 ++++++++++------ > fs/ksmbd/smb2pdu.c | 51 ++++++++++++++++++++++++++++------------------ > 2 files changed, 43 insertions(+), 26 deletions(-) > > diff --git a/fs/ksmbd/auth.c b/fs/ksmbd/auth.c > index 71c989f1568d..c9d32fea5669 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); Let's remove unused lm_off. > 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 || It should be blob_len instead of auth_msg_off ? > + 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..f02766b1e9ce 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,16 @@ 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) || > + negblob_len < offsetof(struct negotiate_message, NegotiateFlags)) > + 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 +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 > >
On Tue, Oct 19, 2021 at 4:35 PM Namjae Jeon <linkinjeon@kernel.org> wrote: > > 2021-10-19 17:36 GMT+09:00, Marios Makassikis <mmakassikis@freebox.fr>: > > 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 v2: > > - check that negblob in smb2_sess_setup() is large enough to access the > > MessageType field > > > > fs/ksmbd/auth.c | 18 ++++++++++------ > > fs/ksmbd/smb2pdu.c | 51 ++++++++++++++++++++++++++++------------------ > > 2 files changed, 43 insertions(+), 26 deletions(-) > > > > diff --git a/fs/ksmbd/auth.c b/fs/ksmbd/auth.c > > index 71c989f1568d..c9d32fea5669 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); > Let's remove unused lm_off. ok > > 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 || > It should be blob_len instead of auth_msg_off ? Good catch, auth_msg_off is completely wrong here. > > + 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..f02766b1e9ce 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,16 @@ 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) || > > + negblob_len < offsetof(struct negotiate_message, NegotiateFlags)) > > + 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 +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 > > > >
diff --git a/fs/ksmbd/auth.c b/fs/ksmbd/auth.c index 71c989f1568d..c9d32fea5669 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..f02766b1e9ce 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,16 @@ 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) || + negblob_len < offsetof(struct negotiate_message, NegotiateFlags)) + 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 +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 =
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 v2: - check that negblob in smb2_sess_setup() is large enough to access the MessageType field fs/ksmbd/auth.c | 18 ++++++++++------ fs/ksmbd/smb2pdu.c | 51 ++++++++++++++++++++++++++++------------------ 2 files changed, 43 insertions(+), 26 deletions(-)