--nextPart1298435.hEhAbQ6RzU Content-Type: multipart/mixed; boundary="Boundary-02=_mL5nJMU/1JWRV5M" Content-Transfer-Encoding: 7bit --Boundary-02=_mL5nJMU/1JWRV5M Content-Type: multipart/alternative; boundary="Boundary-01=_mL5nJs9MTRCagbp" Content-Transfer-Encoding: 7bit Content-Disposition: inline --Boundary-01=_mL5nJs9MTRCagbp Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi all, I've implemented the auto-exec-bit-ifying of .desktop files in KRun and mad= e=20 the changes suggested in the last couple of threads, including using owned-= by- root as an exception as originally discussed instead of merely "not writabl= e=20 by user". Currently the work is in 3 patches (all attached): The first patch changes KDesktopFile::isAuthorizedDesktopFile() to include = the=20 extra restrictions that we're placing on .desktop files. This should be=20 applied first as the remaining patches both require this change. The second path implements security by not allowing klauncher to launch a=20 =2Edesktop file that doesn't meet the criteria given by=20 KDesktopFile::isAuthorizedDesktopFile() (a fairly large change from my firs= t=20 submission but all that's happened is the logic is defined in KDesktopFile= =20 instead of being duplicated) The third patch is against KRun, and implements the auto +x. This _needs=20 review_, it was basically all written today after I got home from work (whi= ch=20 included running 5K in shorts while it was 1 C outside so it was a rough da= y=20 ;) The idea is pop up a nice dialog [1] giving the user a readable description= of=20 what the problem is. A Details button is supposed to be included which=20 contains the Exec=3D line but isn't working for some reason. Also the amou= nt of=20 text buys us into the dreaded Qt layout vs. X11 bug which I've tried to=20 minimize the effects of. On that note, I'm game to better ways to phrase t= his=20 dialog, it doesn't seem efficient somehow. Assuming the user clicks on continue the file is made executable by doing 2= =20 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=20 =2Edesktop file contents below it. I'm not really happy about manually mov= ing=20 bytes around (especially as v1 in my testing today had an infinite loop) bu= t I=20 don't trust readAll()/write() for library code. Please look at this to mak= e=20 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 immediat= ely=20 launched. Is there anything I'm missing here now? Please let me know, otherwise I'd= =20 like to know if there are objections to committing on Sunday. Regards, - Michael Pyne [1] http://purinchu.net/dumping-ground/krun2.png --Boundary-01=_mL5nJs9MTRCagbp Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 7bit 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

