<!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;">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).<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 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:<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>Index: kinit/klauncher.cpp<br>
===================================================================<br>
--- kinit/klauncher.cpp (revision 927231)<br>
+++ kinit/klauncher.cpp (working copy)<br>
@@ -774,16 +774,50 @@<br>
return start_service(service, urls, envs, startup_id.toLocal8Bit(), blind, false, msg);<br>
}<br>
<br>
+// helper function for start_service()<br>
+static bool<br>
+isStandardService(const char *type, const QString &servicePath)<br>
+{<br>
+ QStringList resourceDirs = KGlobal::dirs()->resourceDirs(type);<br>
+ foreach(QString resourceDir, resourceDirs) {<br>
+ if(resourceDir == servicePath)<br>
+ return true;<br>
+ }<br>
+<br>
+ return false;<br>
+}<br>
+<br>
bool<br>
KLauncher::start_service(KService::Ptr service, const QStringList &_urls,<br>
const QStringList &envs, const QByteArray &startup_id,<br>
bool blind, bool autoStart, const QDBusMessage &msg)<br>
{<br>
QStringList urls = _urls;<br>
- if (!service->isValid())<br>
+ QString servicePath = service->entryPath();<br>
+ bool runPermitted = !QDir::isAbsolutePath(servicePath) || // relative paths will look in standard dirs<br>
+ QFileInfo(servicePath).isExecutable() || // executable files are OK<br>
+ !QFileInfo(servicePath).isWritable(); // presumably so are files the user can't create<br>
+<br>
+ if (!runPermitted) { // Path was absolute, double-check it's in standard directories<br>
+ QFileInfo serviceInfo(servicePath);<br>
+ QString canonicalService = serviceInfo.canonicalPath(); // Work if .desktop file is found from a symlink<br>
+<br>
+ const char *const serviceTypes[] = {"apps", "services", "xdgdata-apps"};<br>
+ // sizeof stuff adapts to changes in array length<br>
+ for(size_t i = 0; i < sizeof(serviceTypes) / sizeof(serviceTypes[0]); ++i) {<br>
+ if(::isStandardService(serviceTypes[i], canonicalService)) {<br>
+ runPermitted = true;<br>
+ break;<br>
+ }<br>
+ }<br>
+ }<br>
+<br>
+ if (!service->isValid() || !runPermitted)<br>
{<br>
requestResult.result = ENOEXEC;<br>
requestResult.error = i18n("Service '%1' is malformatted.", service->entryPath());<br>
+ if (service->isValid())<br>
+ requestResult.error = i18n("Service '%1' must be executable to run.", service->entryPath());<br>
cancel_service_startup_info( NULL, startup_id, envs ); // cancel it if any<br>
return false;<br>
}<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>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:<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 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).<br>
* If not, is the file executable? (I'm not sure if this needs Q_OS_WIN masking or anything however).<br>
* If still not, could the user create the file? If not allow it.<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>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. ;)<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 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?)<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 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).<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>Standing by for comments (although it will be some hours before I'm online again most likely).<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</p></body></html>