sharutils does not build with -Werror=format-security

[Santiago Vila]

Hello.

The following patch is required to allow compilation of
sharutils 4.13.5 with -Werror=format-security.

Thanks.


From: Simon Ruderich <***@ruderich.org>
Subject: Allow compilation with -Werror=format-security
Bug-Debian: http://bugs.debian.org/700313
X-Debian-version: 1:4.11.1-2

--- a/src/shar.c
+++ b/src/shar.c
@@ -461,7 +461,7 @@

if (stat (local_name, &struct_stat))
{
- error (0, errno, local_name);
+ error (0, errno, "%s", local_name);
return SHAR_EXIT_FILE_NOT_FOUND;
}

@@ -470,7 +470,7 @@

if (directory = opendir (local_name), !directory)
{
- error (0, errno, local_name);
+ error (0, errno, "%s", local_name);
return SHAR_EXIT_CANNOT_OPENDIR;
}

@@ -563,7 +563,7 @@
#else
if (closedir (directory))
{
- error (0, errno, local_name);
+ error (0, errno, "%s", local_name);
return SHAR_EXIT_CANNOT_OPENDIR;
}
#endif
@@ -954,7 +954,7 @@
free (c_dir);
}
else
- error (0, errno, _("Cannot get current directory name"));
+ error (0, errno, "%s", _("Cannot get current directory name"));
}
}

@@ -2134,7 +2134,7 @@
*/
FILE * fp = freopen ("/dev/null", fwriteonly_mode, stderr);
if (fp != stderr)
- error (SHAR_EXIT_FAILED, errno,
+ error (SHAR_EXIT_FAILED, errno, "%s",
_("reopening stderr to /dev/null"));
}

@@ -2200,7 +2200,7 @@
if (HAVE_OPT(QUERY_USER))
{
if (HAVE_OPT(NET_HEADERS))
- error (0, 0, _("PLEASE avoid -X shars on Usenet or public networks"));
+ error (0, 0, "%s", _("PLEASE avoid -X shars on Usenet or public networks"));

fputs ("shar_wish=\n", output);
}
@@ -2348,7 +2348,7 @@
optionLoadLine (&sharOptions, arg);
}
else
- error (0, errno, arg);
+ error (0, errno, "%s", arg);
continue;
}

[Bruce Korb]

Post by Santiago Vila
Hello.
The following patch is required to allow compilation of
sharutils 4.13.5 with -Werror=format-security.
Thanks.

Hi Santiago, Simon, et al.

OK. I give: what possible good is -Werror=format-security?
According to the doc, it is dangerous to use variables as formats
because the variable "just might" contain a %n and wreak havoc.
That makes i18n an interesting challenge. If the format contains
translatable text, the formatting string *MUST* not be static.

Post by Santiago Vila
static int
check_accessibility (const char *local_name, const char *restore_name)
{
if (access (local_name, 4))
{
error (0, errno, _("Cannot access %s"), local_name);
return SHAR_EXIT_FILE_NOT_FOUND;
}
@@ -954,7 +954,7 @@
free (c_dir);
}
else
- error (0, errno, _("Cannot get current directory name"));
+ error (0, errno, "%s", _("Cannot get current directory name"));
}
}

this change is pointless nonsense. I think GCC went off the rails when
they warned "OMG! You have a NUL byte in the format string" and this
is again an overkill. If someone switches out the l10n library for
a bogus one, then you get the old garbage-in-garbage-out problem.

Where the third argument to error is, in fact, some variable string,
I will pass it through a "%s" format specifier. I suspect they should
all be validated as clean, but that patch is easier than verifying
validity. Compile with this warning set IFF NLS is disabled.
If Debian/GCC still complains, then the warning should be silenced.

