Re: mg: Fix Coverity Scan warning: Insecure data handling

[Mark Lumsden]

Thanks Todd. I'll pass your analysis on to Joachim.


Date: Tue, 09 Mar 2021 14:14:33 -0700
From: Todd C. Miller 
To: Mark Lumsden 
Cc: tech@openbsd.org
Subject: Re: mg: Fix Coverity Scan warning: Insecure data handling 


On Tue, 09 Mar 2021 20:14:19 +, Mark Lumsden wrote:


Here is a diff from Joachim Wiberg's version of mg.

"The strlcpy() function is guaranteed to never copy more than 'len - 1'
bytes, so there is no need to check if we copied more.  This is a bogus
warning since the introduction of strlcpy()."


That looks wrong to me.  strlcpy() returns the number of bytes it
would have copied if there was space.  But if there was insufficient
space then the return value can be larger.  It is not safe to blindly
use the return value without checking it first.

- todd

mg: Fix Coverity Scan warning: Insecure data handling

[Mark Lumsden]

Here is a diff from Joachim Wiberg's version of mg.

"The strlcpy() function is guaranteed to never copy more than 'len - 1'
bytes, so there is no need to check if we copied more.  This is a bogus
warning since the introduction of strlcpy()."

Tested and seems reasonable. ok?

Index: cinfo.c
===
RCS file: /cvs/src/usr.bin/mg/cinfo.c,v
retrieving revision 1.18
diff -u -p -r1.18 cinfo.c
--- cinfo.c 19 Mar 2015 21:22:15 -  1.18
+++ cinfo.c 9 Mar 2021 20:09:54 -
@@ -106,7 +106,6 @@ char *
 getkeyname(char *cp, size_t len, int k)
 {
const char  *np;
-   size_t   copied;

if (k < 0)
k = CHARMASK(k);/* sign extended char */
@@ -151,8 +150,6 @@ getkeyname(char *cp, size_t len, int k)
*cp = '\0';
return (cp);
}
-   copied = strlcpy(cp, np, len);
-   if (copied >= len)
-   copied = len - 1;
-   return (cp + copied);
+
+   return (cp + strlcpy(cp, np, len));
 }

Re: mg: Fix Coverity Scan warning: Insecure data handling

[Theo de Raadt]

Todd C. Miller  wrote:

> On Tue, 09 Mar 2021 20:14:19 +, Mark Lumsden wrote:
> 
> > Here is a diff from Joachim Wiberg's version of mg.
> >
> > "The strlcpy() function is guaranteed to never copy more than 'len - 1'
> > bytes, so there is no need to check if we copied more.  This is a bogus
> > warning since the introduction of strlcpy()."
> 
> That looks wrong to me.  strlcpy() returns the number of bytes it
> would have copied if there was space.  But if there was insufficient
> space then the return value can be larger.  It is not safe to blindly
> use the return value without checking it first.

Yes, it is a pretty severe misunderstanding.

Re: mg: Fix Coverity Scan warning: Insecure data handling

[Todd C . Miller]

On Tue, 09 Mar 2021 20:14:19 +, Mark Lumsden wrote:

> Here is a diff from Joachim Wiberg's version of mg.
>
> "The strlcpy() function is guaranteed to never copy more than 'len - 1'
> bytes, so there is no need to check if we copied more.  This is a bogus
> warning since the introduction of strlcpy()."

That looks wrong to me.  strlcpy() returns the number of bytes it
would have copied if there was space.  But if there was insufficient
space then the return value can be larger.  It is not safe to blindly
use the return value without checking it first.

 - todd