Re: [PATCH] meta:recipes-extended: stat fix security gaps

["Randle, William C" <william.c.randle@intel.com>]

On Mon, 2016-05-16 at 16:37 -0500, Plauchu Edwin wrote:
> 
> On 16/05/16 16:28, Khem Raj wrote:
> > 
> > > 
> > > On May 16, 2016, at 1:19 PM, edwin.plauchu.camacho@linux.intel.com wrote:
> > > 
> > > From: Edwin Plauchu <edwin.plauchu.camacho@intel.com>
> > > 
> > > This patch avoids stat fails to compile with compiler flags which elevate
> > > common string formatting issues into an error (-Wformat -Wformat-security
> > > -Werror=format-security).
> > > 
> > > [YOCTO #9550]
> > > 
> > > Signed-off-by: Edwin Plauchu <edwin.plauchu.camacho@intel.com>
> > > ---
> > > meta/conf/distro/include/security_flags.inc        |  1 -
> > > .../stat/stat-3.3/fix-security-format.patch        | 77
> > > ++++++++++++++++++++++
> > > meta/recipes-extended/stat/stat_3.3.bb             |  1 +
> > > 3 files changed, 78 insertions(+), 1 deletion(-)
> > > create mode 100644 meta/recipes-extended/stat/stat-3.3/fix-security-
> > > format.patch
> > > 
> > > diff --git a/meta/conf/distro/include/security_flags.inc
> > > b/meta/conf/distro/include/security_flags.inc
> > > index 7a91cec..5ae6dd8 100644
> > > --- a/meta/conf/distro/include/security_flags.inc
> > > +++ b/meta/conf/distro/include/security_flags.inc
> > > @@ -105,7 +105,6 @@ SECURITY_STRINGFORMAT_pn-gettext = ""
> > > SECURITY_STRINGFORMAT_pn-kexec-tools = ""
> > > SECURITY_STRINGFORMAT_pn-makedevs = ""
> > > SECURITY_STRINGFORMAT_pn-oh-puzzles = ""
> > > -SECURITY_STRINGFORMAT_pn-stat = ""
> > > SECURITY_STRINGFORMAT_pn-unzip = ""
> > > SECURITY_STRINGFORMAT_pn-zip = ""
> > > 
> > > diff --git a/meta/recipes-extended/stat/stat-3.3/fix-security-format.patch 
> > > b/meta/recipes-extended/stat/stat-3.3/fix-security-format.patch
> > > new file mode 100644
> > > index 0000000..7d9f8df
> > > --- /dev/null
> > > +++ b/meta/recipes-extended/stat/stat-3.3/fix-security-format.patch
> > > @@ -0,0 +1,77 @@
> > > +meta: recipes-extended: Fixing security formatting issues on stat
> > > +
> > > +Fix security formatting issues related to printf without NULL argument
> > > +
> > > +stat.c: In function 'print_human_access':
> > > +stat.c:292:13: error: format not a string literal and no format arguments
> > > [-Werror=format-security]
> > > +     printf (access);
> > > +             ^
> > > +stat.c: In function 'print_human_time':
> > > +stat.c:299:57: error: format not a string literal and no format arguments
> > > [-Werror=format-security]
> > > +   if (strftime(str, 40, "%c", localtime(t)) > 0) printf(str);
> > > +                                                         ^
> > > +stat.c: In function 'print_it':
> > > +stat.c:613:6: error: format not a string literal and no format arguments
> > > [-Werror=format-security]
> > > +      printf(b);
> > > +      ^
> > > +stat.c:642:6: error: format not a string literal and no format arguments
> > > [-Werror=format-security]
> > > +      printf(b);
> > > +      ^
> > > +
> > > +[YOCTO #9550]
> > > +[https://bugzilla.yoctoproject.org/show_bug.cgi?id=9550]
> > > +
> > > +Upstream-Status: Pending
> > > +
> > > +Signed-off-by: Edwin Plauchu <edwin.plauchu.camacho@intel.com>
> > > +
> > > +diff --git a/stat.c b/stat.c
> > > +index 1ed07a9..351ab54 100644
> > > +--- a/stat.c
> > > ++++ b/stat.c
> > > +@@ -21,6 +21,8 @@
> > > +
> > > + #include "fs.h"
> > > +
> > > ++#define __PRINT(STR) printf (STR,NULL)
> > > ++
> > Can we use proper formatting string here something like
> > printf(“%s”, access );
> > 
> > or use fputs() Call instead
> With fputs we need to specify stdout stream and
> the printf "%s" option needs a little bit more processing in formatting.
> 
> The actual change covers the security considerations with minimal add of 
> NULL if you
> know why the another ways will be better please tell me.
> 
> Thanks in advance
> Edwin Plauchu
> > 
> > 
> > > 
> > > + void print_human_type(unsigned short mode)
> > > + {
> > > +   switch (mode & S_IFMT)
> > > +@@ -289,15 +291,15 @@ void print_human_access(struct stat *statbuf)
> > > +     default:
> > > +       access[0] = '?';
> > > +     }
> > > +-    printf (access);
> > > ++    __PRINT(access);
> > > + }
> > > +
> > > + void print_human_time(time_t *t)
> > > + {
> > > +   char str[40];
> > > +
> > > +-  if (strftime(str, 40, "%c", localtime(t)) > 0) printf(str);
> > > +-  else printf("Cannot calculate human readable time, sorry");
> > > ++  if (strftime(str, 40, "%c", localtime(t)) > 0) __PRINT(str);
> > > ++  else __PRINT("Cannot calculate human readable time, sorry");
> > > + }
> > > +
> > > + /* print statfs info */
> > > +@@ -610,7 +612,7 @@ void print_it(char *masterformat, char *filename,
> > > + 	{
> > > + 	    strcpy (pformat, "%");
> > > + 	    *m++ = '\0';
> > > +-	    printf(b);
> > > ++	    __PRINT(b);
> > > +
> > > + 	    /* copy all format specifiers to our format string */
> > > + 	    while (isdigit(*m) || strchr("#0-+. I", *m))
> > > +@@ -639,7 +641,7 @@ void print_it(char *masterformat, char *filename,
> > > + 	}
> > > + 	else
> > > + 	{
> > > +-	    printf(b);
> > > ++	    __PRINT(b);
> > > + 	    b = NULL;
> > > + 	}
> > > +     }


Is there a particular reason you used a macro for this package when all the
others you submitted so far use printf(arg, NULL) directly? I think it would be
good to be consistent.

    -Bill