| Message ID | 20210923034855.612832-3-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] ksmbd: add validation in smb2 negotiate | expand |
On 9/22/2021 11:48 PM, Namjae Jeon wrote: > This patch add validation to check request buffer check in smb2 > negotiate. > > Cc: Tom Talpey <tom@talpey.com> > 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> > --- > v3: > - fix integer(nego_ctxt_off) overflow issue. > - change data type of variables to unsigned. > > fs/ksmbd/smb2pdu.c | 40 ++++++++++++++++++++++++++++++++++++++++ > fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++-- > 2 files changed, 60 insertions(+), 2 deletions(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index 301558a04298..2f9719a3f089 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -1080,6 +1080,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work) > struct smb2_negotiate_req *req = work->request_buf; > struct smb2_negotiate_rsp *rsp = work->response_buf; > int rc = 0; > + unsigned int smb2_buf_len, smb2_neg_size; > __le32 status; > > ksmbd_debug(SMB, "Received negotiate request\n"); > @@ -1097,6 +1098,45 @@ int smb2_handle_negotiate(struct ksmbd_work *work) > goto err_out; > } > > + smb2_buf_len = get_rfc1002_len(work->request_buf); Where is it validated that the pdu actually contains the number of bytes in the DirectTCP header? Honestly I don't understand why the 4 bytes are passed around at all. Normally I would expect these to be stripped off after validation by the lower-layer receive processing. This would simplify the gazillion "- 4" corrections all over the code, too. > + smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects) - 4; > + if (conn->dialect == SMB311_PROT_ID) { > + unsigned int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset); > + unsigned int nego_ctxt_count = le16_to_cpu(req->NegotiateContextCount); > + > + if (smb2_buf_len < (u64)nego_ctxt_off + nego_ctxt_count) { This seems to be wrong. nego_ctxt_off is the base offset, but the nego_ctxt_count is the number, not the length of the contexts! > + rsp->hdr.Status = STATUS_INVALID_PARAMETER; > + rc = -EINVAL; > + goto err_out; > + } > + > + if (smb2_neg_size > nego_ctxt_off) { Isn't this completely redundant with the next test? > + 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) { This validates that all the dialects are present, but it does not account for the padding and negotiate contexts fields which follow them. > + 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) { Same connects as the 3.1.1 validation above. > + 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 ebc835ab414c..02fe2a06dda9 100644 > --- a/fs/ksmbd/smb_common.c > +++ b/fs/ksmbd/smb_common.c > @@ -235,13 +235,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); Same comments as above on length field and passed buffer size. > + __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; What is this test protecting? neg_size is the length of the pdu *before* the Dialects field. > + > + 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); > } > @@ -251,10 +260,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; So, this is SMB1-style negotiation, and it's very unclear if the ByteCount is being checked for overflow. The code in mainline is very different, and it's not clear what this patch applies to. > + > + 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; > } > >
2021-09-24 0:54 GMT+09:00, Tom Talpey <tom@talpey.com>: > On 9/22/2021 11:48 PM, Namjae Jeon wrote: >> This patch add validation to check request buffer check in smb2 >> negotiate. >> >> Cc: Tom Talpey <tom@talpey.com> >> 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> >> --- >> v3: >> - fix integer(nego_ctxt_off) overflow issue. >> - change data type of variables to unsigned. >> >> fs/ksmbd/smb2pdu.c | 40 ++++++++++++++++++++++++++++++++++++++++ >> fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++-- >> 2 files changed, 60 insertions(+), 2 deletions(-) >> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> index 301558a04298..2f9719a3f089 100644 >> --- a/fs/ksmbd/smb2pdu.c >> +++ b/fs/ksmbd/smb2pdu.c >> @@ -1080,6 +1080,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work) >> struct smb2_negotiate_req *req = work->request_buf; >> struct smb2_negotiate_rsp *rsp = work->response_buf; >> int rc = 0; >> + unsigned int smb2_buf_len, smb2_neg_size; >> __le32 status; >> >> ksmbd_debug(SMB, "Received negotiate request\n"); >> @@ -1097,6 +1098,45 @@ int smb2_handle_negotiate(struct ksmbd_work *work) >> goto err_out; >> } >> >> + smb2_buf_len = get_rfc1002_len(work->request_buf); > > Where is it validated that the pdu actually contains the number > of bytes in the DirectTCP header? ksmbd_smb2_check_message() validate it. > > Honestly I don't understand why the 4 bytes are passed around at all. > Normally I would expect these to be stripped off after validation by > the lower-layer receive processing. This would simplify the gazillion > "- 4" corrections all over the code, too. Okay, I have a patch for this. There was a small amount of code modification, so I thought to apply it after the buffer check issues are fixed. > >> + smb2_neg_size = offsetof(struct smb2_negotiate_req, Dialects) - 4; >> + if (conn->dialect == SMB311_PROT_ID) { >> + unsigned int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset); >> + unsigned int nego_ctxt_count = >> le16_to_cpu(req->NegotiateContextCount); >> + >> + if (smb2_buf_len < (u64)nego_ctxt_off + nego_ctxt_count) { > > This seems to be wrong. nego_ctxt_off is the base offset, but the > nego_ctxt_count is the number, not the length of the contexts! Ah, Right. This can be validated in deassemble_neg_contexts(). > >> + rsp->hdr.Status = STATUS_INVALID_PARAMETER; >> + rc = -EINVAL; >> + goto err_out; >> + } >> + >> + if (smb2_neg_size > nego_ctxt_off) { > > Isn't this completely redundant with the next test? How do we know if the DialectCount of the next check is valid? > >> + 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) { > > This validates that all the dialects are present, but it does not > account for the padding and negotiate contexts fields which follow > them. negotiate contexts will be validated in deassemble_neg_contexts(). > >> + 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) { > > Same connects as the 3.1.1 validation above. < 3.1.1 doesn't have negotiate contexts. So It should be checked with smb2_buf_len(rfc1002 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 ebc835ab414c..02fe2a06dda9 100644 >> --- a/fs/ksmbd/smb_common.c >> +++ b/fs/ksmbd/smb_common.c >> @@ -235,13 +235,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); > > Same comments as above on length field and passed buffer size. This will be improved on another patch. > >> + __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; > > What is this test protecting? neg_size is the length of the pdu *before* > the Dialects field. We need to validate DialectCount is valid first ? > >> + >> + 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); >> } >> @@ -251,10 +260,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; > > So, this is SMB1-style negotiation, and it's very unclear if the > ByteCount is being checked for overflow. The code in mainline is > very different, and it's not clear what this patch applies to. ByteCount should be checked in ksmbd_lookup_dialect_by_name(). Could you please give a idea how to validate ByteCount ? Thanks for your review! > >> + >> + 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; >> } >> >> >
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index 301558a04298..2f9719a3f089 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -1080,6 +1080,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work) struct smb2_negotiate_req *req = work->request_buf; struct smb2_negotiate_rsp *rsp = work->response_buf; int rc = 0; + unsigned int smb2_buf_len, smb2_neg_size; __le32 status; ksmbd_debug(SMB, "Received negotiate request\n"); @@ -1097,6 +1098,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) { + unsigned int nego_ctxt_off = le32_to_cpu(req->NegotiateContextOffset); + unsigned int nego_ctxt_count = le16_to_cpu(req->NegotiateContextCount); + + if (smb2_buf_len < (u64)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 ebc835ab414c..02fe2a06dda9 100644 --- a/fs/ksmbd/smb_common.c +++ b/fs/ksmbd/smb_common.c @@ -235,13 +235,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); } @@ -251,10 +260,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: Tom Talpey <tom@talpey.com> 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> --- v3: - fix integer(nego_ctxt_off) overflow issue. - change data type of variables to unsigned. fs/ksmbd/smb2pdu.c | 40 ++++++++++++++++++++++++++++++++++++++++ fs/ksmbd/smb_common.c | 22 ++++++++++++++++++++-- 2 files changed, 60 insertions(+), 2 deletions(-)