Bug 131516 - KMail forgets account setting for storing a POP3 password
Summary: KMail forgets account setting for storing a POP3 password
Status: RESOLVED FIXED
Alias: None
Product: kmail
Classification: Applications
Component: general (show other bugs)
Version: 1.9.1
Platform: Gentoo Packages Linux
: NOR normal
Target Milestone: ---
Assignee: kdepim bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-07-29 15:44 UTC by Christian Janoff
Modified: 2007-09-14 12:17 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Janoff 2006-07-29 15:44:30 UTC
Version:           1.9.1 (using KDE KDE 3.5.2)
Installed from:    Gentoo Packages
Compiler:          gcc-3.4.6 
OS:                Linux

(First of all: I do *not* use kwallet.)

To reproduce this bug do the following:

1. Add a new POP3 account (the account doesn't have to be
   a real existing account - that's not needed for this
   test) and select "Store POP password"in the
   configuration dialog. Click OK.

(1b. If kwallet pops up, cancel it, then confirm KMail's
     dialog box that it really should store the password
     in its own configuraton file instead of using kwallet.)

2. Check mail for the new account. A small popup window
   appears with an *un*checked checkbox. It should be
   checked since KMail already should know from the
   configuration dialog that it should store the password.

That's annoying because if I forget to check this box
KMail also turns off the configuration setting.

Simple solution: Remove the checkbox from the popup window
if "store password" is turned on in the configuration. If it's turned off there, I think it's no problem to keep it in the popup window.

Christian
Comment 1 Christian Janoff 2006-08-01 16:40:52 UTC
There's another problem related to this bug:

When I close KMail, it wants to store the password.

That's ok, I told KMail to do so.

But ... *everytime* when I close KMail, it prompts me again
if I want to install KWallet. I then cancel the KWallet
installation because I don't want to.

After that KMail *always* asks, if I really want KMail to store
the password in its own configuration file. I have to confirm that
after every single KMail session.

Christian
Comment 2 Christian Janoff 2006-08-07 18:13:19 UTC
If someone doesn't want to use kwallet at all, there's a workaround:

Security & Privacy -> KDE Wallet -> *DIS*able the KDE wallet subsystem.
Comment 3 Thomas McGuire 2007-06-10 22:26:36 UTC
The original bug is fixed now, not by removing the checkbox, but by setting the correct value to the checkbox.

Fixed by:

>SVN commit 673666 by tmcguire:

>Fix 2 issues with the password dialog:
>- The 'keep password' checkbox was always hidden
>- The 'keep password' checkbox wasn't initalized with the correct value

>Now KMail can save passwords again.
Comment 4 Thomas McGuire 2007-06-11 22:33:06 UTC
SVN commit 674135 by tmcguire:

More password storing fixes:
- Don't try to migrate to the wallet once the user has decided not to use it.
  Now KMail doesn't ask to migrate the password when exiting.
- If the user chooses not to store the password at all, honor that
  and don't ask him again to store it.


BUG: 95615
CCBUG: 131516

 M  +51 -31    networkaccount.cpp  


--- trunk/KDE/kdepim/kmail/networkaccount.cpp #674134:674135
@@ -120,6 +120,8 @@
     if( mStorePasswd != store && store )
       mPasswdDirty = true;
     mStorePasswd = store;
