First work on .desktop file brouhaha [PATCH]

[Michael Pyne]

I've done a bit of working on making the .desktop files less likely to cause 
grief for unwitting KDE users (as mentioned on LWN.net and Slashdot).

The following patch (also attached) is my first attempt at implementing a 
modicum of security while also allowing most .desktop files to work as is.  
I'll go ahead and include it inline here and then afterwards include my 
discussion:

Index: kinit/klauncher.cpp
===================================================================
--- kinit/klauncher.cpp	(revision 927231)
+++ kinit/klauncher.cpp	(working copy)
@@ -774,16 +774,50 @@
    return start_service(service, urls, envs, startup_id.toLocal8Bit(), blind, 
false, msg);
 }
 
+// helper function for start_service()
+static bool
+isStandardService(const char *type, const QString &servicePath)
+{
+   QStringList resourceDirs = KGlobal::dirs()->resourceDirs(type);
+   foreach(QString resourceDir, resourceDirs) {
+      if(resourceDir == servicePath)
+         return true;
+   }
+
+   return false;
+}
+
 bool
 KLauncher::start_service(KService::Ptr service, const QStringList &_urls,
                          const QStringList &envs, const QByteArray 
&startup_id,
                          bool blind, bool autoStart, const QDBusMessage &msg)
 {
    QStringList urls = _urls;
-   if (!service->isValid())
+   QString servicePath = service->entryPath();
+   bool runPermitted = !QDir::isAbsolutePath(servicePath) ||     // relative 
paths will look in standard dirs
+                        QFileInfo(servicePath).isExecutable() || // 
executable files are OK
+                       !QFileInfo(servicePath).isWritable();     // 
presumably so are files the user can't create
+
+   if (!runPermitted) { // Path was absolute, double-check it's in standard 
directories
+      QFileInfo serviceInfo(servicePath);
+      QString canonicalService = serviceInfo.canonicalPath(); // Work if 
.desktop file is found from a symlink
+
+      const char *const serviceTypes[] = {"apps", "services", "xdgdata-
apps"};
+      // sizeof stuff adapts to changes in array length
+      for(size_t i = 0; i < sizeof(serviceTypes) / sizeof(serviceTypes[0]); 
++i) {
+         if(::isStandardService(serviceTypes[i], canonicalService)) {
+            runPermitted = true;
+            break;
+         }
+      }
+   }
+
+   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());
       cancel_service_startup_info( NULL, startup_id, envs ); // cancel it if 
any
       return false;
    }

Discussion: The patch works by checking the .desktop file in 
KLauncher::start_service() (which seems to be called indirectly by basically 
any path in kdelibs to run a service).  The check consists of:

* Is the path to the file relative?  If so permit it for now as it will be 
searched for later (this is my assumption at least, I have not gone through 
the code paths to verify).
* If not, is the file executable? (I'm not sure if this needs Q_OS_WIN masking 
or anything however).
* If still not, could the user create the file?  If not allow it.

In the event of an absolute path further checks are conducted in order to 
verify that the path to the desktop file is one of the resource dirs used by 
KStandardDirs for the following types: "apps", "services", "xdgdata-apps". 
(Does this cover all the types that need to be searched?).  The path to the 
service is canonicalized so that a symlink to an installed service should 
still work (I didn't actually test. ;)

Assuming the check does not pass an error message is returned that the service 
must be executable (but the same ENOEXEC error code is returned as I feel 
that's accurate, do we need a different one?)

I have not implemented auto-executable-bit-ifying .desktop files (I think that 
would go into libkonq?)  Also, is there other code paths I need to look for in 
kdelibs that end up directly executing .desktop files (everything I've used so 
far has been blocked appropriately in my testing, kioclient, krunner, kickoff, 
and dolphin).

Standing by for comments (although it will be some hours before I'm online 
again most likely).

Regards,
 - Michael Pyne
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20090218/9942a310/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-desktop-file-security.patch
Type: text/x-patch
Size: 2342 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20090218/9942a310/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20090218/9942a310/attachment.sig>