Post by Santiago Vila
diff --git a/src/shar.c b/src/shar.c
index f12cf19..3ce860c 100644
--- a/src/shar.c
+++ b/src/shar.c
@@ -444,7 +444,7 @@ walkdown (
if (stat (local_name, &struct_stat))
{
- error (0, errno, local_name);
+ error (0, errno, "%s", local_name);
return SHAR_EXIT_FILE_NOT_FOUND;
}
@@ -453,7 +453,7 @@ walkdown (
if (directory = opendir (local_name), !directory)
{
- error (0, errno, local_name);
+ error (0, errno, "%s", local_name);
return SHAR_EXIT_CANNOT_OPENDIR;
}
@@ -535,7 +535,7 @@ walkdown (
#else
if (closedir (directory))
{
- error (0, errno, local_name);
+ error (0, errno, "%s", local_name);
return SHAR_EXIT_CANNOT_OPENDIR;
}
#endif
@@ -587,7 +587,7 @@ walktree (walker_t routine, const char *local_name)
if (status != 0)
{
- error (0, errno, local_name_copy);
+ error (0, errno, "%s", local_name_copy);
status = SHAR_EXIT_FILE_NOT_FOUND;
}
else
@@ -2368,7 +2368,7 @@ main (int argc, char ** argv)
optionLoadLine (&sharOptions, arg);
}
else
- error (0, errno, arg);
+ error (0, errno, "%s", arg);
continue;
}

[Paul Eggert]

Post by Bruce Korb
That makes i18n an interesting challenge.

Yes, the assumption is that the translation libraries
are protected from tampering, just as much as the
application itself is; otherwise, one can attack
the application by supplying bogus translations.
So you're right that if FOO is safe to use as
a format string, then _(FOO) should be safe too.

[Andreas Schwab]

Post by Paul Eggert

Post by Bruce Korb
That makes i18n an interesting challenge.

Yes, the assumption is that the translation libraries
are protected from tampering,

And the translations itself.

Andreas.

[Santiago Vila]

So, your recommendation is that I forget about -Werror=format-security entirely in sharutils?

Is there a way to tell gcc "this line is correct" without disabling -Werror=format-security
everywhere?

[Santiago Vila]

BTW: Debian Bug#700313 is archived and the email ***@bugs.debian.org bounces.
You may better skip Cc: to such address. Thanks.

[Bruce Korb]

Hi Eric,

Post by Santiago Vila
else
- error (0, errno, _("Cannot get current directory name"));
+ error (0, errno, "%s", _("Cannot get current directory name"));

but in THIS form, xgettext sees no % mark, so it does NOT mark the .po
file, and therefore gettext() no longer attempts to sanitize the
translation, and a translator can sneak in any % mark. Yes, you can
argue that xgettext could be made smarter to properly annotate
string-literal formats with a printf annotation to force runtime
sanitization of the transalation, but for now, adding a "%s" format
argument is the easiest way to shut up the existing tool chain rather
than waiting for a fixed toolchain.

Then in this particular case, you are arguing for muddying up code
to accommodate deficiencies in the code analysis? I definitely
prefer the "do it properly and let the tools get fixed" approach.
To that end, I went to the trouble of adding -Wformat-contains-nul
warning to GCC.

So you're right that if FOO is safe to use as
a format string, then _(FOO) should be safe too.

http://autogen.sourceforge.net/data/sharutils-4.13.6pre3.tar.xz

I'll actually release 4.13.6 "RSN". Cheers - Bruce

[Bruce Korb]

Post by Santiago Vila
So, your recommendation is that I forget about -Werror=format-security entirely in sharutils?

I'd recommend disabling that error mode if the patched sharutils still fails.
If disabling NLS causes the warning to disappear, then use the warning IFF
NLS is disabled. (It should -- since the format strings would be constant
at that point.) In other words, you would have a "compile only" test to
verify that other-than-NLS-translated-strings were okay.

Post by Santiago Vila
Is there a way to tell gcc "this line is correct" without disabling -Werror=format-security
everywhere?

#pragma-s, but that won't be happening. That is much worse than "%s"-wrapping
the translated strings.