| Message ID | 20211001120421.327245-17-slow@samba.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Buffer validation patches | expand |
2021-10-01 21:04 GMT+09:00, Ralph Boehme <slow@samba.org>: > Note: we already have the same check in is_chained_smb2_message(), but there > it > only applies to compound requests, so we have to repeat the check here to > cover > both cases. > > Cc: Namjae Jeon <linkinjeon@kernel.org> > Cc: Tom Talpey <tom@talpey.com> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > Cc: Steve French <smfrench@gmail.com> > Cc: Hyunchul Lee <hyc.lee@gmail.com> > Signed-off-by: Ralph Boehme <slow@samba.org> > --- > fs/ksmbd/smb2misc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c > index 7ed266eb6c5e..541b39b7a84b 100644 > --- a/fs/ksmbd/smb2misc.c > +++ b/fs/ksmbd/smb2misc.c > @@ -338,6 +338,9 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work) > if (check_smb2_hdr(hdr)) > return 1; > > + if (len < sizeof(struct smb2_pdu) - 4) > + return 1; when only this patch is applied, how can you guarantee that session id and tree id of smb2 header are vaild ? > + > if (hdr->StructureSize != SMB2_HEADER_STRUCTURE_SIZE) { > ksmbd_debug(SMB, "Illegal structure size %u\n", > le16_to_cpu(hdr->StructureSize)); > -- > 2.31.1 > >
Am 02.10.21 um 07:55 schrieb Namjae Jeon: > 2021-10-01 21:04 GMT+09:00, Ralph Boehme <slow@samba.org>: >> Note: we already have the same check in is_chained_smb2_message(), but there >> it >> only applies to compound requests, so we have to repeat the check here to >> cover >> both cases. >> >> Cc: Namjae Jeon <linkinjeon@kernel.org> >> Cc: Tom Talpey <tom@talpey.com> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> Cc: Steve French <smfrench@gmail.com> >> Cc: Hyunchul Lee <hyc.lee@gmail.com> >> Signed-off-by: Ralph Boehme <slow@samba.org> >> --- >> fs/ksmbd/smb2misc.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c >> index 7ed266eb6c5e..541b39b7a84b 100644 >> --- a/fs/ksmbd/smb2misc.c >> +++ b/fs/ksmbd/smb2misc.c >> @@ -338,6 +338,9 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work) >> if (check_smb2_hdr(hdr)) >> return 1; >> >> + if (len < sizeof(struct smb2_pdu) - 4) >> + return 1; > when only this patch is applied, how can you guarantee that session id > and tree id of smb2 header are vaild ? what do you mean? This just checks the actual packet lenght is large enough to access the header and the body lenght field. -slow
Hi Ralph, 2021년 10월 1일 (금) 오후 9:25, Ralph Boehme <slow@samba.org>님이 작성: > > Note: we already have the same check in is_chained_smb2_message(), but there it > only applies to compound requests, so we have to repeat the check here to cover > both cases. > > Cc: Namjae Jeon <linkinjeon@kernel.org> > Cc: Tom Talpey <tom@talpey.com> > Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> > Cc: Steve French <smfrench@gmail.com> > Cc: Hyunchul Lee <hyc.lee@gmail.com> > Signed-off-by: Ralph Boehme <slow@samba.org> > --- > fs/ksmbd/smb2misc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c > index 7ed266eb6c5e..541b39b7a84b 100644 > --- a/fs/ksmbd/smb2misc.c > +++ b/fs/ksmbd/smb2misc.c > @@ -338,6 +338,9 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work) > if (check_smb2_hdr(hdr)) > return 1; > > + if (len < sizeof(struct smb2_pdu) - 4) > + return 1; > + Do we need this check before accessing any fields of smb2_hdr in ksmbd_verify_smb_message()? > if (hdr->StructureSize != SMB2_HEADER_STRUCTURE_SIZE) { > ksmbd_debug(SMB, "Illegal structure size %u\n", > le16_to_cpu(hdr->StructureSize)); > -- > 2.31.1 >
Am 02.10.21 um 14:45 schrieb Hyunchul Lee: > Hi Ralph, > > 2021년 10월 1일 (금) 오후 9:25, Ralph Boehme <slow@samba.org>님이 작성: >> >> Note: we already have the same check in is_chained_smb2_message(), but there it >> only applies to compound requests, so we have to repeat the check here to cover >> both cases. >> >> Cc: Namjae Jeon <linkinjeon@kernel.org> >> Cc: Tom Talpey <tom@talpey.com> >> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >> Cc: Steve French <smfrench@gmail.com> >> Cc: Hyunchul Lee <hyc.lee@gmail.com> >> Signed-off-by: Ralph Boehme <slow@samba.org> >> --- >> fs/ksmbd/smb2misc.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c >> index 7ed266eb6c5e..541b39b7a84b 100644 >> --- a/fs/ksmbd/smb2misc.c >> +++ b/fs/ksmbd/smb2misc.c >> @@ -338,6 +338,9 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work) >> if (check_smb2_hdr(hdr)) >> return 1; >> >> + if (len < sizeof(struct smb2_pdu) - 4) >> + return 1; >> + > > Do we need this check before accessing any fields of smb2_hdr in > ksmbd_verify_smb_message()? well, my idea was to have the core PDU size checking logic in ksmbd_smb2_check_message() and ksmbd_verify_smb_message() merely switches between SMB1/SMB2. -slow
2021-10-02 21:49 GMT+09:00, Ralph Boehme <slow@samba.org>: > Am 02.10.21 um 14:45 schrieb Hyunchul Lee: >> Hi Ralph, >> >> 2021년 10월 1일 (금) 오후 9:25, Ralph Boehme <slow@samba.org>님이 작성: >>> >>> Note: we already have the same check in is_chained_smb2_message(), but >>> there it >>> only applies to compound requests, so we have to repeat the check here to >>> cover >>> both cases. >>> >>> Cc: Namjae Jeon <linkinjeon@kernel.org> >>> Cc: Tom Talpey <tom@talpey.com> >>> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> >>> Cc: Steve French <smfrench@gmail.com> >>> Cc: Hyunchul Lee <hyc.lee@gmail.com> >>> Signed-off-by: Ralph Boehme <slow@samba.org> >>> --- >>> fs/ksmbd/smb2misc.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c >>> index 7ed266eb6c5e..541b39b7a84b 100644 >>> --- a/fs/ksmbd/smb2misc.c >>> +++ b/fs/ksmbd/smb2misc.c >>> @@ -338,6 +338,9 @@ int ksmbd_smb2_check_message(struct ksmbd_work >>> *work) >>> if (check_smb2_hdr(hdr)) >>> return 1; >>> >>> + if (len < sizeof(struct smb2_pdu) - 4) >>> + return 1; >>> + >> >> Do we need this check before accessing any fields of smb2_hdr in >> ksmbd_verify_smb_message()? > > well, my idea was to have the core PDU size checking logic in > ksmbd_smb2_check_message() and ksmbd_verify_smb_message() merely > switches between SMB1/SMB2. Hyunchul pointed out that this header buffer check should be moved to the above check_smb2_hdr(). I think that it should be move to the above ksmbd_smb2_cur_pdu_buflen(). > > -slow > > -- > Ralph Boehme, Samba Team https://samba.org/ > SerNet Samba Team Lead https://sernet.de/en/team-samba >
diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c index 7ed266eb6c5e..541b39b7a84b 100644 --- a/fs/ksmbd/smb2misc.c +++ b/fs/ksmbd/smb2misc.c @@ -338,6 +338,9 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work) if (check_smb2_hdr(hdr)) return 1; + if (len < sizeof(struct smb2_pdu) - 4) + return 1; + if (hdr->StructureSize != SMB2_HEADER_STRUCTURE_SIZE) { ksmbd_debug(SMB, "Illegal structure size %u\n", le16_to_cpu(hdr->StructureSize));
Note: we already have the same check in is_chained_smb2_message(), but there it only applies to compound requests, so we have to repeat the check here to cover both cases. Cc: Namjae Jeon <linkinjeon@kernel.org> Cc: Tom Talpey <tom@talpey.com> Cc: Ronnie Sahlberg <ronniesahlberg@gmail.com> Cc: Steve French <smfrench@gmail.com> Cc: Hyunchul Lee <hyc.lee@gmail.com> Signed-off-by: Ralph Boehme <slow@samba.org> --- fs/ksmbd/smb2misc.c | 3 +++ 1 file changed, 3 insertions(+)