security goals re strcpy/strcat/sprintf (Re: "Secure coding" audit checkers and Kerberos)

[Nicolas Williams]

On Wed, Oct 15, 2008 at 11:55:03PM -0400, Greg Hudson wrote:
> On Wed, 2008-10-15 at 21:58 -0500, Nicolas Williams wrote:
> > You use a precision field for %s?  Where?
> 
> Not often.  Precision fields:
> 
> ./src/kadmin/ktutil/ktutil_funcs.c:     sprintf(promptstr, "Password for %.1000s", princ_str);
> ./src/kadmin/cli/kadmin.c:      sprintf(prompt1, "Enter password for principal \"%.900s\"",
> ./src/kadmin/cli/kadmin.c:              "Re-enter password for principal \"%.900s\"",
> ./src/kadmin/cli/kadmin.c:      sprintf(prompt1, "Enter password for principal \"%.900s\"",
> ./src/kadmin/cli/kadmin.c:              "Re-enter password for principal \"%.900s\"",
> 
> The precision field in those uses can probably be removed if the calls
> are switched to use snprintf.

Yeah, those are all sprintf() calls.  You shouldn't use sprintf().
(Yes, I wrote in favor of using *s*printf(), but I should have qualified
that to exclude sprintf() itself.)

> We use field widths with %s in 33 places, but it's all printf and
> fprintf, not sprintf.  Mostly in plugins/kdb.

I don't think that's a big deal if the output is intended for humans,
but if the fprintf()s are to files and those can be re-read, that could
be an issue.

> On another note, I have done a spot check of the uses of strcpy and
> strcat in the krb5 source tree, and a great many of them appear to be
> easily replaced with either strdup() or asprintf(), reducing the code
> size in the process.  There are some exceptions, but it might be easier
> to decide on an approach for the exceptions after eliminating the
> common, easy cases.

I agree.