--Boundary-01=_mL5nJs9MTRCagbp-- --Boundary-02=_mL5nJMU/1JWRV5M Content-Type: text/x-patch; charset="UTF-8"; name="brouhaha-001-kdesktopfile.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="brouhaha-001-kdesktopfile.patch" Index: kdecore/config/kdesktopfile.cpp =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =2D-- kdecore/config/kdesktopfile.cpp (revision 928782) +++ kdecore/config/kdesktopfile.cpp (working copy) @@ -24,6 +24,7 @@ #include =20 #include +#include =20 #include #include "kconfig_p.h" @@ -139,8 +140,13 @@ =20 bool KDesktopFile::isAuthorizedDesktopFile(const QString& path) { =2D if (KAuthorized::authorize("run_desktop_files")) =2D 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_desk= top_files' restriction." << endl; + return false; + } =20 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; =2D if (dirs->relativeLocation("data", path).startsWith("kdesktop/Desktop"= )) =2D return true; =20 =2D kWarning() << "Access to '" << path << "' denied because of 'run_deskt= op_files' restriction." << endl; + // Not otherwise permitted, so only allow if the file is executable, or = if + // owned by root (uid =3D=3D 0) + QFileInfo entryInfo( path ); + if (entryInfo.isExecutable() || entryInfo.ownerId() =3D=3D 0) + return true; + + kWarning() << "Access to '" << path << "' denied because of 'non_executa= ble_desktop_file' restriction." << endl; return false; } =20 --Boundary-02=_mL5nJMU/1JWRV5M Content-Type: text/x-patch; charset="UTF-8"; name="brouhaha-002-klauncher.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="brouhaha-002-klauncher.patch" Index: kinit/klauncher.cpp =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =2D-- kinit/klauncher.cpp (revision 928782) +++ kinit/klauncher.cpp (working copy) @@ -49,6 +49,7 @@ #include #include #include +#include #include =20 #include @@ -780,10 +781,15 @@ bool blind, bool autoStart, const QDBusMessage &m= sg) { QStringList urls =3D _urls; =2D if (!service->isValid()) + bool runPermitted =3D KDesktopFile::isAuthorizedDesktopFile(service->en= tryPath()); + + if (!service->isValid() || !runPermitted) { requestResult.result =3D ENOEXEC; =2D requestResult.error =3D i18n("Service '%1' is malformatted.", serv= ice->entryPath()); + if (service->isValid()) + requestResult.error =3D i18n("Service '%1' must be executable to = run.", service->entryPath()); + else + requestResult.error =3D i18n("Service '%1' is malformatted.", ser= vice->entryPath()); cancel_service_startup_info( NULL, startup_id, envs ); // cancel it = if any return false; } --Boundary-02=_mL5nJMU/1JWRV5M Content-Type: text/x-patch; charset="UTF-8"; name="brouhaha-003-krun-autoexec.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="brouhaha-003-krun-autoexec.patch" Index: kio/kio/krun.cpp =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =2D-- kio/kio/krun.cpp (revision 928782) +++ kio/kio/krun.cpp (working copy) @@ -44,6 +44,7 @@ #include "kfile/krecentdocument.h" #include "kdesktopfileactions.h" =20 +#include #include #include #include @@ -65,6 +66,10 @@ #include #include #include +#include +#include +#include +#include =20 #ifdef Q_WS_X11 #include @@ -684,14 +689,184 @@ return urls; } =20 +// 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.err= orString(); + return false; + } + + QByteArray header =3D desktopFile.peek( 2 ); // First two chars of file + if (header.size() =3D=3D 0) { + kError(7010) << "Error inspecting service" << fileName << desktopFile.= errorString(); + return false; // Some kind of error + } + + if (header !=3D "#!") + { + // 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 =3D=3D 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 << sa= veFile.errorString(); + saveFile.abort(); + return false; + } + + // Now copy the one into the other and then close and reopen desktopFi= le + const int bufSize =3D 1024; + char buffer[bufSize]; + qint64 numRead, numWritten; + + while(!desktopFile.atEnd()) + { + numRead =3D desktopFile.read( buffer, bufSize ); + if( numRead =3D=3D -1 ) + { + kError(7010) << "Unable to add header for" << fileName << desktopF= ile.errorString(); + saveFile.abort(); + return false; + } + + numWritten =3D 0; + while( numWritten < numRead ) + { + qint64 writtenThisTime =3D saveFile.write( &buffer[numWritten], nu= mRead - numWritten ); + if (writtenThisTime =3D=3D -1) + { + kError(7010) << "Error copying service" << fileName << saveFile.= errorString(); + saveFile.abort(); + return false; + } + numWritten +=3D 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 << desktopFil= e.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.permission= s() )) + { + kError(7010) << "Unable to change permissions for" << fileName << desk= topFile.errorString(); + return false; + } + + // whew + return true; +} + +// Helper function to make a .desktop file executable if prompted by the u= ser. +// returns true if KRun::run() should continue with execution, false if us= er declined +// to make the file executable or we failed to make it executable. +static bool makeServiceExecutable( const KService& service, QWidget* windo= w ) +{ + if (!KAuthorized::authorize("run_desktop_files")) + { + kWarning() << "No authorization to execute " << service.entryPath(); + KMessageBox::sorry( window, i18n("You are not authorized to execute th= is service.") ); + return false; // Don't circumvent the Kiosk + } + + QString serviceName =3D service.genericName(); + if (serviceName.isEmpty()) + serviceName =3D service.desktopEntryName(); + QString continueStr =3D i18nc("@action:button", + "Make service executable and continue"); + + QString warningMessage =3D i18nc("@info", + "You are trying to run the service %1= , 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 pr= ogram." + "If you created this service, then click %2 to mark the " + "service as executable and run it. " + "If you downloaded this service, it was emailed to you, or if yo= u are simply " + "unsure then you should Cancel.", + serviceName, continueStr); + + // TODO: Make this actually show up in KMessageBox + QString details =3D i18n("This service will run the following command: %= 1", service.exec() ); + + KGuiItem continueItem =3D KStandardGuiItem::cont(); + continueItem.setText( continueStr ); + + // We want to be able to provide the details window but only "sorry" mes= sage boxes have + // a static method so we need to use createKMessageBox, which means we n= eed to provide + // the KDialog. We'll change the Continue to have a more descriptive bu= tton but otherwise + // be the standard continue button. + + KDialog *baseDialog =3D new KDialog( window ); + baseDialog->setButtons( KDialog::Ok | KDialog::Cancel ); + baseDialog->setButtonGuiItem( KDialog::Ok, continueItem ); + baseDialog->setDefaultButton( KDialog::Cancel ); // NoDefault doesn't wo= rk? + baseDialog->setCaption( i18nc("Warning about executing unknown .desktop = file", "Warning") ); + + // We must use NoExec because otherwise the message box will be queued a= nd 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 =3D baseDialog->exec(); + kDebug(7010) << "result:" << result; + if (result !=3D KDialog::Accepted) + { + return false; + } + + // Assume that service is an absolute path since we're being called (rel= ative paths + // would have been allowed unless Kiosk said no, therefore we already kn= ow 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& as= n ) { if (!_service.entryPath().isEmpty() && =2D !KDesktopFile::isAuthorizedDesktopFile( _service.entryPath())) + !KDesktopFile::isAuthorizedDesktopFile( _service.entryPath()) && + !::makeServiceExecutable( _service, window )) { =2D kWarning() << "No authorization to execute " << _service.entryPath(= ); =2D KMessageBox::sorry( window, i18n("You are not authorized to execute= this service.") ); return false; } =20 --Boundary-02=_mL5nJMU/1JWRV5M-- --nextPart1298435.hEhAbQ6RzU Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (GNU/Linux) iEYEABECAAYFAkmfkuoACgkQqjQYp5Omm0qbhACg1niGTi6zDGJ0ULyYYvp0Bp0o ZEYAnjjc7Hoh8G45BRXDLOrEUvCJC8Mk =qfuo -----END PGP SIGNATURE----- --nextPart1298435.hEhAbQ6RzU--