[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    [PATCH] .desktop security ++
From:       Michael Pyne <mpyne () purinchu ! net>
Date:       2009-02-21 5:36:38
Message-ID: 200902210036.42729.mpyne () purinchu ! net
[Download RAW message or body]

[Attachment #2 (multipart/mixed)]

[Attachment #4 (multipart/alternative)]


Hi all,

I've implemented the auto-exec-bit-ifying of .desktop files in KRun and made 
the changes suggested in the last couple of threads, including using owned-by-
root as an exception as originally discussed instead of merely "not writable 
by user".

Currently the work is in 3 patches (all attached):

The first patch changes KDesktopFile::isAuthorizedDesktopFile() to include the 
extra restrictions that we're placing on .desktop files.  This should be 
applied first as the remaining patches both require this change.

The second path implements security by not allowing klauncher to launch a 
.desktop file that doesn't meet the criteria given by 
KDesktopFile::isAuthorizedDesktopFile() (a fairly large change from my first 
submission but all that's happened is the logic is defined in KDesktopFile 
instead of being duplicated)

The third patch is against KRun, and implements the auto +x.  This _needs 
review_, it was basically all written today after I got home from work (which 
included running 5K in shorts while it was 1 C outside so it was a rough day 
;)

The idea is pop up a nice dialog [1] giving the user a readable description of 
what the problem is.  A Details button is supposed to be included which 
contains the Exec= line but isn't working for some reason.  Also the amount of 
text buys us into the dreaded Qt layout vs. X11 bug which I've tried to 
minimize the effects of.  On that note, I'm game to better ways to phrase this 
dialog, it doesn't seem efficient somehow.

Assuming the user clicks on continue the file is made executable by doing 2 
things:

1. Add a #!/usr/bin/env xdg-open (if #! is not already present)
 - This is done by using KSaveFile, by writing the header and then dumping the 
.desktop file contents below it.  I'm not really happy about manually moving 
bytes around (especially as v1 in my testing today had an infinite loop) but I 
don't trust readAll()/write() for library code.  Please look at this to make 
sure I've done it right.

2. chmod u+x /path/to/foo.desktop (this was simpler ;)

Assuming everything proceeded swimmingly the .desktop file is then immediately 
launched.

Is there anything I'm missing here now?  Please let me know, otherwise I'd 
like to know if there are objections to committing on Sunday.

Regards,
 - Michael Pyne

[1] http://purinchu.net/dumping-ground/krun2.png

[Attachment #7 (text/html)]

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" \
"http://www.w3.org/TR/REC-html40/strict.dtd"><html><head><meta name="qrichtext" \
content="1" /><style type="text/css">p, li { white-space: pre-wrap; \
}</style></head><body style=" font-family:'Consolas'; font-size:11pt; \
font-weight:400; font-style:normal;">Hi all,<br> <p style="-qt-paragraph-type:empty; \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>I've implemented the \
auto-exec-bit-ifying of .desktop files in KRun and made the changes suggested in the \
last couple of threads, including using owned-by-root as an exception as originally \
discussed instead of merely "not writable by user".<br> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"><br></p>Currently the work is in 3 patches (all attached):<br> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>The \
first patch changes KDesktopFile::isAuthorizedDesktopFile() to include the extra \
restrictions that we're placing on .desktop files.  This should be applied first as \
the remaining patches both require this change.<br> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>The \
second path implements security by not allowing klauncher to launch a .desktop file \
that doesn't meet the criteria given by KDesktopFile::isAuthorizedDesktopFile() (a \
fairly large change from my first submission but all that's happened is the logic is \
defined in KDesktopFile instead of being duplicated)<br> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>The \
third patch is against KRun, and implements the auto +x.  This _needs review_, it was \
basically all written today after I got home from work (which included running 5K in \
shorts while it was 1 C outside so it was a rough day ;)<br> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>The \
idea is pop up a nice dialog [1] giving the user a readable description of what the \
problem is.  A Details button is supposed to be included which contains the Exec= \
line but isn't working for some reason.  Also the amount of text buys us into the \
dreaded Qt layout vs. X11 bug which I've tried to minimize the effects of.  On that \
note, I'm game to better ways to phrase this dialog, it doesn't seem efficient \
somehow.<br> <p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"><br></p>Assuming the user clicks on continue the file is made \
executable by doing 2 things:<br> <p style="-qt-paragraph-type:empty; margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;"><br></p>1. Add a #!/usr/bin/env xdg-open (if #! \
                is not already present)<br>
 - This is done by using KSaveFile, by writing the header and then dumping the \
.desktop file contents below it.  I'm not really happy about manually moving bytes \
around (especially as v1 in my testing today had an infinite loop) but I don't trust \
readAll()/write() for library code.  Please look at this to make sure I've done it \
right.<br> <p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"><br></p>2. chmod u+x /path/to/foo.desktop (this was simpler ;)<br> \
<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"><br></p>Assuming everything proceeded swimmingly the .desktop file \
is then immediately launched.<br> <p style="-qt-paragraph-type:empty; margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;"><br></p>Is there anything I'm missing here now?  \
Please let me know, otherwise I'd like to know if there are objections to committing \
on Sunday.<br> <p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
                -qt-user-state:0;"><br></p>Regards,<br>
 - Michael Pyne<br>
<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"><br></p>[1] \
http://purinchu.net/dumping-ground/krun2.png</p></body></html>


["brouhaha-001-kdesktopfile.patch" (text/x-patch)]

Index: kdecore/config/kdesktopfile.cpp
===================================================================
--- kdecore/config/kdesktopfile.cpp	(revision 928782)
+++ kdecore/config/kdesktopfile.cpp	(working copy)
@@ -24,6 +24,7 @@
 #include <unistd.h>
 
 #include <QtCore/QDir>
+#include <QtCore/QFileInfo>
 
 #include <config.h>
 #include "kconfig_p.h"
@@ -139,8 +140,13 @@
 
 bool KDesktopFile::isAuthorizedDesktopFile(const QString& path)
 {
-  if (KAuthorized::authorize("run_desktop_files"))
-     return true;
+  // Explicitly forbid desktop files if Kiosk does not allow.  There
+  // may be other reasons that desktop files are forbidden so keep
+  // checking otherwise.
+  if (!KAuthorized::authorize("run_desktop_files")) {
+     kWarning() << "Access to '" << path << "' denied because of 'run_desktop_files' \
restriction." << endl; +     return false;
+  }
 
   if (path.isEmpty())
      return false; // Empty paths are not ok.
@@ -155,10 +161,14 @@
      return true;
   if (QDir::isRelativePath( dirs->relativeLocation("services", path) ))
      return true;
-  if (dirs->relativeLocation("data", path).startsWith("kdesktop/Desktop"))
-     return true;
 
-  kWarning() << "Access to '" << path << "' denied because of 'run_desktop_files' \
restriction." << endl; +  // Not otherwise permitted, so only allow if the file is \
executable, or if +  // owned by root (uid == 0)
+  QFileInfo entryInfo( path );
+  if (entryInfo.isExecutable() || entryInfo.ownerId() == 0)
+      return true;
+
+  kWarning() << "Access to '" << path << "' denied because of \
'non_executable_desktop_file' restriction." << endl;  return false;
 }
 


["brouhaha-002-klauncher.patch" (text/x-patch)]

Index: kinit/klauncher.cpp
===================================================================
--- kinit/klauncher.cpp	(revision 928782)
+++ kinit/klauncher.cpp	(working copy)
@@ -49,6 +49,7 @@
 #include <krun.h>
 #include <kstandarddirs.h>
 #include <ktemporaryfile.h>
+#include <kdesktopfile.h>
 #include <kurl.h>
 
 #include <kio/global.h>
@@ -780,10 +781,15 @@
                          bool blind, bool autoStart, const QDBusMessage &msg)
 {
    QStringList urls = _urls;
-   if (!service->isValid())
+   bool runPermitted = KDesktopFile::isAuthorizedDesktopFile(service->entryPath());
+
+   if (!service->isValid() || !runPermitted)
    {
       requestResult.result = ENOEXEC;
-      requestResult.error = i18n("Service '%1' is malformatted.", service->entryPath());
+      if (service->isValid())
+         requestResult.error = i18n("Service '%1' must be executable to run.", service->entryPath());
+      else
+         requestResult.error = i18n("Service '%1' is malformatted.", service->entryPath());
       cancel_service_startup_info( NULL, startup_id, envs ); // cancel it if any
       return false;
    }

["brouhaha-003-krun-autoexec.patch" (text/x-patch)]

Index: kio/kio/krun.cpp
===================================================================
--- kio/kio/krun.cpp	(revision 928782)
+++ kio/kio/krun.cpp	(working copy)
@@ -44,6 +44,7 @@
 #include "kfile/krecentdocument.h"
 #include "kdesktopfileactions.h"
 
+#include <kauthorized.h>
 #include <kmessageboxwrapper.h>
 #include <kurl.h>
 #include <kglobal.h>
@@ -65,6 +66,10 @@
 #include <QTextDocument>
 #include <kde_file.h>
 #include <kconfiggroup.h>
+#include <kdialog.h>
+#include <kstandardguiitem.h>
+#include <kguiitem.h>
+#include <ksavefile.h>
 
 #ifdef Q_WS_X11
 #include <kwindowsystem.h>
@@ -684,14 +689,184 @@
   return urls;
 }
 
+// Helper function to make the given .desktop file executable by ensuring
+// that a #!/usr/bin/env xdg-open line is added if necessary and the file has
+// the +x bit set for the user.  Returns false if either fails.
+static bool makeFileExecutable( const QString &fileName )
+{
+  // Open the file and read the first two characters, check if it's
+  // #!.  If not, create a new file, prepend appropriate lines, and copy
+  // over.
+  QFile desktopFile( fileName );
+  if (!desktopFile.open( QFile::ReadOnly )) {
+    kError(7010) << "Error opening service" << fileName << desktopFile.errorString();
+    return false;
+  }
+
+  QByteArray header = desktopFile.peek( 2 ); // First two chars of file
+  if (header.size() == 0) {
+    kError(7010) << "Error inspecting service" << fileName << desktopFile.errorString();
+    return false; // Some kind of error
+  }
+
+  if (header != "#!")
+  {
+    // Add header
+    KSaveFile saveFile;
+    saveFile.setFileName( fileName );
+    if (!saveFile.open()) {
+      kError(7010) << "Unable to open replacement file for" << fileName << saveFile.errorString();
+      return false;
+    }
+
+    QByteArray shebang("#!/usr/bin/env xdg-open\n");
+    if(-1 == saveFile.write( shebang ))
+    {
+      // A mere half-write isn't a failure mode I care to code about
+      kError(7010) << "Error occurred adding header for" << fileName << saveFile.errorString();
+      saveFile.abort();
+      return false;
+    }
+
+    // Now copy the one into the other and then close and reopen desktopFile
+    const int bufSize = 1024;
+    char buffer[bufSize];
+    qint64 numRead, numWritten;
+
+    while(!desktopFile.atEnd())
+    {
+      numRead = desktopFile.read( buffer, bufSize );
+      if( numRead == -1 )
+      {
+        kError(7010) << "Unable to add header for" << fileName << desktopFile.errorString();
+        saveFile.abort();
+        return false;
+      }
+
+      numWritten = 0;
+      while( numWritten < numRead )
+      {
+        qint64 writtenThisTime = saveFile.write( &buffer[numWritten], numRead - numWritten );
+        if (writtenThisTime == -1)
+        {
+          kError(7010) << "Error copying service" << fileName << saveFile.errorString();
+          saveFile.abort();
+          return false;
+        }
+        numWritten += writtenThisTime;
+      }
+    }
+
+    desktopFile.close();
+    if (!saveFile.finalize())
+    { // Figures....
+      kError(7010) << "Error committing changes to service" << fileName << saveFile.errorString();
+      return false;
+    }
+
+    if (!desktopFile.open( QFile::ReadOnly ))
+    {
+      kError(7010) << "Error re-opening service" << fileName << desktopFile.errorString();
+      return false;
+    }
+  } // Add header
+
+  // corresponds to owner on unix, which will have to do since if the user
+  // isn't the owner we can't change perms anyways.
+  if (!desktopFile.setPermissions( QFile::ExeUser | desktopFile.permissions() ))
+  {
+    kError(7010) << "Unable to change permissions for" << fileName << desktopFile.errorString();
+    return false;
+  }
+
+  // whew
+  return true;
+}
+
+// Helper function to make a .desktop file executable if prompted by the user.
+// returns true if KRun::run() should continue with execution, false if user declined
+// to make the file executable or we failed to make it executable.
+static bool makeServiceExecutable( const KService& service, QWidget* window )
+{
+  if (!KAuthorized::authorize("run_desktop_files"))
+  {
+    kWarning() << "No authorization to execute " << service.entryPath();
+    KMessageBox::sorry( window, i18n("You are not authorized to execute this service.") );
+    return false; // Don't circumvent the Kiosk
+  }
+
+  QString serviceName = service.genericName();
+  if (serviceName.isEmpty())
+    serviceName = service.desktopEntryName();
+  QString continueStr = i18nc("@action:button",
+            "Make service executable and continue");
+
+  QString warningMessage = i18nc("@info",
+    "<para>You are trying to run the service <application>%1</application>, but it is not "
+    "marked as an executable program.  This may be a mistake in the system "
+    "configuration, but could also be an attempt at running a malicious program.</para>"
+    "<para>If you created this service, then click <interface>%2</interface> to mark the "
+    "service as executable and run it.</para> "
+    "<para>If you downloaded this service, it was emailed to you, or if you are simply "
+    "unsure then you should <interface>Cancel</interface>.</para>",
+    serviceName, continueStr);
+
+  // TODO: Make this actually show up in KMessageBox
+  QString details = i18n("This service will run the following command: %1", service.exec() );
+
+  KGuiItem continueItem = KStandardGuiItem::cont();
+  continueItem.setText( continueStr );
+
+  // We want to be able to provide the details window but only "sorry" message boxes have
+  // a static method so we need to use createKMessageBox, which means we need to provide
+  // the KDialog.  We'll change the Continue to have a more descriptive button but otherwise
+  // be the standard continue button.
+
+  KDialog *baseDialog = new KDialog( window );
+  baseDialog->setButtons( KDialog::Ok | KDialog::Cancel );
+  baseDialog->setButtonGuiItem( KDialog::Ok, continueItem );
+  baseDialog->setDefaultButton( KDialog::Cancel ); // NoDefault doesn't work?
+  baseDialog->setCaption( i18nc("Warning about executing unknown .desktop file", "Warning") );
+
+  // We must use NoExec because otherwise the message box will be queued and the function will
+  // instead return immediately with no result code.
+  KMessageBox::createKMessageBox(
+         baseDialog, QMessageBox::Warning, warningMessage, QStringList(),
+         QString(), 0 /* bool* */, KMessageBox::NoExec, details);
+
+  baseDialog->setMinimumSize(400, 270); // Long text Qt bug still exists...
+  int result = baseDialog->exec();
+  kDebug(7010) << "result:" << result;
+  if (result != KDialog::Accepted)
+  {
+    return false;
+  }
+
+  // Assume that service is an absolute path since we're being called (relative paths
+  // would have been allowed unless Kiosk said no, therefore we already know where the
+  // .desktop file is.  Now add a header to it if it doesn't already have one
+  // and add the +x bit.
+
+  if (!::makeFileExecutable( service.entryPath() ))
+  {
+    KMessageBox::sorry(
+      window,
+      i18n("Unable to make the service %1 executable, aborting execution", serviceName)
+    );
+
+    return false;
+  }
+
+  return true;
+}
+
 bool KRun::run( const KService& _service, const KUrl::List& _urls, QWidget* window,
     bool tempFiles, const QString& suggestedFileName, const QByteArray& asn )
 {
   if (!_service.entryPath().isEmpty() &&
-      !KDesktopFile::isAuthorizedDesktopFile( _service.entryPath()))
+      !KDesktopFile::isAuthorizedDesktopFile( _service.entryPath()) &&
+      !::makeServiceExecutable( _service, window ))
   {
-     kWarning() << "No authorization to execute " << _service.entryPath();
-     KMessageBox::sorry( window, i18n("You are not authorized to execute this service.") );
      return false;
   }
 

["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread] 

Configure | About | News | Add a list | Sponsored by KoreLogic