[bug #60977] [troff]: uses sprintf to write to buffer using user-controlled format string

[G. Branden Robinson]

URL:
  <https://savannah.gnu.org/bugs/?60977>

                 Summary: [troff]: uses sprintf to write to buffer using
user-controlled format string
                 Project: GNU troff
            Submitted by: gbranden
            Submitted on: Tue 27 Jul 2021 01:48:32 AM UTC
                Category: Core
                Severity: 4 - Important
              Item Group: None
                  Status: In Progress
                 Privacy: Public
             Assigned to: gbranden
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None

    _______________________________________________________

Details:

A compiler warning points out a legitimate issue.


../src/roff/troff/node.cpp: In member function ‘virtual void
suppress_node::tprint(troff_output_file*)’:
../src/roff/troff/node.cpp:4105:50: warning: format not a string literal,
argument types not checked [-Wformat-nonliteral]
  sprintf(name, last_image_filename, last_image_id);
                                                  ^


In this case the format string, "last_image_filename", comes from an \O5
escape sequence, such as `\O[5lfoobar-%d.png]`.

I haven't been able to force anything untoward to happen using this escape
sequence in a groff REPL, but I didn't try very hard and I am not an
experienced system penetrator.

I have a fix in progress.


diff --git a/src/preproc/html/pre-html.cpp b/src/preproc/html/pre-html.cpp
index 88fcca18..66618c1f 100644
--- a/src/preproc/html/pre-html.cpp
+++ b/src/preproc/html/pre-html.cpp
@@ -544,6 +544,7 @@ static void makeFileName(void)
   if (image_template == NULL)
     sys_fatal("malloc");
   strcpy(image_template, macroset_template);
+  // Keep format string synced with troff:suppress_node::tprint().
   strcat(image_template, "%d");
 }
 
diff --git a/src/roff/troff/node.cpp b/src/roff/troff/node.cpp
index f2fc6da8..87cd0d60 100644
--- a/src/roff/troff/node.cpp
+++ b/src/roff/troff/node.cpp
@@ -4053,8 +4053,8 @@ void suppress_node::put(troff_output_file *out, const
char *s)
  */
 
 static char last_position = 0;
-static const char *last_image_filename = 0;
-static int last_image_id = 0;
+static const char *image_filename = 0;
+static int subimage_counter = 0;
 
 inline int min(int a, int b)
 {
@@ -4086,23 +4086,53 @@ void suppress_node::tprint(troff_output_file *out)
   if (is_on == 2) {
     // remember position and filename
     last_position = position;
-    char *tem = (char *)last_image_filename;
-    last_image_filename = strsave(filename.contents());
-    if (tem)
-      free(tem);
-    last_image_id = image_id;
-    // printf("start of image and page = %d\n", current_page);
+    image_filename = strsave(filename.contents());
   }
   else {
-    // now check whether the suppress node requires us to issue limits.
+    // Now check whether the suppress node requires us to issue limits.
     if (emit_limits) {
       char name[8192];
-      // remember that the filename will contain a %d in which the
-      // last_image_id is placed
-      if (last_image_filename == (char *) 0)
+      if (image_filename == (char *) 0)
        *name = '\0';
-      else
-       sprintf(name, last_image_filename, last_image_id);
+      else {
+       // Jump through a flaming hoop to avoid a "format nonliteral"
+       // warning from blindly using sprintf...and avoid trouble from
+       // mischievous image stems.
+       //
+       // Keep this format string synced with pre-html:makeFileName().
+       const char format[] = "%d";
+       const size_t format_len = strlen(format);
+       const char *percent_position = strstr(image_filename, format);
+       if (percent_position) {
+         subimage_counter++;
+         assert(sizeof subimage_counter <= 8);
+         // A 64-bit signed int produces up to 19 decimal digits.
+         char *subimage_number = (char *)malloc(20); // 19 digits + \0
+         if (0 == subimage_number)
+           fatal("memory allocation failure");
+         // Replace the %d in the filename with this number.
+         size_t enough = strlen(image_filename) + 19 - format_len;
+         char *new_name = (char *)malloc(enough);
+         if (0 == new_name)
+           fatal("memory allocation failure");
+         ptrdiff_t prefix_length = percent_position - image_filename;
+         strncpy(new_name, image_filename, prefix_length);
+         sprintf(subimage_number, "%d", subimage_counter);
+         size_t number_length = strlen(subimage_number);
+         strcpy(new_name + prefix_length, subimage_number);
+         // Skip over the format in the source string.
+         const char *suffix_src = image_filename + prefix_length
+           + format_len;
+         char *suffix_dst = new_name + prefix_length + number_length;
+         strcat(suffix_dst, suffix_src);
+         assert(strlen(new_name) < 8191);
+         strncpy(name, new_name, 8191);
+         free(new_name);
+         free(subimage_number);
+       }
+       else
+         strcpy(name, image_filename);
+      }
       if (is_html) {
        switch (last_position) {
        case 'c':





    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?60977>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/