From kde-core-devel Fri Feb 20 05:02:40 2009 From: Michael Pyne Date: Fri, 20 Feb 2009 05:02:40 +0000 To: kde-core-devel Subject: Update on progress [PATCH] Message-Id: <200902200002.45047.mpyne () purinchu ! net> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=123510623322932 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--nextPart1333478.rePLJuk6r9" --nextPart1333478.rePLJuk6r9 Content-Type: multipart/mixed; boundary="Boundary-02=_wljnJMR/9LvvDUW" Content-Transfer-Encoding: 7bit --Boundary-02=_wljnJMR/9LvvDUW Content-Type: multipart/alternative; boundary="Boundary-01=_wljnJvS53Hw06Zj" Content-Transfer-Encoding: 7bit Content-Disposition: inline --Boundary-01=_wljnJvS53Hw06Zj Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Wednesday 18 February 2009, Michael Pyne wrote: > Standing by for comments (although it will be some hours before I'm online > again most likely). So far I have the outstanding request to call i18n() only once in the event= of=20 an error which I still need to change. I've been working on KRun today (in kio) based on some discussion on IRC wi= th=20 dfaure as to where best to add the feature to make a file executable. My=20 current work is attached as 2 patches. It compiles but is untested as I ne= ed=20 to be awake in 6 hours for work. =46irst patch is against kdecore's KDesktopFile to make the=20 isAuthorizedDesktopFile static method reflect the criteria used in my first= =20 patch against klauncher in addition to Kiosk (which means I will probably=20 alter my first patch to simply call KDesktopFile::isAuthorizedDesktopFile),= =20 since this code is used by KRun and was already mostly identical. =20 KDesktopFile::isAuthorizedDesktopFile used to always allow desktop files on= =20 the user's Desktop, which is what we're trying to avoid now.... ;) The second patch (fix-desktop-file-security-2) adds the ability to make the= =20 file executable and continue automatically (if the reason execution was=20 forbidden wasn't due to Kiosk, that is). I'm using a KMessageBox with a=20 details button. I think the text is kind of clumsy but I'm very tired righ= t=20 now and can't think of anything better. :) Again, I haven't been able to test yet, if someone does make a KRun testcas= e=20 I'd appreciate screenshots so we can see what it look like, otherwise I wil= l=20 do so tomorrow when I get off of work and doing chores from my wife. ;) Still missing is actually tying all of this into the file view used by Konq= ,=20 Dolphin, and Folder View, which all still ends up in klauncher since I=20 apparently have not covered all the possibilities in KRun. But I'd appreci= ate=20 feedback on what I've got so far. Regards, - Michael Pyne --Boundary-01=_wljnJvS53Hw06Zj Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 7bit On Wednesday 18 February 2009, Michael Pyne wrote:
> Standing by for comments (although it will be some hours before I'm online
> again most likely).


So far I have the outstanding request to call i18n() only once in the event of an error which I still need to change.


I've been working on KRun today (in kio) based on some discussion on IRC with dfaure as to where best to add the feature to make a file executable. My current work is attached as 2 patches. It compiles but is untested as I need to be awake in 6 hours for work.


First patch is against kdecore's KDesktopFile to make the isAuthorizedDesktopFile static method reflect the criteria used in my first patch against klauncher in addition to Kiosk (which means I will probably alter my first patch to simply call KDesktopFile::isAuthorizedDesktopFile), since this code is used by KRun and was already mostly identical. KDesktopFile::isAuthorizedDesktopFile used to always allow desktop files on the user's Desktop, which is what we're trying to avoid now.... ;)


The second patch (fix-desktop-file-security-2) adds the ability to make the file executable and continue automatically (if the reason execution was forbidden wasn't due to Kiosk, that is). I'm using a KMessageBox with a details button. I think the text is kind of clumsy but I'm very tired right now and can't think of anything better. :)


Again, I haven't been able to test yet, if someone does make a KRun testcase I'd appreciate screenshots so we can see what it look like, otherwise I will do so tomorrow when I get off of work and doing chores from my wife. ;)


Still missing is actually tying all of this into the file view used by Konq, Dolphin, and Folder View, which all still ends up in klauncher since I apparently have not covered all the possibilities in KRun. But I'd appreciate feedback on what I've got so far.


Regards,
- Michael Pyne

--Boundary-01=_wljnJvS53Hw06Zj-- --Boundary-02=_wljnJMR/9LvvDUW Content-Type: text/x-patch; charset="UTF-8"; name="fix-desktop-file-security-2.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="fix-desktop-file-security-2.patch" Index: 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-- krun.cpp (revision 928198) +++ krun.cpp (working copy) @@ -44,6 +44,7 @@ #include "kfile/krecentdocument.h" #include "kdesktopfileactions.h" =20 +#include #include #include #include @@ -65,6 +66,9 @@ #include #include #include +#include +#include +#include =20 #ifdef Q_WS_X11 #include @@ -684,15 +688,83 @@ return urls; } =20 +// 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 i18n("Make service executable and continue"); + + QString warningMessage =3D i18n("You are trying to run the service %1, b= ut it is not " + "marked as an executable program. This may be a mistake in th= e system " + "configuration, but could also be an attempt at running a mali= cious program.\n\n" + "If you created this service then click %2 to mark the service= as executable. " + "If you downloaded this service or if you are unsure then you = should probably " + "cancel the service.", serviceName, continueStr); + + 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::NoDefault ); + + if (KMessageBox::createKMessageBox( + baseDialog, QMessageBox::Warning, warningMessage, QStringList(), = QString(), 0 /* bool* */, + 0 /* no options */, details ) =3D=3D KDialog::Cancel) + { + 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. + + QFile::Permissions perms =3D QFile::permissions( service.entryPath() ); + + // corresponds to owner on unix, which will have to do since if the user= isn't the owner + // we can't change perms anyways. + perms |=3D QFile::ExeUser; + + if (!QFile::setPermissions( service.entryPath(), perms )) + { + 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.") ); =2D return false; + return false; } =20 if ( !tempFiles ) --Boundary-02=_wljnJMR/9LvvDUW Content-Type: text/x-patch; charset="UTF-8"; name="fix-desktop-file-security-kdesktop-file.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="fix-desktop-file-security-kdesktop-file.patch" Index: 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-- config/kdesktopfile.cpp (revision 928198) +++ 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 + // not writable by the user. + QFileInfo entryInfo( path ); + if (entryInfo.isExecutable() || !entryInfo.isWritable()) + return true; + + kWarning() << "Access to '" << path << "' denied because of 'non_executa= ble_desktop_file' restriction." << endl; return false; } =20 --Boundary-02=_wljnJMR/9LvvDUW-- --nextPart1333478.rePLJuk6r9 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) iEYEABECAAYFAkmeOXQACgkQqjQYp5Omm0onpQCfVZOvkn8iobn+VWB+It+iGl8N WR0An1ZVHi/ONxj2uMdIFv6Vxe7RQhxx =7vsA -----END PGP SIGNATURE----- --nextPart1333478.rePLJuk6r9--