#22 predictable /tmp paths in PreviewPopup::DisplayImage()

[Wolfgang Frisch]

4Pane uses predictable /tmp paths in PreviewPopup::DisplayImage()

 pngfilepath = "/tmp/" + fn.GetName() + ".png";
 if (SvgToPng(filepath, pngfilepath, handle))
   image = wxImage(pngfilepath);
 wxRemoveFile(pngfilepath);

If fs.protected_symlinks=1, an unprivileged user can prevent 4Pane from displaying previews for SVG images.
If fs.protected_symlinks=0, an unprivileged user can overwrite arbitrary world-readable files owned by the 4Pane user.

Steps to reproduce:

nobody@localhost:/tmp> ln -s /home/user/somefile foo.png
# ... wait until the user previews a file named foo.svg
# somefile will be overwritten

An attacker can pre-create symlinks for the names of all existing SVG files on the system to increase the likelihood of triggering the bug.

https://bugzilla.suse.com/show_bug.cgi?id=1225509

[David Hart]

Thank you for your report, and I apologise for the delay. This is now fixed by #d8b74e.

However I couldn't reproduce this here (debian) or in openSUSE 15.6. Though the behaviour you describe should in theory have occurred, in practice the file failed to be overwritten. Did it actually happen in tumbleweed?

[Wolfgang Frisch]

P.S.

Though the behaviour you describe should in theory have occurred, in practice the file failed to be overwritten. Did it actually happen in tumbleweed?

Files are only overwritten when /proc/sys/fs/protected_symlinks is 0, which is not the default on most distros.
If it's set to 1 the bug only prevents 4Pane from displaying SVG previews.

[Wolfgang Frisch]

Hi David,

thanks for the fix. Unfortunately there's still a problem:
srand(time(NULL)); makes the generated values very easy to predict, because time(NULL) has a granularity of just one second.
I suggest mkstemp() which generates secure /tmp file names, or,
if you really want to do it manually, consider C++ STL random functions (for example std::random_device and std::mt19937).

The issue was reproducible on openSUSE Tumbleweed. I have not tried other distros.

All the best,
Wolfgang

[Wolfgang Frisch]

Please find attached a patch that fixes the remaining issue. wxFileName::CreateTempFileName() uses the secure mkstemp() function internally.

commit 27f353b38817a90370d8908f8b71c28c3915f848 (HEAD -> fix_predictable_rng_tmp)
Author: Wolfgang Frisch <wolfgang.frisch@suse.com>
Date:   Mon Jan 13 14:12:47 2025 +0100

    Generate secure temporary paths in DisplayImage()

    srand(time(NULL)) is easy to predict because time() has a granularity of
    just one second. Instead use wxWidgets' CreateTempFileName() function.

    See also:

    - commit d8b74e4df86fb526ee9caad284b9eb3efe528ac5
    - https://sourceforge.net/p/fourpane/bugs/22/

diff --git a/MyTreeCtrl.cpp b/MyTreeCtrl.cpp
index d82cfc8..1b4a82d 100644
--- a/MyTreeCtrl.cpp
+++ b/MyTreeCtrl.cpp
@@ -1890,7 +1890,7 @@ void PreviewPopup::DisplayImage(const wxString& fpath)
 {
 wxLogNull NoErrorMessages;
 wxString filepath(fpath);
-wxString pngfilepath, rndstr;
+wxString pngfilepath;
 wxImage image;

 if (filepath.Right(4) == ".svg")
@@ -1898,12 +1898,11 @@ if (filepath.Right(4) == ".svg")
     if (!handle) return; // Presumably librsvg is not available at present

     // Create a filepath in /tmp/ to store the .png

-    rndstr = ""; srand(time(NULL));
-    wxString allowedchars="ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
-    for (size_t n=0; n < 9; ++n) // Make the string name unguessable; see https://sourceforge.net/p/fourpane/bugs/22/
-       rndstr << wxString::Format("%c", allowedchars[(char)(rand() % 52)]);
     wxFileName fn(filepath);
-    pngfilepath = "/tmp/" + rndstr + fn.GetName() + ".png";
+    pngfilepath = wxFileName::CreateTempFileName(fn.GetName());
+    if (pngfilepath.empty()) {
+        wxLogWarning(wxT("CreateTempFileName() failed in DisplayImage()"));
+    }
     if (SvgToPng(filepath, pngfilepath, handle))
       image = wxImage(pngfilepath);
     wxRemoveFile(pngfilepath);

[David Hart]

Thanks again. In my senility I'd completely forgotten about wxFileName::CreateTempFileName. Now committed.