Bug 388

Some people will want the check box restored and some won't. Neither group should suffer. Any solution provided to resolve this issue probably should provide a build option to enable/disable showing the password check box. Or possibly a global configuration option in a kcfg file.

Created attachment 62 [details]
KDE 3.5.10 kdesu dialog box in Slackware 12.2

Traditionally, the Slackware developers seldom modify the KDE sources from upstream expect for security issues and obvious bugs. The attached picture shows an unmodified kdesu dialog box from KDE 3.5.10 in Slackware 12.2.

Notice the check box to remember the password.

As stated in a previous comment, the password is not permanent, is session only, and lasts only for a specified period (times out).

Distros promoting sudo are likely to disable that check box but that should not be a blanket policy in Trinity. Whether to use that check box should be up to packagers.

This bug still exists in 3.5.13.

I have copies of 3.5.10 sources to compare to TDE.

Created attachment 193 [details]
Restores the "Keep password" check box to the kdesu dialog box

This one-liner patch restores the visibility of the 'Keep password' check box to the kdesu dialog box. In TDE that check box is hard-coded to be invisible by default, which is the opposite of expected behavior from the vanilla 3.5.10 that many users are accustomed.

If this patch is contrary to how some want to control that check box, then we need to have a healthy discussion about how to please both camps. :)

Otherwise this bug report can be closed.

Perhaps the code set has changed in TDE from 3.5.10. If I understand correctly, in 3.5.10 kdesu worked like this:

* The password is not recorded in any config file anywhere. The password is maintained in memory.

* The password is good only for that session. Exiting TDE kills all kdesu hooks. If running with a graphical login that would be exiting TDE to the login manager. If running X from the command line that would be exiting X back to the command line. Logging out at the command line is not necessary --- only exiting the TDE session.

* If the check box is not enabled, the password is forgotten immediately. This is easily tested by closing the app and immediately running kdesu to run the same app. The password is again required.

* If the check box is enabled, the password is remembered only for the period set in $PREFIX/include/kdesu/defaults.h and ONLY for that app. I think the default is 2 hours. This is testable by waiting for the default time out period and then again launching kdesu for that app. A password is again required.

* If kdesu is launched for a different app, then repeat the previous steps. Passwords stored in memory for one app do not grant access to other apps.

Is the check box a potential security risk? Sure, the time out period does pose a potential risk. Even if an admin closes a kdesu app, another person could restart the app if the timeout period has not expired. A safe policy is to not enable the check box in those environments or to use the TDE screen lockdown. Some people might then argue to just eliminate the check box, but not everybody works in that environment. The check box should be available to those who want the feature. For example, I work at home and have no cats to walk on my keyboard. :)

The administrator's section of KControl would be a good location to configure whether to show the check box. From the command line or konsole, running kdesu -n will open the dialog box without the check box. Disabling the check box through KControl would have the same effect of setting the kdesu default to use the -n parameter. The setting could be saved in a kdesurc config file in /etc/trinity. Traditionally only admins have change permissions to /etc.

Perhaps there could be a default kdesurc in /usr/share/config, but the code looks in $SYSCONFDIR too for an admin system setting override.

The point is some people want this feature and some don't. The only solution is to provide a mechanism to satisfy both crowds. Upstream developers should not decide --- let users decide. :)

The patch provided in bug report 755 allows kdebase to build and in Slackware 13.1, solves the problem reported in bug report 634 by enabling HAL support.

However, building with HAL support neutralizes the patch submitted in this bug report, which restores the 'Keep password' check box to the kdesu dialog box.

Please do not close this report or that of 755 unless both issues are resolved.

Never mind the clash between HAL support and this patch. I found the problem.

In my build script inadvertently I changed package file ownerships to root:root. That change modified the $PREFIX/bin/kdesud binary from 2755 and root:nogroup to 0755 root:root.

The actual "damage" is caused by changing permissions from 2755 to 0755. Changing the group has no noticeable effect. I can toggle the permissions between 2755 and 0755 and see the effect immediately regardless of the group. Apparently kdesud must run set GID.

Setting kdesud to other than 2755 triggers not showing the 'Keep password' check box.

Proposed long-term solution

The affected application is already stated in the dialog box. However, the text is not obvious and tends to blend with the box. Perhaps the app information should be bolded or be more verbose. For example, use KRunner to type kdesu kate and this is what appears:

"Command: kate"

Perhaps that can be changed to:

"Command: Run kate as root"

Additionally, cCurrently the kdesu dialog looks like this:

=======================================
Run as root - KDE su
=======================================
The action you requested needs root
privileges. Please enter root's
password below or click Ignore to
continue with your current privileges.

Command: kate
Password: _________________

|_| Keep password

         Ignore    OK     Cancel
=======================================

Perhaps underneath the 'Keep password' check box widget, when the user enables the check box, the following text appears dynamically:

