Pageviews

Sunday, 24 June 2018

Insecure Permissions in GIMP - CVE-2018-12713

Hi Internet,

#ShortPost

Summary: GIMP through 2.10.2 makes g_get_tmp_dir calls to establish temporary filenames, which may result in a filename that already exists, as demonstrated by the gimp_write_and_read_file function in app/tests/test-xcf.c. This might be leveraged by attackers to overwrite files or read file content that was intended to be private.
Img Src: https://www.gimp.org/

While having a look on GIMP code I observed that,
File test-xcf.cat#L314  which was;
filename = g_build_filename (g_get_tmp_dir (), "gimp-test.xcf", NULL);
The function with 'getenv("TMP")';it returns untrustable input. This issue was reported to GNOME GIMP team and a patch was pushed.

   GimpImage           *image;
   GimpImage           *loaded_image;
   GimpPlugInProcedure *proc;
-  gchar               *filename;
+  gchar               *filename = NULL;
+  gint                 file_handle;
   GFile               *file;
 
   /* Create the image */
@@ -311,7 +312,9 @@ gimp_write_and_read_file (Gimp     *gimp,
                          use_gimp_2_8_features);
 
   /* Write to file */
-  filename = g_build_filename (g_get_tmp_dir (), "gimp-test.xcf", NULL);
+  file_handle = g_file_open_tmp ("gimp-test-XXXXXX.xcf", &filename, NULL);
+  g_assert (file_handle != -1);
+  close (file_handle);
   file = g_file_new_for_path (filename);
   g_free (filename);
Not sure this is really solving the issue reported, which is that `g_get_tmp_dir()` uses environment variables (yet as g_file_open_tmp() uses g_get_tmp_dir()…). But at least g_file_open_tmp() should create unique temporary files, which prevents overriding existing files (which is most likely the only real attack possible here, or at least the only one I can think of unless some weird vulnerabilities exist in glib) CVE-2018-12713 was assigned to this issue.


Regards
Dhiraj

3 comments:

  1. Hey, this code been using only during tests, its not part of the application at all :)

    ReplyDelete
  2. To be clear, to the best of my knowledge this ‘vulnerability’ only exists in unit tests, which are not (and never have been) shipped to users. This ‘vulnerability’ does not affect users. I don’t think it warrants a CVE number.

    I believe that labelling and promoting vulnerabilities which don’t actually affect users detracts from the ones we actually need to care about. Please don’t do that. Save your efforts for things which actually have an impact.

    ReplyDelete
  3. ... and how is that title related to the issue? "Insecure permissions" would match rather that issue with access(), and not really this one.

    ReplyDelete