[KDE Usability] Review Request: KAuth support in KDM kcontrol module

[Oswald Buddenhagen]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3631/#review5617
-----------------------------------------------------------


a symlink attack here means that the evil user passes the name of the symlink and replaces its destination between the permission check and actually opening the file (i.e., exploits the race condition). your latest patch closes this hole.

i thought some more about the security of the chosen approach in general ... we are allowing the user to replace the entire contents of kdmrc. this allows him to do anything from DoSing the system to making kdm call arbitrary commands for him. there are two approaches to that:
- go all paranoid and sanity-check every value in the installed config files, like ensuring that only root-owned non-symlinks are used for commands, etc.
- postulate that security is irrelevant here. after all, the user(s) permitted to execute these actions are most probably going to be real admins, so they can just "su root" and do anything anyway (hey, why didn't anybody think of that when i complained about security the first time? :D). i'd still leave the installation code in the current semi-secure state and document the big conceptual hole - in case somebody copies it.
i guess the latter approach makes perfect sense. if somebody comes up with a use case where it is not, one can still insert a content filter at a later point (one would copy the files to a temporary name, then open with kconfig, sanity-check and then rename to the final name).



/trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp
<http://reviewboard.kde.org/r/3631/#comment5315>

    this is a bit little for today. 100k or so would make sense, and only because we are copying small files.



/trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp
<http://reviewboard.kde.org/r/3631/#comment5316>

    why the nested absolutePath, and only in one of the calls?


- Oswald


On 2010-05-11 21:13:57, Igor Krivenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3631/
> -----------------------------------------------------------
> 
> (Updated 2010-05-11 21:13:57)
> 
> 
> Review request for kdelibs, usability, Dario Freddi, Oswald Buddenhagen, and Frederik Gladhorn.
> 
> 
> Summary
> -------
> 
> This is a patch for KDM kcontrol module.
> It implements Kauth support for changing kdm settings and managing user images. 
> It still doesn't support installation/deletion of themes (including installation via knewstuff).
> As far as I know there were plans to port KNewStuff3 to KAuth, so a more consistent approach is
> to rely on the KNewStuff's integration when it's available.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/CMakeLists.txt 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/background.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/background.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.h PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kcmkdm_actions.actions PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-conv.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-conv.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-dlg.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-dlg.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-gen.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-gen.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-shut.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-shut.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-theme.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-theme.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-users.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-users.cpp 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/main.h 1125102 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/main.cpp 1125102 
> 
> Diff: http://reviewboard.kde.org/r/3631/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Saving KDM settings
>   http://reviewboard.kde.org/r/3631/s/360/
> Managing user images
>   http://reviewboard.kde.org/r/3631/s/361/
> 
> 
> Thanks,
> 
> Igor
> 
>