"Keeping the password is good only for X hours, only for kate, and only for this session."

The duration X is fetched from defaults.h.

If the user does not enable the check box the text does not appear. By appearing dynamically, any potential security risk of using this option becomes more obvious. Or the warning text could be ghosted when the check box is disabled and unghosted
when enabled.

This should be resolved in GIT hash 8d7bb28.

Thanks for reporting!

Oops!

I could not build. I received a message about setKeepWarning not being declared.

I was able to build with this patch:

diff -urN tdebase/tdesu/tdesu/sudlg.cpp tdebase.new/tdesu/tdesu/sudlg.cpp
--- tdebase/tdesu/tdesu/sudlg.cpp	2012-06-09 21:38:04.000000000 -0500
+++ tdebase.new/tdesu/tdesu/sudlg.cpp	2012-06-09 22:46:13.000000000 -0500
@@ -43,7 +43,7 @@
 	}
     }
     setPrompt(prompt);
-    setKeepWarning(i18n("<qt>The stored password will be:<br> * Kept for up to %1 hours<br> * Destroyed on logout").arg(timeout));
+    TQString setKeepWarning(i18n("<qt>The stored password will be:<br> * Kept for up to %1 hours<br> * Destroyed on logout").arg(timeout));

     if( withIgnoreButton )
 	setButtonText(User1, i18n("&Ignore"));

If the patch makes sense then I'll push.

(In reply to comment #11)
> Oops!
> 
> I could not build. I received a message about setKeepWarning not being
> declared.

Sorry, I forgot to mention: rebuild tdelibs. ;-)

Okay, I see: GIT hash 3c752316.

(Odpověď na komentář #12)
> Sorry, I forgot to mention: rebuild tdelibs. ;-)

It does not interfere with anything else, just tdelibs + kdebase? I could backport it to v3.5.13.1. So I'm watching, to not break ABI.

Rebuilding now....

Note:

In 3.5.10 and previous, the dialog always defaulted with the "Keep password" check box disabled/unchecked. In TDE the check box defaults to enabled/checked.

Although wonderfully convenient to somebody like me who uses the dialog many times during the day, I recall our long-ago conversations in the mail list regarding security concerns. In that spirit should the check box default to disabled/unchecked?

There was a patch from the OpenSuse folks from one of the early BIG patch sets merged into Trinity that creates a profile tdesurc file. The config file looks like this:

[Passwords]
Keep=true

The key value controls the check box, but does not work like the old 3.5.10 days. When Keep=true then the check box is always enabled/checked, regardless of which app is being opened. The opposite for Keep=false.

In 3.5.10 the check box always defaulted to disabled/unchecked for each new app that is opened through (k)desu. Continually reopening that same app did not prompt the dialog until after the timeout expired. Then the dialog appears again for that app, and the check box was again disabled/unchecked. To me that seems like the appropriate approach.

I tried reversing a small snippet in the original patch:

diff -urN tdebase/tdesu/tdesu/tdesu.cpp tdebase.new/tdesu/tdesu/tdesu.cpp
--- tdebase/tdesu/tdesu/tdesu.cpp 2012-03-07 17:52:05.000000000 -0600
+++ tdebase.new/tdesu/tdesu/tdesu.cpp 2012-03-21 14:10:10.000000000 -0500
@@ -382,7 +382,6 @@
             change_uid = false;
         password = dlg.password();
         keep = dlg.keep();
-        KConfigGroup(config,"Passwords").writeEntry("Keep", keep);
         data.setSilent( KStartupInfoData::No );
         KStartupInfo::sendChange( id, data );
     }

However, that small patch does not change the check box, which remains defaulted to enabled/checked.

The default check box status is fixed in GIT hash 87363770.

Tim,

I'm reopening this bug report.

The dynamic message is nice!

The dynamic text says the password will be stored up to 7200 hours. Whew. That is 300 days. :-)

As displayed, that message conflicts with tdelibs/tdesu/defaults.h. That is where the timeout is defined, which is prefixed to 2 hours (const int defTimeout = 120*60;). The 2 hour timeout is a security feature.

That integer constant is defined in seconds. Thus, the dynamic message should say 7200 seconds --- if you are grabbing that information from defaults.h.

Having the dynamic message say 2 hours (or 120 minutes) is more human readable and would match defaults.h. I can patch the message text to "seconds," but I don't know how in C++ to convert the seconds to hours while ensuring conversion integrity. I also don't know whether your recent patch is using the defTimeout constant.

Hopefully this is an easy fix!

(In reply to comment #17)
> Tim,
> 
> I'm reopening this bug report.
> 
> The dynamic message is nice!
> 
> The dynamic text says the password will be stored up to 7200 hours. Whew. That
> is 300 days. :-)

Oops!  Should be fixed in GIT hash 8ee0ab9. :-)

Tim

