Possible unsafe use of strncat in sbin/sysctl/sysctl.c

[Jimmy Johansson <jimmy%Update.UU.SE@localhost>]

Hi,


I propose the following patch, as I think this is what the developer
intended anyway:

Index: sbin/sysctl/sysctl.c
===================================================================
RCS file: /cvsroot/src/sbin/sysctl/sysctl.c,v
retrieving revision 1.136
diff -u -w -p -r1.136 sysctl.c
--- sbin/sysctl/sysctl.c        3 Aug 2011 01:58:30 -0000       1.136
+++ sbin/sysctl/sysctl.c        23 Aug 2011 18:57:52 -0000
@@ -579,8 +579,8 @@ print_tree(int *name, u_int namelen, str
                for (ni = 0; ni < namelen; ni++) {
                        (void)snprintf(n, sizeof(n), "%d", name[ni]);
                        if (ni > 0)
-                               strncat(gdname, ".", sizeof(gdname));
-                       strncat(gdname, n, sizeof(gdname));
+                               (void)strlcat(gdname, ".", sizeof(gdname));
+                       (void)strlcat(gdname, n, sizeof(gdname));
                }
        }
 
@@ -589,11 +589,11 @@ print_tree(int *name, u_int namelen, str
        else if (add) {
                snprintf(n, sizeof(n), "%d", pnode->sysctl_num);
                if (namelen > 1) {
-                       strncat(gsname, sep, sizeof(gsname));
-                       strncat(gdname, ".", sizeof(gdname));
+                       (void)strlcat(gsname, sep, sizeof(gsname));
+                       (void)strlcat(gdname, ".", sizeof(gdname));
                }
-               strncat(gsname, pnode->sysctl_name, sizeof(gsname));
-               strncat(gdname, n, sizeof(gdname));
+               (void)strlcat(gsname, pnode->sysctl_name, sizeof(gsname));
+               (void)strlcat(gdname, n, sizeof(gdname));
        }
 
        if (Mflag && pnode != &my_root) {

I haven't looked at the code in detail so there might be code that
checks that the second argument will always fit. I still believe that
the way strncat is used here is unsafe and if it was used in a safe
manner then I wouldn't have had to read the code in detail to know if it
was safe.

If I read strncat(3) correctly safe use is either

  strncat(foo, bar, sizeof(foo) - strlen(foo) - 1);

or to use

  strlcat(foo, bar, sizeof(foo));

but not

  strncat(foo, bar, sizeof(foo));

as this will be wrong even if foo is empty because it doesn't allow for
the terminating '\0'.

I think the developer intended to use strlcat, as the rest of the code
uses strlcat and not strncat.

Regards,

Jimmy
-- 
If you don't shoot the bearers of bad news, people will keep bringing it to you.