Current-Users archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

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


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.


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

Home | Main Index | Thread Index | Old Index