[PATCH] Fixes for issues found by covscan or Coverity
Jeremy Allison
jra at samba.org
Tue Feb 12 21:40:13 UTC 2019
On Tue, Feb 12, 2019 at 04:47:14PM +0100, Andreas Schneider via samba-technical wrote:
> Hi,
>
> attached is a patchset which addresses some issues either found by our
> internal covscan or the Coverity scan service (the latter has a CID).
>
>
> Review is much appreciated, push if OK please.
OK, I don't understand patches #1 and #2.
Can you explain in Very.Simple.Words what
the possible overflow access is please ?
I don't see it, sorry :-).
Is there some more details of were the overflow
access inside dup_smb2_vec4() that you haven't
posted ?
Jeremy.
>
>
> Andreas
>
> --
> Andreas Schneider asn at samba.org
> Samba Team www.samba.org
> GPG-ID: 8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D
> From abb7afff2838fcee6e191f0390d146671692bf6b Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Mon, 4 Feb 2019 16:10:50 +0100
> Subject: [PATCH 1/5] s3:smbd: Add additional size check to dup_smb2_req()
>
> Covscan reported:
> 3. samba-4.9.1/source3/smbd/smb2_server.c:1242: assignment: Assigning: "i" = "1".
> 5. samba-4.9.1/source3/smbd/smb2_server.c:1243: overrun-buffer-val:
> Overrunning struct type iovec of 16 bytes by passing it to a
> function which accesses it at byte offset 48.
>
> 1241| /* Setup the vectors identically to the ones in req. */
> 1242| for (i = 1; i < count; i += SMBD_SMB2_NUM_IOV_PER_REQ) {
> 1243|-> if (!dup_smb2_vec4(outvec, &outvec[i], &req->out.vector[i])) {
> 1244| break;
> 1245| }
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> source3/smbd/smb2_server.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
> index 1e9ed331aa6..a4dacfcdae7 100644
> --- a/source3/smbd/smb2_server.c
> +++ b/source3/smbd/smb2_server.c
> @@ -1240,7 +1240,9 @@ static struct smbd_smb2_request *dup_smb2_req(const struct smbd_smb2_request *re
> memcpy(newreq->out.nbt_hdr, req->out.nbt_hdr, 4);
>
> /* Setup the vectors identically to the ones in req. */
> - for (i = 1; i < count; i += SMBD_SMB2_NUM_IOV_PER_REQ) {
> + for (i = 1;
> + i < count && i + SMBD_SMB2_NUM_IOV_PER_REQ <= count;
> + i += SMBD_SMB2_NUM_IOV_PER_REQ) {
> if (!dup_smb2_vec4(outvec, &outvec[i], &req->out.vector[i])) {
> break;
> }
> --
> 2.20.1
>
>
> From e7f891fa97a43f09f6b707253f2aed8db4d12973 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Mon, 4 Feb 2019 16:17:30 +0100
> Subject: [PATCH 2/5] s3:locking: Add additional check for sizes
>
> 42. samba-4.9.1/source3/locking/brlock.c:875: overrun-buffer-val:
> Overrunning struct type lock_struct of 72 bytes by passing it to a
> function which accesses it at byte offset 72.
> 873|
> 874| /* Work out overlaps. */
> 875|-> tmp_count += brlock_posix_split_merge(&tp[count], curr_lock, plock);
> 876| posix_count += tmp_count;
> 877| count += tmp_count;
>
> 36. samba-4.9.1/source3/locking/brlock.c:1245: overrun-buffer-val:
> Overrunning struct type lock_struct of 72 bytes by passing it to a
> function which accesses it at byte offset 72.
> 1243|
> 1244| /* Work out overlaps. */
> 1245|-> tmp_count = brlock_posix_split_merge(&tp[count],
> lock, plock);
> 1246|
> 1247| if (tmp_count == 0) {
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> source3/locking/brlock.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/source3/locking/brlock.c b/source3/locking/brlock.c
> index 0c91d55e813..2e06aeac2be 100644
> --- a/source3/locking/brlock.c
> +++ b/source3/locking/brlock.c
> @@ -826,7 +826,9 @@ static NTSTATUS brl_lock_posix(struct messaging_context *msg_ctx,
>
> count = posix_count = 0;
>
> - for (i=0; i < br_lck->num_locks; i++) {
> + for (i = 0;
> + i < br_lck->num_locks && count < br_lck->num_locks;
> + i++) {
> struct lock_struct *curr_lock = &locks[i];
>
> /* If we have a pending read lock, a lock downgrade should
> @@ -1217,7 +1219,9 @@ static bool brl_unlock_posix(struct messaging_context *msg_ctx,
> }
>
> count = 0;
> - for (i = 0; i < br_lck->num_locks; i++) {
> + for (i = 0;
> + i < br_lck->num_locks && count < br_lck->num_locks;
> + i++) {
> struct lock_struct *lock = &locks[i];
> unsigned int tmp_count;
>
> --
> 2.20.1
>
>
> From dc9e90c3ded3a8f7ca34da6eb03510a9be63d658 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Mon, 4 Feb 2019 17:19:55 +0100
> Subject: [PATCH 3/5] s3:locking: Add missing NULL check
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> source3/locking/locking.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/source3/locking/locking.c b/source3/locking/locking.c
> index ae5f0bbcf33..d4c3b32be7f 100644
> --- a/source3/locking/locking.c
> +++ b/source3/locking/locking.c
> @@ -150,6 +150,9 @@ bool strict_lock_check_default(files_struct *fsp, struct lock_struct *plock)
> * autocleanup. This is the slow path anyway.
> */
> br_lck = brl_get_locks(talloc_tos(), fsp);
> + if (br_lck == NULL) {
> + return true;
> + }
> ret = brl_locktest(br_lck, plock);
> TALLOC_FREE(br_lck);
> }
> --
> 2.20.1
>
>
> From 85784495122335097371ec0098d1679322954f99 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Mon, 4 Feb 2019 17:23:05 +0100
> Subject: [PATCH 4/5] s3:utils: Add missing NULL check in
> rpc_fetch_domain_aliases()
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> source3/utils/net_rpc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/source3/utils/net_rpc.c b/source3/utils/net_rpc.c
> index f2d63d2af65..a56190f7be5 100644
> --- a/source3/utils/net_rpc.c
> +++ b/source3/utils/net_rpc.c
> @@ -4669,6 +4669,10 @@ static NTSTATUS rpc_fetch_domain_aliases(struct rpc_pipe_client *pipe_hnd,
>
> if (alias.num_members > 0) {
> alias.members = SMB_MALLOC_ARRAY(struct dom_sid, alias.num_members);
> + if (alias.members == NULL) {
> + status = NT_STATUS_NO_MEMORY;
> + goto done;
> + }
>
> for (j = 0; j < alias.num_members; j++)
> sid_copy(&alias.members[j],
> --
> 2.20.1
>
>
> From bc317f9985ade3c45fad253f9dc3f21ad3cd9937 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Mon, 4 Feb 2019 16:44:02 +0100
> Subject: [PATCH 5/5] lib:replace: Avoid out of bound reads warnings
>
> 1. Condition *((char const *)string) == '.', taking true branch.
> 2. Condition (char const *)string[1] == '.', taking true branch.
> 3. Condition (char const *)string[2] == 0, taking true branch.
> 1655 if (ISDOTDOT(string))
> 4. alias: Assigning: string = ".". string now points to byte 0 of "." (which consists of 2 bytes).
> 1656 string = ".";
>
> 1. Condition *((char const *)string) == '.', taking true branch.
> 2. Condition (char const *)string[1] == '.', taking true branch.
> 3. index_const: Pointer string directly indexed by constant 2.
> 160 if (ISDOTDOT(string)) {
> 161 string = ".";
> 162 }
>
> I guess it wants a check for '\0' at string[1] and leave.
>
> CID 710714
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> lib/replace/system/dir.h | 38 +++++++++++++++++++++++++++-----------
> 1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/lib/replace/system/dir.h b/lib/replace/system/dir.h
> index 497b5a7c0fa..e1ad8382854 100644
> --- a/lib/replace/system/dir.h
> +++ b/lib/replace/system/dir.h
> @@ -50,22 +50,38 @@
> # include <libgen.h>
> #endif
>
> -/* Test whether a file name is the "." or ".." directory entries.
> - * These really should be inline functions.
> +/*
> + * Test whether a file name is the "." or ".." directory entries.
> */
> +static inline bool is_dot(const char *path)
> +{
> + if (unlikely(path == NULL)) {
> + return false;
> + }
> +
> + if (unlikely(path[0] == '\0')) {
> + return false;
> + }
> +
> + return path[0] == '.' && path[1] == '\0';
> +}
> +
> +static inline bool is_dotdot(const char *path)
> +{
> + /* The is_dot() function will to NULL checks */
> + if (is_dot(path)) {
> + return false;
> + }
> +
> + return path[1] == '.' && path[2] == '\0';
> +}
> +
> #ifndef ISDOT
> -#define ISDOT(path) ( \
> - *((const char *)(path)) == '.' && \
> - *(((const char *)(path)) + 1) == '\0' \
> - )
> +#define ISDOT(path) is_dot(path)
> #endif
>
> #ifndef ISDOTDOT
> -#define ISDOTDOT(path) ( \
> - *((const char *)(path)) == '.' && \
> - *(((const char *)(path)) + 1) == '.' && \
> - *(((const char *)(path)) + 2) == '\0' \
> - )
> +#define ISDOTDOT(path) is_dotdot(path)
> #endif
>
> #endif
> --
> 2.20.1
>
More information about the samba-technical
mailing list