[PATCH] conf: Fix one Coverity CID 258163 warning, work around another one

[PATCH] conf: Fix one Coverity CID 258163 warning, work around another one

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 1748 bytes --]

In conf_runas(), Coverity reports that we might dereference uid and
gid despite possibly being NULL (CWE-476) because of the check after
the first sscanf(). They can't be NULL, but I actually wanted to
check that UID and GID are non-zero (the user could otherwise pass
--runas root:root and defy the whole mechanism).

Later on, we have the same type of warning for 'gr': it's compared
against NULL, so it might be NULL, which is actually the case: but
in that case, we don't dereference it, because we'll return -ENOENT
right away. Rewrite the clause to silence the warning.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 conf.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/conf.c b/conf.c
index ddad9a3..d615cf5 100644
--- a/conf.c
+++ b/conf.c
@@ -857,7 +857,7 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid)
 	struct group *gr;
 
 	/* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */
-	if (sscanf(opt, "%u:%u", uid, gid) == 2 && uid && gid)
+	if (sscanf(opt, "%u:%u", uid, gid) == 2 && *uid && *gid)
 		return 0;
 
 	*uid = strtol(opt, &endptr, 0);
@@ -874,12 +874,10 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid)
 	/* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */
 	if (sscanf(opt, "%" STR(LOGIN_NAME_MAX) "[^:]:"
 			"%" STR(LOGIN_NAME_MAX) "s", ubuf, gbuf) == 2) {
-		pw = getpwnam(ubuf);
-		if (!pw || !(*uid = pw->pw_uid))
+		if (!(pw = getpwnam(ubuf)) || !(*uid = pw->pw_uid))
 			return -ENOENT;
 
-		gr = getgrnam(gbuf);
-		if (!gr || !(*gid = gr->gr_gid))
+		if (!(gr = getgrnam(gbuf)) || !(*gid = gr->gr_gid))
 			return -ENOENT;
 
 		return 0;
-- 
@@ -857,7 +857,7 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid)
 	struct group *gr;
 
 	/* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */
-	if (sscanf(opt, "%u:%u", uid, gid) == 2 && uid && gid)
+	if (sscanf(opt, "%u:%u", uid, gid) == 2 && *uid && *gid)
 		return 0;
 
 	*uid = strtol(opt, &endptr, 0);
@@ -874,12 +874,10 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid)
 	/* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */
 	if (sscanf(opt, "%" STR(LOGIN_NAME_MAX) "[^:]:"
 			"%" STR(LOGIN_NAME_MAX) "s", ubuf, gbuf) == 2) {
-		pw = getpwnam(ubuf);
-		if (!pw || !(*uid = pw->pw_uid))
+		if (!(pw = getpwnam(ubuf)) || !(*uid = pw->pw_uid))
 			return -ENOENT;
 
-		gr = getgrnam(gbuf);
-		if (!gr || !(*gid = gr->gr_gid))
+		if (!(gr = getgrnam(gbuf)) || !(*gid = gr->gr_gid))
 			return -ENOENT;
 
 		return 0;
-- 
2.35.1

Re: [PATCH] conf: Fix one Coverity CID 258163 warning, work around another one

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2145 bytes --]

On Fri, May 20, 2022 at 11:01:22AM +0200, Stefano Brivio wrote:
> In conf_runas(), Coverity reports that we might dereference uid and
> gid despite possibly being NULL (CWE-476) because of the check after
> the first sscanf(). They can't be NULL, but I actually wanted to
> check that UID and GID are non-zero (the user could otherwise pass
> --runas root:root and defy the whole mechanism).
> 
> Later on, we have the same type of warning for 'gr': it's compared
> against NULL, so it might be NULL, which is actually the case: but
> in that case, we don't dereference it, because we'll return -ENOENT
> right away. Rewrite the clause to silence the warning.
> 
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>

Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>

> ---
>  conf.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index ddad9a3..d615cf5 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -857,7 +857,7 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid)
>  	struct group *gr;
>  
>  	/* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */
> -	if (sscanf(opt, "%u:%u", uid, gid) == 2 && uid && gid)
> +	if (sscanf(opt, "%u:%u", uid, gid) == 2 && *uid && *gid)
>  		return 0;
>  
>  	*uid = strtol(opt, &endptr, 0);
> @@ -874,12 +874,10 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid)
>  	/* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */
>  	if (sscanf(opt, "%" STR(LOGIN_NAME_MAX) "[^:]:"
>  			"%" STR(LOGIN_NAME_MAX) "s", ubuf, gbuf) == 2) {
> -		pw = getpwnam(ubuf);
> -		if (!pw || !(*uid = pw->pw_uid))
> +		if (!(pw = getpwnam(ubuf)) || !(*uid = pw->pw_uid))
>  			return -ENOENT;
>  
> -		gr = getgrnam(gbuf);
> -		if (!gr || !(*gid = gr->gr_gid))
> +		if (!(gr = getgrnam(gbuf)) || !(*gid = gr->gr_gid))
>  			return -ENOENT;
>  
>  		return 0;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]