From 7addc67cd3f6a4c0358f70b4fb9906ebcd88b9e3 Mon Sep 17 00:00:00 2001 From: Simeon Simeonov Date: Sun, 15 Apr 2018 15:40:25 +0200 Subject: Add support for auto-unloading of the master password --- libsylph/masterpassword.c | 37 ++++++++++++++++--------------------- libsylph/masterpassword.h | 5 +++++ libsylph/ssl.c | 20 ++++++++++---------- libsylph/ssl.h | 6 +++--- src/main.c | 14 ++++++++++++-- src/prefs_ui.c | 27 ++++++++++++++++----------- 6 files changed, 62 insertions(+), 47 deletions(-) diff --git a/libsylph/masterpassword.c b/libsylph/masterpassword.c index 8174def..bac49c4 100644 --- a/libsylph/masterpassword.c +++ b/libsylph/masterpassword.c @@ -9,7 +9,7 @@ * * This library is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * Lesser General Public License for more details. * * You should have received a copy of the GNU Lesser General Public @@ -30,6 +30,7 @@ gchar *master_password; +gboolean master_password_enabled_on_init; void set_master_password(const char *password) { master_password = password; @@ -91,7 +92,7 @@ gchar *decrypt_with_master_password(const gchar *str) { if (master_password == NULL) { /* we have empty or auto unloaded master password */ if ((!prefs_common.auto_unload_master_password) || - (check_master_password_interactively(3) != RC_OK)) { + (check_master_password_interactively(3) != MP_RC_OK)) { return g_strdup(str); } debug_print("Reloaded master password\n"); @@ -104,7 +105,7 @@ gchar *decrypt_with_master_password(const gchar *str) { if (decrypt_data(&new_str, str + str_prefix, master_password, - strlen(str) - str_prefix) != RC_OK) { + strlen(str) - str_prefix) != MP_RC_OK) { OPENSSL_cleanse(new_str, strlen(new_str)); g_free(new_str); return g_strdup(str); @@ -123,17 +124,13 @@ gchar *encrypt_with_master_password(const gchar *str) { gchar *new_str, *mpes1_str; gint length_encrypted; - if ((!str) || (!prefs_common.use_master_password)) - return g_strdup(str); + if ((!str) || (!master_password_active())) + return NULL; - if (master_password == NULL) { - /* we have empty or auto unloaded master password */ - if ((!prefs_common.auto_unload_master_password) || - (check_master_password_interactively(3) != RC_OK)) { - return g_strdup(str); - } - debug_print("Reloaded master password\n"); - } + /* + * unlike the decrypt function, here it is up to the caller + * to make sure that auto unloaded master password is handled properly + */ if (encrypt_data(&new_str, &length_encrypted, @@ -141,19 +138,17 @@ gchar *encrypt_with_master_password(const gchar *str) { master_password, strlen(str), prefs_common.encrypted_password_min_length, - TRUE) != RC_OK) { - OPENSSL_cleanse(new_str, strlen(new_str)); + TRUE) != MP_RC_OK) { g_free(new_str); - return g_strdup(str); + return NULL; } mpes1_str = g_strdup_printf("mpes1:%s", new_str); - OPENSSL_cleanse(new_str, strlen(new_str)); g_free(new_str); return mpes1_str; #else - return g_strdup(str); + return NULL; #endif } @@ -170,7 +165,7 @@ gint set_master_password_interactively(guint max_attempts) { if (generate_password_hash( &prefs_common.master_password_hash, master_password, - NULL) != RC_OK) { + NULL) != MP_RC_OK) { /* should not really happen unless buggy code / library */ g_free(prefs_common.master_password_hash); prefs_common.master_password_hash = NULL; @@ -204,8 +199,8 @@ gint check_master_password_interactively(guint max_attempts) { continue; } if (check_password(master_password, - prefs_common.master_password_hash) == RC_OK) { - return RC_OK; /* match */ + prefs_common.master_password_hash) == MP_RC_OK) { + return MP_RC_OK; /* match */ } debug_print(_("Wrong master password entered (%d)\n"), cnt); OPENSSL_cleanse(master_password, strlen(master_password)); diff --git a/libsylph/masterpassword.h b/libsylph/masterpassword.h index cc32358..7254643 100644 --- a/libsylph/masterpassword.h +++ b/libsylph/masterpassword.h @@ -24,7 +24,12 @@ #include "config.h" #endif +#define MP_RC_OK 0 +#define MP_RC_WRONG_HASH_OR_KEY 1 +#define MP_RC_INVALID_FORMAT 2 /* invalid digest format */ + extern gchar *master_password; +extern gboolean master_password_enabled_on_init; /* m.p. enabled on init? */ void set_master_password(const char *password); gchar *get_master_password(void); void cleanse_buffer(void *buf, size_t len); diff --git a/libsylph/ssl.c b/libsylph/ssl.c index 44b2935..e782b0e 100644 --- a/libsylph/ssl.c +++ b/libsylph/ssl.c @@ -446,7 +446,7 @@ static gint secure_derive_key(guchar *key, OPENSSL_free(buffer); EVP_MD_CTX_destroy(mdctx); - return RC_OK; + return SSL_RC_OK; } @@ -472,7 +472,7 @@ gint encrypt_data(gchar **encrypted, EVP_CIPHER_CTX *ctx; EVP_MD_CTX *mdctx; - rc = RC_ERROR; + rc = SSL_RC_ERROR; if (length_data < 1) { return -1; @@ -492,7 +492,7 @@ gint encrypt_data(gchar **encrypted, if (secure_derive_key(key, key_size, passphrase, - salt) != RC_OK) { + salt) != SSL_RC_OK) { OPENSSL_cleanse(key, key_size); debug_print("Could not generate secure key\n"); goto cleanup; @@ -594,7 +594,7 @@ gint encrypt_data(gchar **encrypted, *encrypted = g_base64_encode(total_buffer, length_total); *length_encrypted = strlen(*encrypted); - rc = RC_OK; + rc = SSL_RC_OK; cleanup: /* key */ @@ -740,7 +740,7 @@ gint decrypt_data(gchar **decrypted, (const gchar*) cleartext_buffer, length_hash) != 0) { debug_print("Invalid hash\n"); - rc = RC_WRONG_HASH_OR_KEY; + rc = SSL_RC_WRONG_HASH_OR_KEY; goto cleanup; } @@ -755,7 +755,7 @@ gint decrypt_data(gchar **decrypted, *decrypted = OPENSSL_malloc(length_decrypted); memcpy(*decrypted, data_payload_buffer + 2, length_decrypted); - rc = RC_OK; + rc = SSL_RC_OK; cleanup: @@ -793,7 +793,7 @@ gint generate_password_hash(gchar **password_hash, if (salt == NULL) { if (RAND_bytes(lsalt, SALT_SIZE) != 1) { debug_print("Random problems...\n"); - return RC_ERROR; + return SSL_RC_ERROR; } } else { memcpy(lsalt, salt, SALT_SIZE); @@ -827,7 +827,7 @@ gint generate_password_hash(gchar **password_hash, OPENSSL_cleanse(salt_b64, strlen(salt_b64)); OPENSSL_free(salt_b64); - return RC_OK; + return SSL_RC_OK; } @@ -838,7 +838,7 @@ gint check_password(const gchar *password, const gchar *password_hash) { gchar **tokens, *new_hash; gsize salt_length; - rc = RC_ERROR; + rc = SSL_RC_ERROR; tokens = g_strsplit(password_hash, "$", -1); @@ -858,7 +858,7 @@ gint check_password(const gchar *password, const gchar *password_hash) { goto cleanup; } - if (generate_password_hash(&new_hash, password, salt) != RC_OK) { + if (generate_password_hash(&new_hash, password, salt) != SSL_RC_OK) { debug_print("Password hash generation failed\n"); goto cleanup; } diff --git a/libsylph/ssl.h b/libsylph/ssl.h index 6338911..4c0ccd1 100644 --- a/libsylph/ssl.h +++ b/libsylph/ssl.h @@ -37,9 +37,9 @@ #include "socket.h" -#define RC_OK 0 -#define RC_ERROR -1 -#define RC_WRONG_HASH_OR_KEY 1 +#define SSL_RC_OK 0 +#define SSL_RC_ERROR -1 +#define SSL_RC_WRONG_HASH_OR_KEY 1 typedef enum { SSL_METHOD_SSLv23, diff --git a/src/main.c b/src/main.c index 3c87c70..97a4bd0 100644 --- a/src/main.c +++ b/src/main.c @@ -280,7 +280,7 @@ int main(int argc, char *argv[]) #if USE_SSL if (prefs_common.use_master_password) { if (prefs_common.master_password_hash != NULL) { - if (check_master_password_interactively(3) != RC_OK) { + if (check_master_password_interactively(3) != MP_RC_OK) { if (alertpanel(_("Master password"), _("Invalid master password"), GTK_STOCK_DISCARD, @@ -292,7 +292,7 @@ int main(int argc, char *argv[]) } else { alertpanel_notice( _("Master password enabled but not set. Setting one now")); - if (set_master_password_interactively(3) != RC_OK) { + if (set_master_password_interactively(3) != MP_RC_OK) { if (alertpanel(_("Master password"), _("Unable to set master password"), GTK_STOCK_DISCARD, @@ -303,6 +303,16 @@ int main(int argc, char *argv[]) } } } + /* security goal: + * if the master password is enabled and loaded on init, + * an attacker can potentially set use_master_password - disabled + * afterwards and then force Sylpheed to save account data - the result + * being passwords saved to accountrc in plain-text. + * possible solution: + * Do not allow the passwords to be stored in plain-text if Sylpheed + * was started with use_master_password - enabled. + */ + master_password_enabled_on_init = prefs_common.use_master_password; #endif filter_set_addressbook_func(addressbook_has_address); filter_read_config(); diff --git a/src/prefs_ui.c b/src/prefs_ui.c index 051a058..2ab1d45 100644 --- a/src/prefs_ui.c +++ b/src/prefs_ui.c @@ -279,23 +279,28 @@ void prefs_set_data_from_epass_entry(PrefParam *pparam) /* This is where decrypted passwords are encrypted and stored again */ - /* TODO: A potential exploit is disabling master password and then forcing - * Sylpheed to store the password - */ - /* master_password == NULL for any of the following reasons: * - use_master_password is not enabled (we just save without encrypting) - * - master_password is auto unloaded (let the encrypt function handle it) + * - master_password is auto unloaded (prompt for the master password) * - undefined reason (refure to store the password) */ if (!prefs_common.use_master_password) { - prefs_set_data_from_entry(pparam); - return; - } else if ((master_password == NULL) && - (!prefs_common.auto_unload_master_password)) { - debug_print("Master password enabled, but not loaded for no " - "apparent reason. Not storing\n"); + /* check if use_master_password was enabled when Sylpheed started */ + if (!master_password_enabled_on_init) { + prefs_set_data_from_entry(pparam); + } return; + } else if (master_password == NULL) { + if (!prefs_common.auto_unload_master_password) { + debug_print("Master password enabled, but not loaded for no " + "apparent reason. Not storing\n"); + return; + } else { + if (check_master_password_interactively(3) != MP_RC_OK) { + debug_print("Failed to reload the master password\n"); + return; + } + } } ui_data = (PrefsUIData *)pparam->ui_data; -- cgit v1.3