I see you opted for minutes. I suppose we could have divided by 3600 to obtain hours, but that would break (integer division) if somebody locally patched defaults.h to a number less than 3600 seconds/one hour.

Rebuilt. Tested. Thank you!

One more topic of concern (I realize Ubuntu does not use this dialog like other distros and thus you likely are unaware of this behavior). I don't want to make proverbial mountains out of molehills. Just let me know what you think is the saner approach to security. :-)

I mentioned in Comment 15, a snippet from a massive OpenSuse patch merged in the early Trinity days changed the behavior of the tdesu dialog check box.

As is, when launching tdesu for the first time, say to run kate as root, the check box is disabled/unchecked. For the next two hours the user will not see the dialog whenever launching kate with tdesu. After launching kate, the first time the user launches konqueror file manager as root through tdesu, the dialog will appear with the check box automatically enabled/checked --- because of the user's profile tdesurc being updated from the first usage. Similarly, after the two hour timeout expires, running kate again through tdesu will see a dialog with the check box enabled/checked.

When starting a new empty session, the key value in tdesurc will always enable/check the check box. The only way to get the dialog to show a disabled/unchecked check box is to manually uncheck the box. That then becomes the behavior the next time tdesu dialog is launched. Restarting the session destroys the password retention, but the check box is always enabled.

Is this overall behavior safe with respect to security? That is the remaining question. This new behavior is convenient to power users or isolated home users with less concern about security, but we won't mind seeing Trinity used in enterprise environments, where security fundamentals are a concern.

We don't want to make using tdesu irritating, but a constant default of a disabled/unchecked box seems conservative and a subtle promotion of nominal security.

To appease both types of users probably requires a global tdesurc that can be edited manually or through a kcontrol administrative mode control.

I can restore the original 3.5.10 behavior with the simple one-line patch shown in Comment 15, repeated here:

diff -urN tdebase/tdesu/tdesu/tdesu.cpp tdebase.new/tdesu/tdesu/tdesu.cpp
--- tdebase/tdesu/tdesu/tdesu.cpp 2012-03-07 17:52:05.000000000 -0600
+++ tdebase.new/tdesu/tdesu/tdesu.cpp 2012-03-21 14:10:10.000000000 -0500
@@ -382,7 +382,6 @@
             change_uid = false;
         password = dlg.password();
         keep = dlg.keep();
-        KConfigGroup(config,"Passwords").writeEntry("Keep", keep);
         data.setSilent( KStartupInfoData::No );
         KStartupInfo::sendChange( id, data );
     }

I can  and have run that patch in my builds and verify the 3.5.10 behavior is restored. Should the patch be pushed to GIT? My patch reverses that one line from the OpenSuse patch.

The original 3.5.10 supported a tdesurc config file, as you saw when patching for the dynamic message. Yet the tdesurc config file never was really used anywhere. The OpenSuse patch that added this one-liner makes use of the tdesurc file, but changes the behavior of the check box.

(In reply to comment #20)
> One more topic of concern (I realize Ubuntu does not use this dialog like other
> distros and thus you likely are unaware of this behavior). I don't want to make
> proverbial mountains out of molehills. Just let me know what you think is the
> saner approach to security. :-)
> 
> I mentioned in Comment 15, a snippet from a massive OpenSuse patch merged in
> the early Trinity days changed the behavior of the tdesu dialog check box.
> 
> As is, when launching tdesu for the first time, say to run kate as root, the
> check box is disabled/unchecked. For the next two hours the user will not see
> the dialog whenever launching kate with tdesu. After launching kate, the first
> time the user launches konqueror file manager as root through tdesu, the dialog
> will appear with the check box automatically enabled/checked --- because of the
> user's profile tdesurc being updated from the first usage. Similarly, after the
> two hour timeout expires, running kate again through tdesu will see a dialog
> with the check box enabled/checked.
> 
> When starting a new empty session, the key value in tdesurc will always
> enable/check the check box. The only way to get the dialog to show a
> disabled/unchecked check box is to manually uncheck the box. That then becomes
> the behavior the next time tdesu dialog is launched. Restarting the session
> destroys the password retention, but the check box is always enabled.
> 
> Is this overall behavior safe with respect to security? That is the remaining
> question. This new behavior is convenient to power users or isolated home users
> with less concern about security, but we won't mind seeing Trinity used in
> enterprise environments, where security fundamentals are a concern.
> 
> We don't want to make using tdesu irritating, but a constant default of a
> disabled/unchecked box seems conservative and a subtle promotion of nominal
> security.

I don't see any real issue with the current behaviour, as the 'sudo' executable on the command line also retains the password for some amount of time.  Additionally, TDE does have a general policy of trying to remember what the user has selected in the past (sort of a "smart" default setting) to minimise time spent clicking/typing in the interface; the proposed change would create a rather obvious break in this policy.

Okay, we live with the current behavior.