| Message ID | 20210921225109.6388-2-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] ksmbd: remove RFC1002 check in smb2 request | expand |
Hi Namjae patch looks great! Few nitpicks below. Am 22.09.21 um 00:51 schrieb Namjae Jeon: > This patch add validation to check request buffer check in smb2 > negotiate. > > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > Cc: Ralph Böhme <slow@samba.org> > Cc: Steve French <smfrench@gmail.com> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > --- > fs/ksmbd/smb2pdu.c | 41 ++++++++++++++++++++++++++++++++++++++++- > fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++-- > 2 files changed, 60 insertions(+), 3 deletions(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index baf7ce31d557..1fe37ad4e5bc 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -1071,7 +1071,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work) > struct ksmbd_conn *conn = work->conn; > struct smb2_negotiate_req *req = work->request_buf; > struct smb2_negotiate_rsp *rsp = work->response_buf; > - int rc = 0; > + int rc = 0, smb2_buf_len, smb2_neg_size; I guess all len variables should use unsigned types to facilitate well defined overflow checks. > __le32 status; > > ksmbd_debug(SMB, "Received negotiate request\n"); > @@ -1089,6 +1089,45 @@ int smb2_handle_negotiate(struct ksmbd_work *work) > goto err_out; > } > > + smb2_buf_len = get_rfc1002_len(work->request_buf); > + smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects) - 4; > + if (conn->dialect == SMB311_PROT_ID) { > + int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset); > + int nego_ctxt_count = le16_to_cpu(req->NegotiateContextCount); > + > + if (smb2_buf_len < nego_ctxt_off + nego_ctxt_count) { overflow check needed for 32 bit arch? > + rsp->hdr.Status = STATUS_INVALID_PARAMETER; > + rc = -EINVAL; > + goto err_out; > + } > + > + if (smb2_neg_size > nego_ctxt_off) { > + rsp->hdr.Status = STATUS_INVALID_PARAMETER; > + rc = -EINVAL; > + goto err_out; > + } > + > + if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) > > + nego_ctxt_off) { > + rsp->hdr.Status = STATUS_INVALID_PARAMETER; > + rc = -EINVAL; > + goto err_out; > + } > + } else { > + if (smb2_neg_size > smb2_buf_len) { > + rsp->hdr.Status = STATUS_INVALID_PARAMETER; > + rc = -EINVAL; > + goto err_out; > + } > + > + if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) > > + smb2_buf_len) { > + rsp->hdr.Status = STATUS_INVALID_PARAMETER; > + rc = -EINVAL; > + goto err_out; > + } > + } > + > conn->cli_cap = le32_to_cpu(req->Capabilities); > switch (conn->dialect) { > case SMB311_PROT_ID: > diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c > index 1da67217698d..da17b21ac685 100644 > --- a/fs/ksmbd/smb_common.c > +++ b/fs/ksmbd/smb_common.c > @@ -229,13 +229,22 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, __le16 dialects_count) > > static int ksmbd_negotiate_smb_dialect(void *buf) > { > - __le32 proto; > + int smb_buf_length = get_rfc1002_len(buf); unsigned Thanks! -slow
2021-09-22 23:17 GMT+09:00, Ralph Boehme <slow@samba.org>: > Hi Namjae > > patch looks great! Few nitpicks below. > > Am 22.09.21 um 00:51 schrieb Namjae Jeon: >> This patch add validation to check request buffer check in smb2 >> negotiate. >> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> Cc: Ralph Böhme <slow@samba.org> >> Cc: Steve French <smfrench@gmail.com> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> --- >> fs/ksmbd/smb2pdu.c | 41 ++++++++++++++++++++++++++++++++++++++++- >> fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++-- >> 2 files changed, 60 insertions(+), 3 deletions(-) >> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> index baf7ce31d557..1fe37ad4e5bc 100644 >> --- a/fs/ksmbd/smb2pdu.c >> +++ b/fs/ksmbd/smb2pdu.c >> @@ -1071,7 +1071,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work) >> struct ksmbd_conn *conn = work->conn; >> struct smb2_negotiate_req *req = work->request_buf; >> struct smb2_negotiate_rsp *rsp = work->response_buf; >> - int rc = 0; >> + int rc = 0, smb2_buf_len, smb2_neg_size; > > I guess all len variables should use unsigned types to facilitate well > defined overflow checks. As Ronnie pointed out, if checking max stream size, will be no problem. I'll fix it though. > >> __le32 status; >> >> ksmbd_debug(SMB, "Received negotiate request\n"); >> @@ -1089,6 +1089,45 @@ int smb2_handle_negotiate(struct ksmbd_work *work) >> goto err_out; >> } >> >> + smb2_buf_len = get_rfc1002_len(work->request_buf); >> + smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects) - 4; >> + if (conn->dialect == SMB311_PROT_ID) { >> + int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset); >> + int nego_ctxt_count = le16_to_cpu(req->NegotiateContextCount); >> + >> + if (smb2_buf_len < nego_ctxt_off + nego_ctxt_count) { > > overflow check needed for 32 bit arch? Okay, will fix it on v3. Thanks! > >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; >> + rc = -EINVAL; >> + goto err_out; >> + } >> + >> + if (smb2_neg_size > nego_ctxt_off) { >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; >> + rc = -EINVAL; >> + goto err_out; >> + } >> + >> + if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) > >> + nego_ctxt_off) { >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; >> + rc = -EINVAL; >> + goto err_out; >> + } >> + } else { >> + if (smb2_neg_size > smb2_buf_len) { >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; >> + rc = -EINVAL; >> + goto err_out; >> + } >> + >> + if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) > >> + smb2_buf_len) { >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; >> + rc = -EINVAL; >> + goto err_out; >> + } >> + } >> + >> conn->cli_cap = le32_to_cpu(req->Capabilities); >> switch (conn->dialect) { >> case SMB311_PROT_ID: >> diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c >> index 1da67217698d..da17b21ac685 100644 >> --- a/fs/ksmbd/smb_common.c >> +++ b/fs/ksmbd/smb_common.c >> @@ -229,13 +229,22 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, >> __le16 dialects_count) >> >> static int ksmbd_negotiate_smb_dialect(void *buf) >> { >> - __le32 proto; >> + int smb_buf_length = get_rfc1002_len(buf); > > unsigned > > Thanks! > -slow > > -- > Ralph Boehme, Samba Team https://samba.org/ > SerNet Samba Team Lead https://sernet.de/en/team-samba > >
On Thu, Sep 23, 2021 at 9:13 AM Namjae Jeon <linkinjeon@kernel.org> wrote: > > 2021-09-22 23:17 GMT+09:00, Ralph Boehme <slow@samba.org>: > > Hi Namjae > > > > patch looks great! Few nitpicks below. > > > > Am 22.09.21 um 00:51 schrieb Namjae Jeon: > >> This patch add validation to check request buffer check in smb2 > >> negotiate. > >> > >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > >> Cc: Ralph Böhme <slow@samba.org> > >> Cc: Steve French <smfrench@gmail.com> > >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > >> --- > >> fs/ksmbd/smb2pdu.c | 41 ++++++++++++++++++++++++++++++++++++++++- > >> fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++-- > >> 2 files changed, 60 insertions(+), 3 deletions(-) > >> > >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > >> index baf7ce31d557..1fe37ad4e5bc 100644 > >> --- a/fs/ksmbd/smb2pdu.c > >> +++ b/fs/ksmbd/smb2pdu.c > >> @@ -1071,7 +1071,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work) > >> struct ksmbd_conn *conn = work->conn; > >> struct smb2_negotiate_req *req = work->request_buf; > >> struct smb2_negotiate_rsp *rsp = work->response_buf; > >> - int rc = 0; > >> + int rc = 0, smb2_buf_len, smb2_neg_size; > > > > I guess all len variables should use unsigned types to facilitate well > > defined overflow checks. > As Ronnie pointed out, if checking max stream size, will be no problem. > I'll fix it though. You should add a check to ksmbd_conn_handler_loop() that the length is < 0x01000000 too. > >> >> __le32 status; > >> > >> ksmbd_debug(SMB, "Received negotiate request\n"); > >> @@ -1089,6 +1089,45 @@ int smb2_handle_negotiate(struct ksmbd_work *work) > >> goto err_out; > >> } > >> > >> + smb2_buf_len = get_rfc1002_len(work->request_buf); > >> + smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects) - 4; > >> + if (conn->dialect == SMB311_PROT_ID) { > >> + int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset); > >> + int nego_ctxt_count = le16_to_cpu(req->NegotiateContextCount); > >> + > >> + if (smb2_buf_len < nego_ctxt_off + nego_ctxt_count) { > > > > overflow check needed for 32 bit arch? > Okay, will fix it on v3. > Thanks! > > > >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; > >> + rc = -EINVAL; > >> + goto err_out; > >> + } > >> + > >> + if (smb2_neg_size > nego_ctxt_off) { > >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; > >> + rc = -EINVAL; > >> + goto err_out; > >> + } > >> + > >> + if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) > > >> + nego_ctxt_off) { > >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; > >> + rc = -EINVAL; > >> + goto err_out; > >> + } > >> + } else { > >> + if (smb2_neg_size > smb2_buf_len) { > >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; > >> + rc = -EINVAL; > >> + goto err_out; > >> + } > >> + > >> + if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) > > >> + smb2_buf_len) { > >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; > >> + rc = -EINVAL; > >> + goto err_out; > >> + } > >> + } > >> + > >> conn->cli_cap = le32_to_cpu(req->Capabilities); > >> switch (conn->dialect) { > >> case SMB311_PROT_ID: > >> diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c > >> index 1da67217698d..da17b21ac685 100644 > >> --- a/fs/ksmbd/smb_common.c > >> +++ b/fs/ksmbd/smb_common.c > >> @@ -229,13 +229,22 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, > >> __le16 dialects_count) > >> > >> static int ksmbd_negotiate_smb_dialect(void *buf) > >> { > >> - __le32 proto; > >> + int smb_buf_length = get_rfc1002_len(buf); > > > > unsigned > > > > Thanks! > > -slow > > > > -- > > Ralph Boehme, Samba Team https://samba.org/ > > SerNet Samba Team Lead https://sernet.de/en/team-samba > > > >
2021-09-23 9:12 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>: > On Thu, Sep 23, 2021 at 9:13 AM Namjae Jeon <linkinjeon@kernel.org> wrote: >> >> 2021-09-22 23:17 GMT+09:00, Ralph Boehme <slow@samba.org>: >> > Hi Namjae >> > >> > patch looks great! Few nitpicks below. >> > >> > Am 22.09.21 um 00:51 schrieb Namjae Jeon: >> >> This patch add validation to check request buffer check in smb2 >> >> negotiate. >> >> >> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> >> Cc: Ralph Böhme <slow@samba.org> >> >> Cc: Steve French <smfrench@gmail.com> >> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> >> --- >> >> fs/ksmbd/smb2pdu.c | 41 ++++++++++++++++++++++++++++++++++++++++- >> >> fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++-- >> >> 2 files changed, 60 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> >> index baf7ce31d557..1fe37ad4e5bc 100644 >> >> --- a/fs/ksmbd/smb2pdu.c >> >> +++ b/fs/ksmbd/smb2pdu.c >> >> @@ -1071,7 +1071,7 @@ int smb2_handle_negotiate(struct ksmbd_work >> >> *work) >> >> struct ksmbd_conn *conn = work->conn; >> >> struct smb2_negotiate_req *req = work->request_buf; >> >> struct smb2_negotiate_rsp *rsp = work->response_buf; >> >> - int rc = 0; >> >> + int rc = 0, smb2_buf_len, smb2_neg_size; >> > >> > I guess all len variables should use unsigned types to facilitate well >> > defined overflow checks. >> As Ronnie pointed out, if checking max stream size, will be no problem. >> I'll fix it though. > > You should add a check to ksmbd_conn_handler_loop() that the length is > < 0x01000000 too. Right, I will! Thanks! > >> >> >> __le32 status; >> >> >> >> ksmbd_debug(SMB, "Received negotiate request\n"); >> >> @@ -1089,6 +1089,45 @@ int smb2_handle_negotiate(struct ksmbd_work >> >> *work) >> >> goto err_out; >> >> } >> >> >> >> + smb2_buf_len = get_rfc1002_len(work->request_buf); >> >> + smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects) - >> >> 4; >> >> + if (conn->dialect == SMB311_PROT_ID) { >> >> + int nego_ctxt_off = >> >> le32_to_cpu(req->NegotiateContextOffset); >> >> + int nego_ctxt_count = >> >> le16_to_cpu(req->NegotiateContextCount); >> >> + >> >> + if (smb2_buf_len < nego_ctxt_off + nego_ctxt_count) { >> > >> > overflow check needed for 32 bit arch? >> Okay, will fix it on v3. >> Thanks! >> > >> >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; >> >> + rc = -EINVAL; >> >> + goto err_out; >> >> + } >> >> + >> >> + if (smb2_neg_size > nego_ctxt_off) { >> >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; >> >> + rc = -EINVAL; >> >> + goto err_out; >> >> + } >> >> + >> >> + if (smb2_neg_size + le16_to_cpu(req->DialectCount) * >> >> sizeof(__le16) > >> >> + nego_ctxt_off) { >> >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; >> >> + rc = -EINVAL; >> >> + goto err_out; >> >> + } >> >> + } else { >> >> + if (smb2_neg_size > smb2_buf_len) { >> >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; >> >> + rc = -EINVAL; >> >> + goto err_out; >> >> + } >> >> + >> >> + if (smb2_neg_size + le16_to_cpu(req->DialectCount) * >> >> sizeof(__le16) > >> >> + smb2_buf_len) { >> >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; >> >> + rc = -EINVAL; >> >> + goto err_out; >> >> + } >> >> + } >> >> + >> >> conn->cli_cap = le32_to_cpu(req->Capabilities); >> >> switch (conn->dialect) { >> >> case SMB311_PROT_ID: >> >> diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c >> >> index 1da67217698d..da17b21ac685 100644 >> >> --- a/fs/ksmbd/smb_common.c >> >> +++ b/fs/ksmbd/smb_common.c >> >> @@ -229,13 +229,22 @@ int ksmbd_lookup_dialect_by_id(__le16 >> >> *cli_dialects, >> >> __le16 dialects_count) >> >> >> >> static int ksmbd_negotiate_smb_dialect(void *buf) >> >> { >> >> - __le32 proto; >> >> + int smb_buf_length = get_rfc1002_len(buf); >> > >> > unsigned >> > >> > Thanks! >> > -slow >> > >> > -- >> > Ralph Boehme, Samba Team https://samba.org/ >> > SerNet Samba Team Lead https://sernet.de/en/team-samba >> > >> > >
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index baf7ce31d557..1fe37ad4e5bc 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -1071,7 +1071,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work) struct ksmbd_conn *conn = work->conn; struct smb2_negotiate_req *req = work->request_buf; struct smb2_negotiate_rsp *rsp = work->response_buf; - int rc = 0; + int rc = 0, smb2_buf_len, smb2_neg_size; __le32 status; ksmbd_debug(SMB, "Received negotiate request\n"); @@ -1089,6 +1089,45 @@ int smb2_handle_negotiate(struct ksmbd_work *work) goto err_out; } + smb2_buf_len = get_rfc1002_len(work->request_buf); + smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects) - 4; + if (conn->dialect == SMB311_PROT_ID) { + int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset); + int nego_ctxt_count = le16_to_cpu(req->NegotiateContextCount); + + if (smb2_buf_len < nego_ctxt_off + nego_ctxt_count) { + rsp->hdr.Status = STATUS_INVALID_PARAMETER; + rc = -EINVAL; + goto err_out; + } + + if (smb2_neg_size > nego_ctxt_off) { + rsp->hdr.Status = STATUS_INVALID_PARAMETER; + rc = -EINVAL; + goto err_out; + } + + if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) > + nego_ctxt_off) { + rsp->hdr.Status = STATUS_INVALID_PARAMETER; + rc = -EINVAL; + goto err_out; + } + } else { + if (smb2_neg_size > smb2_buf_len) { + rsp->hdr.Status = STATUS_INVALID_PARAMETER; + rc = -EINVAL; + goto err_out; + } + + if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) > + smb2_buf_len) { + rsp->hdr.Status = STATUS_INVALID_PARAMETER; + rc = -EINVAL; + goto err_out; + } + } + conn->cli_cap = le32_to_cpu(req->Capabilities); switch (conn->dialect) { case SMB311_PROT_ID: diff --git a/fs/ksmbd/smb_common.c b/fs/ksmbd/smb_common.c index 1da67217698d..da17b21ac685 100644 --- a/fs/ksmbd/smb_common.c +++ b/fs/ksmbd/smb_common.c @@ -229,13 +229,22 @@ int ksmbd_lookup_dialect_by_id(__le16 *cli_dialects, __le16 dialects_count) static int ksmbd_negotiate_smb_dialect(void *buf) { - __le32 proto; + int smb_buf_length = get_rfc1002_len(buf); + __le32 proto = ((struct smb2_hdr *)buf)->ProtocolId; - proto = ((struct smb2_hdr *)buf)->ProtocolId; if (proto == SMB2_PROTO_NUMBER) { struct smb2_negotiate_req *req; + int smb2_neg_size = + offsetof(struct smb2_negotiate_req, Dialects) - 4; req = (struct smb2_negotiate_req *)buf; + if (smb2_neg_size > smb_buf_length) + goto err_out; + + if (smb2_neg_size + le16_to_cpu(req->DialectCount) * sizeof(__le16) > + smb_buf_length) + goto err_out; + return ksmbd_lookup_dialect_by_id(req->Dialects, req->DialectCount); } @@ -245,10 +254,19 @@ static int ksmbd_negotiate_smb_dialect(void *buf) struct smb_negotiate_req *req; req = (struct smb_negotiate_req *)buf; + if (le16_to_cpu(req->ByteCount) < 2) + goto err_out; + + if (offsetof(struct smb_negotiate_req, DialectsArray) - 4 + + le16_to_cpu(req->ByteCount) > smb_buf_length) { + goto err_out; + } + return ksmbd_lookup_dialect_by_name(req->DialectsArray, req->ByteCount); } +err_out: return BAD_PROT_ID; }
This patch add validation to check request buffer check in smb2 negotiate. Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> Cc: Ralph Böhme <slow@samba.org> Cc: Steve French <smfrench@gmail.com> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> --- fs/ksmbd/smb2pdu.c | 41 ++++++++++++++++++++++++++++++++++++++++- fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++-- 2 files changed, 60 insertions(+), 3 deletions(-)