[Pkg-shadow-devel] (no subject)

[Nicolas François]

Hello,

On Mon, Apr 27, 2009 at 01:37:10PM -0500, knotwurk at gmail.com wrote:
> I used the pscan utility on some of the source files for the
> login/shadow package (the lenny src), and received this error:
> 
> login.c:613 SECURITY: snprintf call should have "%s" as argument 2
> login.c:613 FUNC snprintf Last argument is variable or reference: BAD
> 
> I managed to get it to go away by changing this:
>  613                                 snprintf (loginprompt,
>  614                                           sizeof (loginprompt),
>  615                                           _("login: "));

I don't think there is a security issue here.
The problem is that gettext may return a string with a format.
and so:
	snprintf (loginprompt, sizeof (loginprompt), "%s", _("login: "));
would be more secure.


Unfortunately, format string are needed in translations, and there are
many places were the same solution cannot be applied (for example, the
snprintf above the one you pointed - which I do not understand why pscan
did not raise a warning).


The good news is that users cannot change the location of the gettext
catalogs (fixed at compile time), and the compatibility of the format
strings in the original and translation is also checked at build time.

(those assertions should be checked, as it's being quite late now, IIRC,
this was also the goal of sanitize_env, which should not be needed anymore
nowadays)

> to this
>  613                                 int short nohostn="unknown host";
>  614                                 snprintf (loginprompt,
>  615                                           sizeof (loginprompt),
>  616                                           _("%d login: "), nohostn);
> 
> I figured the chances of someone not having noticed this was remote,
> but what the hell.

I changed it to just a strncpy in that specific case, as the snprintf was
not needed neither.

Best Regards,
-- 
Nekral