+    if ( !store )
+      mStorePasswdInConfig = false;
   }
 
   void NetworkAccount::setHost( const QString & host ) {
@@ -163,16 +165,8 @@
 
       if ( !encpasswd.isEmpty() ) {
         setPasswd( KStringHandler::obscure( encpasswd ), true );
-        // migrate to KWallet if available
-        if ( Wallet::isEnabled() ) {
-          config.deleteEntry( "pass" );
-          config.deleteEntry( "passwd" );
-          mPasswdDirty = true;
-          mStorePasswdInConfig = false;
-        } else {
-          mPasswdDirty = false; // set by setPasswd() on first read
-          mStorePasswdInConfig = true;
-        }
+        mPasswdDirty = false; // set by setPasswd() on first read
+        mStorePasswdInConfig = true;
       } else {
         // read password if wallet is already open, otherwise defer to on-demand loading
         if ( Wallet::isOpen( Wallet::NetworkWallet() ) )
@@ -199,37 +193,56 @@
   void NetworkAccount::writeConfig( KConfigGroup & config ) {
     KMAccount::writeConfig( config );
 
-    config.writeEntry( "login", login() );
-    config.writeEntry( "store-passwd", storePasswd() );
-
     if ( storePasswd() ) {
       // write password to the wallet if possible and necessary
       bool passwdStored = false;
-      Wallet *wallet = kmkernel->wallet();
-      if ( mPasswdDirty ) {
-        if ( wallet && wallet->writePassword( "account-" + QString::number(mId), passwd() ) == 0 ) {
+
+      //If the password should be written to the wallet, do that
+      if ( !mStorePasswdInConfig ) {
+        Wallet *wallet = kmkernel->wallet();
+
+        //If the password is dirty, try to store it in the wallet
+        if ( mPasswdDirty ) {
+          if ( wallet && wallet->writePassword( "account-" + QString::number(mId), passwd() ) == 0 )
+            passwdStored = true;
+        }
+
+        //If the password isn't dirty, it is already stored in the wallet.
+        else if ( wallet )
           passwdStored = true;
+
+        //If the password is stored in the wallet, it isn't dirty or stored in the config
+        if ( passwdStored ) {
           mPasswdDirty = false;
           mStorePasswdInConfig = false;
         }
-      } else {
-        passwdStored = wallet ? !mStorePasswdInConfig /*already in the wallet*/ : config.hasKey("pass");
       }
+      else
+        passwdStored = config.hasKey("pass");
+
       // if wallet is not available, write to config file, since the account
       // manager deletes this group, we need to write it always
-      if ( !passwdStored && ( mStorePasswdInConfig || KMessageBox::warningYesNo( 0,
-           i18n("KWallet is not available. It is strongly recommended to use "
-                "KWallet for managing your passwords.\n"
-                "However, KMail can store the password in its configuration "
-                "file instead. The password is stored in an obfuscated format, "
-                "but should not be considered secure from decryption efforts "
-                "if access to the configuration file is obtained.\n"
-                "Do you want to store the password for account '%1' in the "
-                "configuration file?", name() ),
-           i18n("KWallet Not Available"),
-           KGuiItem( i18n("Store Password") ),
-           KGuiItem( i18n("Do Not Store Password") ) )
-           == KMessageBox::Yes ) ) {
+      bool writeInConfigNow = !passwdStored && mStorePasswdInConfig;
+      if ( !passwdStored && !mStorePasswdInConfig ) {
+        int answer = KMessageBox::warningYesNo( 0,
+            i18n("KWallet is not available. It is strongly recommended to use "
+                 "KWallet for managing your passwords.\n"
+                 "However, KMail can store the password in its configuration "
+                 "file instead. The password is stored in an obfuscated format, "
+                 "but should not be considered secure from decryption efforts "
+                 "if access to the configuration file is obtained.\n"
+                 "Do you want to store the password for account '%1' in the "
+                 "configuration file?", name() ),
+            i18n("KWallet Not Available"),
+            KGuiItem( i18n("Store Password") ),
+            KGuiItem( i18n("Do Not Store Password") ) );
+        if (answer == KMessageBox::Yes)
+          writeInConfigNow = true;
+        if (answer == KMessageBox::No)
+          mStorePasswd = false;
+      }
+
+      if ( writeInConfigNow ) {
         config.writeEntry( "pass", KStringHandler::obscure( passwd() ) );
         mStorePasswdInConfig = true;
       }
@@ -243,6 +256,13 @@
         wallet->removeEntry( "account-" + QString::number(mId) );
     }
 
+    // delete password from config file if it is stored in the wallet or
+    // not stored at all
+    if ( !mStorePasswdInConfig )
+      config.deleteEntry( "pass" );
+
+    config.writeEntry( "store-passwd", storePasswd() );
+    config.writeEntry( "login", login() );
     config.writeEntry( "host", host() );
     config.writeEntry( "port", static_cast<unsigned int>( port() ) );
     config.writeEntry( "auth", auth() );
Comment 5 Thomas McGuire 2007-06-15 18:15:52 UTC
SVN commit 675974 by tmcguire:

Always try the wallet again when the user changes his password and has it stored in the config.
See the comment for the reasons.
CCBUGS: 95615,131516


 M  +11 -0     networkaccount.cpp  
 M  +1 -1      networkaccount.h  


--- trunk/KDE/kdepim/kmail/networkaccount.cpp #675973:675974
@@ -165,6 +165,7 @@
 
       if ( !encpasswd.isEmpty() ) {
         setPasswd( KStringHandler::obscure( encpasswd ), true );
+        mOldPassKey = encpasswd;
         mPasswdDirty = false; // set by setPasswd() on first read
         mStorePasswdInConfig = true;
       } else {
@@ -197,6 +198,15 @@
       // write password to the wallet if possible and necessary
       bool passwdStored = false;
 
+      //If the password is different from the one stored in the config,
+      //try to store the new password in the wallet again.
+      //This ensures a malicious user can't just write a dummy pass key in the
+      //config, which would get overwritten by the real password and therefore
+      //leak out of the more secure wallet.
+      if ( mStorePasswdInConfig &&
+           KStringHandler::obscure( mOldPassKey ) != passwd() )
+        mStorePasswdInConfig = false;
+
       //If the password should be written to the wallet, do that
       if ( !mStorePasswdInConfig ) {
         Wallet *wallet = kmkernel->wallet();
@@ -244,6 +254,7 @@
 
       if ( writeInConfigNow ) {
         config.writeEntry( "pass", KStringHandler::obscure( passwd() ) );
+        mOldPassKey = KStringHandler::obscure( passwd() );
         mStorePasswdInConfig = true;
       }
     }
--- trunk/KDE/kdepim/kmail/networkaccount.h #675973:675974
@@ -129,7 +129,7 @@
   protected:
     KMail::SieveConfig mSieveConfig;
     KIO::Slave * mSlave;
-    QString mLogin, mPasswd, mAuth, mHost;
+    QString mLogin, mPasswd, mAuth, mHost, mOldPassKey;
     unsigned short int mPort;
     bool mStorePasswd : 1;
     bool mUseSSL : 1;