Bug 70618 - print/a2ps-* using "file -L %s" as shell argument --> dangerous to use it in world-writable directories
Summary: print/a2ps-* using "file -L %s" as shell argument --> dangerous to use it in ...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Only Me
Assignee: Dirk Meyer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-08-18 16:20 UTC by Rudolf Polzer
Modified: 2004-08-21 11:23 UTC (History)
1 user (show)

See Also:


Attachments
file.diff (1.57 KB, patch)
2004-08-18 16:20 UTC, Rudolf Polzer
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rudolf Polzer 2004-08-18 16:20:26 UTC
a2ps builds a command line for file() containing an unescaped version of
the file name, thus might call external programs described by the file name.
Running a cronjob over a public writable directory a2ps-ing all files in it
- or simply typing "a2ps *.txt" in /tmp - is therefore dangerous.

Fix: Apply this patch (can be directly put into files/):
How-To-Repeat: rpolzer:/tmp 7> touch 'x`echo>&2 42`'                                      
rpolzer:/tmp 8> a2ps -o /dev/null 'x`echo>&2 42`'
42
[x`echo>&2 42` (plain): 0 pages on 0 sheets]
[Total: 0 pages on 0 sheets] saved into the file `/dev/null'
Comment 1 polzer 2004-08-18 16:38:39 UTC
The patch has a minor bug: in out-of-memory conditions, it attempts to free
the empty string "". Perhaps one should better return NULL in this case
and let the other function return NULL, too, that is, change

+  if(!outp)
+    return ""; /* perhaps one should do better error handling here */

into

+  if(!outp)
+    return NULL;

and

+  filename = shell_escape(filename);

into

+  filename = shell_escape(filename);
+  if(filename == NULL)
+    return NULL;

After that, IIRC

@@ -144,11 +174,13 @@

must be changed into

@@ -144,11 +174,15 @@

Then it should work even then.
Comment 2 Rudolf Polzer 2004-08-18 17:00:46 UTC
The patch has a bug in the handling of a NULL return value of malloc.
Here is a "better" patch:


diff -ru ../a2ps-4.13.orig/src/select.c ./src/select.c
--- ../a2ps-4.13.orig/src/select.c	Wed Aug 18 16:32:09 2004
+++ ./src/select.c	Wed Aug 18 16:49:12 2004
@@ -131,6 +131,36 @@
   return 1;
 }
 
+/* escapes the name of a file so that the shell groks it in 'single' q.marks. 
+   The resulting pointer has to be free()ed when not longer used. */
+char *
+shell_escape(const char *fn)
+{
+  size_t len = 0;
+  const char *inp;
+  char *retval, *outp;
+
+  for(inp = fn; *inp; ++inp)
+    switch(*inp)
+    {
+      case '\'': len += 4; break;
+      default:   len += 1; break;
+    }
+
+  outp = retval = malloc(len + 1);
+  if(!outp)
+    return NULL; /* perhaps one should do better error handling here */
+  for(inp = fn; *inp; ++inp)
+    switch(*inp)
+    {
+      case '\'': *outp++ = '\''; *outp++ = '\\'; *outp++ = '\'', *outp++ = '\''; break;
+      default:   *outp++ = *inp; break;
+    }
+  *outp = 0;
+
+  return retval;
+}
+
 /* What says file about the type of a file (result is malloc'd).  NULL
   if could not be run.  */
 
@@ -144,11 +174,15 @@
   if (IS_EMPTY (job->file_command))
     return NULL;
 
+  filename = shell_escape(filename);
+  if(filename == NULL)
+    return NULL;
   /* Call file(1) with the correct option */
-  command = ALLOCA (char, (2
+  command = ALLOCA (char, (4
 			   + strlen (job->file_command)
 			   + ustrlen (filename)));
-  sprintf (command, "%s %s", job->file_command, (const char *) filename);
+  sprintf (command, "%s '%s'", job->file_command, (const char *) filename);
+  free(filename);
   message (msg_tool, (stderr, "Reading pipe: `%s'\n", command));
   file_out = popen (command, "r");
Comment 3 Mark Linimon freebsd_committer freebsd_triage 2004-08-20 21:34:00 UTC
Responsible Changed
From-To: freebsd-ports-bugs->dinoex

Over to maintainer.
Comment 4 Dirk Meyer freebsd_committer 2004-08-21 11:23:01 UTC
State Changed
From-To: open->closed

committed, thanks.