GNOME Bugzilla – Bug 349132
Audioscrobbler plug-in stores user password in clear text
Last modified: 2011-10-15 22:02:31 UTC
Please describe the problem: The plug-in for updating Last-fm is configured through Rhythmbox's plug-in configuration GUI. The data entered there is stored in the configuration tree in /apps/rhythmbox/audioscrobbler. The entry that holds the user's password reveals the password in clear text. Steps to reproduce: 1. Enter a user name and password in the Edit->Plugins...->Last.fm Profile dialog 2. (Possibly not necessary:) Close Rhythmbox 3. Using the configuration editor, navigate to /apps/rhythmbox/audioscrobbler Actual results: I can see the user's password in clear text Expected results: Some form of encrypted data (md5, whatever) Does this happen every time? Yes Other information: This is not a critical bug, since the functionality of both the host application and the plug-in is intact. However, we (the community) should never store passwords as clear text.
The only form of encryption we could apply to the password would be to trivially and reversibly obscure it (xor with fixed key) since we need the actual password to authenticate to last.fm. We (the community) should never give users a false sense of security. There are a couple of other options: - store the password in gnome-keyring (would often require user interaction to get at the password) - prompt for the password the first time it's needed A patch to optionally do one of those rather than storing the password in gconf would be welcome.
I understand the point about false security. I dodn't mean to be rude with my pragmatic comment but hey, I got owned. My main idea here is that any password that can be displayed in normal-like situations - I consider the use configurator GUI be a normal-like behaviour - should be at least scrambled because you never know who sees what. In this regard, a simple scrambling could achieve what I want. Your ideas sound very well thought, and I have to think about them for a bit longer. Has the GNOME group created a standard for storing password-like data ? I am aware of the keyring, but just as I remembered, the user would have to supply a password very often. I don't think that users would appretiate the extra GUI dialogs just because they happen to use a plug-in. However, that would be the most sane approach, but with a session-based idea. If RB:Audioscrobbler could fetch the password and store it in memory (wherever,however, just not visible) until RB exits, the user would only have to unlock the keyring once, which is reasonable. I have to review the operation of Gnome-keyring for this, and possibly implement the feature. This was a very good idea, Mr Matthew ! ;)
There is an Ubuntu bug open about that: https://beta.launchpad.net/rhythmbox/+bug/42686
*** Bug 443539 has been marked as a duplicate of this bug. ***
Can't the md5 of the password be stored instead of the plaintext password? That's what the XMMS plugin does.
Why don't use gnome-keyring-manager?
*** Bug 505586 has been marked as a duplicate of this bug. ***
As #5 says (and I said in Bug 505586) you only have to store the md5sum of the password... since that is the only thing you are using the create a handshake with last.fm From my other bugreport: I suppose you are still using the old protocol there's a short introduction to it here: http://gabistapler.de/blog/index.php?/archives/268-Play-last.fm-streams-without-the-player.html I've documented the new last.fm 1.2 protocol here, if it's of any interest: http://code.google.com/p/thelastripper/wiki/LastFM12UnofficialDocumentation Even the new protocol does only require listeners to store an md5 sum of the password and not the plain text password... The difference is that the md5sum of the password can be used for listing to music, the plaintext can be used to login to last.fm site...
Created attachment 101669 [details] [review] Stores the md5 of the password Stores the md5 of the password instead of the password itself. Automatically converts a stored cleartext password into a stored md5 password. Differentiates an md5 password from a cleartext password by using the prefix "md5:". This should be extensible in case the password hash changes in future. Patch is against 0.11.2 and applies cleanly against trunk (with some offsets). rb-audioscrobbler.c | 104 +++++++++++++++++++++++++++++++++++----------------- rb-audioscrobbler.h | 1 rb-lastfm-source.c | 33 +--------------- 3 files changed, 73 insertions(+), 65 deletions(-)
What exactly can you do with the raw password that you can't do with the md5 of the password?
You can copy the md5 of the password into a browser and use it to login to http://last.fm You can do that with the password it self. However you can login to scrobble or listen to radio using md5 of password and not the password it self. - Ideally the md5 of the password should be stored in gnome-keyring too...
(In reply to comment #11) > You can copy the md5 of the password into a browser and use it to login to > http://last.fm If this is true, it means that md5(password) effectively is the password, so storing it instead of the plain text password does nothing at all. It doesn't seem to be, though.
For access to web services at last.fm md5(password) is effectively the password, in the terms that username and md5(password) is all you need for authentication. Applications will never have to use anything but the md5(password), in the old last.fm protocol version 1.1, you'd send md5(password) in querystring during login. If you want to listen to the last.fm radio streams... If you wish to scrobble with the old version you'd have to reply with md5(md5(your_password) + challenge) acoording to: http://www.audioscrobbler.net/wiki/Protocol1.1.merged In the new last.fm protocol version 1.2, you'd send md5( md5(password) + unixtimestamp) in a querystring during login, if you wish to scrobble or listen to radio stations. As documented here: http://www.audioscrobbler.net/development/protocol/ So applications will NEVER have to use anything but md5(password). In fact you could just ask the user for the md5sum of his password, though that wouldn't be very user friendly :) I don't think last.fm saves anything else than the md5sum of the password on their servers either... By the way, listen to last.fm streams isn't documented in their official documentation, however there's some resources here if it of any interest: http://gabistapler.de/blog/index.php?/archives/268-Play-last.fm-streams-without-the-player.html http://code.google.com/p/thelastripper/wiki/LastFM12UnofficialDocumentation
For the people criticizing the security of storing the md5, you are certainly right that it doesn't provide additional security for last.fm. However the critical point here is that in a non-utopian world, people sometimes use the same password for multiple sites, and not all of these other sites only require the md5 to login. As such, if the md5 were stored instead, the compromised password would be much less useful in compromising other accounts that the user may have. Personally I would recommend storing the md5 instead as a first quick step, and then second, using gnome-keyring to store that, if it is decided that wouldn't be too intrusive. In Ubuntu the keyring is authenticated at login so for those users it would not have additional impact.
Created attachment 119597 [details] [review] updated patch to apply against recent svn Okay, I tried applying the patch to a recent svn checkout of rhythmbox (specifically, Ubuntu's rhythmbox-0.11.6svn20080916) and had 5 failed chunks. After some manual interpolation I came up with a new patch that seems functional when I test it; the password is stored as md5 and the tracks get sent to last.fm as expected. This was my first adventure into rhythmbox code and my C++ is not great, could someone review the patch and consider it? Thanks!
*** Bug 563843 has been marked as a duplicate of this bug. ***
Comment on attachment 119597 [details] [review] updated patch to apply against recent svn going to use gnome-keyring instead of this.
Created attachment 155086 [details] [review] Store audioscrobbler passwords in GNOME keyring the patch makes audioscrobbler plugin save password (and only password) in the default GNOME Keyring. It is saved under the name protocol://user@host/ (for example http://kklimonda@post.audioscrobbler.com/). I've used GtkInfoBar to display errors related to GNOME Keyring so the GTK+ dependency is bumbed to 2.18.0 - it can be written without it if keeping dependency is really important. ugly things I can see myself: - rb_audioscrobbler_migrate_to_keyring() may loose password if there is a problem with GNOME Keyring. It is quite unlikely case though - on the properly configured system something would have to go terribly wrong. - in keyring_set_server_data() I have no idea how to get a host from URI, neither could I find any good snippet of the code. I don't see other ugly things but they have to be there so please comment.
Created attachment 155088 [details] [review] Modified configure.ac and Makefile.am
It's too soon to bump the gtk dependency to 2.18. Maybe use the GeditMessageArea widget like the last.fm source does. I'd suggest using SoupURI to parse URI strings, since we're already using libsoup in the last.fm plugin.
Review of attachment 155086 [details] [review]: ::: plugins/audioscrobbler/rb-audioscrobbler-plugin.c @@ +205,3 @@ + plugin = RB_AUDIOSCROBBLER_PLUGIN (user_data); + if (result == GNOME_KEYRING_RESULT_CANCELLED) + { nit: { goes on the previous line @@ +245,3 @@ } + gtk_widget_show (plugin->preferences); what's the reason for this change? ::: plugins/audioscrobbler/rb-audioscrobbler.c @@ +962,3 @@ + scrobbler_url = g_strdup (SCROBBLER_URL); + rb_debug ("%s", SCROBBLER_URL); + tmp = g_strdup (g_strrstr (scrobbler_url, "://")); Use SoupURI here instead @@ +1131,2 @@ rb_audioscrobbler_add_timeout (audioscrobbler); + audioscrobbler->priv->status = BADAUTH; I think I understand why you're setting the status to BADAUTH here, but that doesn't really seem like the right status. @@ +1233,3 @@ + audioscrobbler->priv->message_label = gtk_label_new (""); + + vbox = GTK_WIDGET (gtk_builder_get_object (builder, "vbox2")); If you're going to refer to a widget from a builder file by name, you should give it a meaningful name.
the reason for changing gtk_widget_show_all () to gtk_widget_show () was the fact that the GtkInfoBar is hidden by default when there is no error and by using gtk_widget_show_all () it was being displayed without any text message or reason. BADAUTH isn't indeed the best choice - I'll think about it some more. I'm going to upload a revised patch with some changes in the rb-lastfm-source.c (no idea how I've missed them) after the weeked as I'm going off the grid for the next few days. Thanks for reviewing it (and reminding me of libsoup).
Created attachment 156017 [details] [review] updated patch to use gnome keyring instead of gconf for password storage last patch didn't how RBLastfmSource stored passwords at all so the new patch was required. I have created a new class for handling configuration - RBAudioscrobblerConfig and made both RBAudioscrobbler and RBLastfmSource use it instead of writing GNOME Keyring support into them directly. I have also decided not to display problems related go GNOME Keyring to the user directly (they are only displayed in debug logs). The only two results we are interested in are GNOME_KEYRING_RESULT_NO_KEYRING_DAEMON and GNOME_KEYRING_RESULT_IO_ERROR - both indicate serious issues with users' configuration, I wasn't sure if the way I've displayed errors to the user was the right one and couldn't come up with a better idea.
Review of attachment 156017 [details] [review]: I haven't tried this out yet, but it generally looks OK. A few style issues, though. ::: plugins/audioscrobbler/rb-audioscrobbler-config.c @@ +99,3 @@ + + if (config->priv->username) + g_free (config->priv->username); g_free checks for NULL, so you don't need to do it yourself @@ +101,3 @@ + g_free (config->priv->username); + if (config->priv->password) + gnome_keyring_free_password (config->priv->password); I'm not sure if gnome_keyring_free_password does too, though @@ +114,3 @@ + +static void +rb_audioscrobbler_config_dispose (GObject *object) need to chain up to the parent class dispose function here @@ +159,3 @@ + password = eel_gconf_get_string (CONF_AUDIOSCROBBLER_PASSWORD); + + display_name = g_strdup_printf ("%s://%s@%s/", config->priv->scheme, probably should have a single function for constructing display names, rather than doing it in two separate places @@ +175,3 @@ + rb_debug ("gnome keyring error: %s", + gnome_keyring_result_to_message (result)); + goto out; don't use goto unless it actually makes things simpler.. if (result == GNOME_KEYRING_RESULT_OK) { eel_gconf_unset (CONF_AUDIOSCROBBLER_PASSWORD); } else { rb_debug ("gnome keyring error: %s", ...); } actually, that should probably be g_warning. @@ +263,3 @@ + rb_debug ("gnome keyring error: %s", + gnome_keyring_result_to_message (result)); + goto out; again, I think it'd be better to avoid goto here @@ +270,3 @@ + config->priv->password = gnome_keyring_memory_strdup (password); + gtk_entry_set_text (GTK_ENTRY (config->priv->password_entry), password); + g_signal_emit_by_name (config, "config-changed"); should use the signal ID instead @@ +275,3 @@ + if (password != NULL) + gnome_keyring_memory_free (password); + return; don't need a return at the end of a void function @@ +285,3 @@ + + if (result != GNOME_KEYRING_RESULT_OK) { + rb_debug ("%s", gnome_keyring_result_to_message (result)); should make this a g_warning and provide a bit more context, like "Unable to find password: %s" @@ +304,3 @@ +{ + config->priv->username = + g_strdup (gtk_entry_get_text (GTK_ENTRY (widget))); need to free the old username here @@ +329,3 @@ + + if (config->priv->password && + !g_strcmp0 (config->priv->password, password) ) { If you're using g_strcmp0, you don't need to check that strings are non-NULL @@ +390,3 @@ + + gconf_password = eel_gconf_get_string (CONF_AUDIOSCROBBLER_PASSWORD); + if (gconf_password && g_strcmp0 (gconf_password, "") != 0) { again, no need to check gconf_password is non-NULL here ::: plugins/audioscrobbler/rb-audioscrobbler.c @@ +380,3 @@ + case PROP_CONFIG: + audioscrobbler->priv->config = g_value_get_object (value); + g_object_ref (G_OBJECT (audioscrobbler->priv->config)); more simply, audioscrobbler->priv->config = g_value_dup_object (value); ::: plugins/audioscrobbler/rb-lastfm-source.c @@ +599,3 @@ + case PROP_CONFIG: + source->priv->config = g_value_get_object (value); + g_object_ref (G_OBJECT (source->priv->config)); source->priv->config = g_value_dup_object (value);
Created attachment 156120 [details] [review] updated patch to use gnome keyring instead of gconf for password storage gnome_keyring_memory_free () does indeed check for NULL. No idea how verbose my description of changes should be, I'm going to assume that bugzilla have a diff system for patches and if not I can write something later. I've made all changes requested, thanks for your review.
Created attachment 156136 [details] [review] updated patch to use gnome keyring instead of gconf for password storage Lets just pretend that the previous patch hasn't happened.. I have no idea how could I remove goto statement the way I did to "make it simpler". I got distracted.
Review of attachment 156136 [details] [review]: Did you test this at all? It pretty much doesn't work for me. You haven't included the necessary changes to plugins/Makefile.am (can't build the audioscrobbler plugin without gnome-keyring) or plugins/audioscrobbler/Makefile.am (need to add GNOME_KEYRING_LIBS and GNOME_KEYRING_CFLAGS in the appropriate places). ::: plugins/audioscrobbler/rb-audioscrobbler-config.c @@ +236,3 @@ + + config->priv->username = + username == NULL ? NULL : g_strdup (username); only use the ternary operator if it makes things clearer.. it doesn't do that here. @@ +265,3 @@ + * like an overkill. so I use _sync () here unless asked otherwise. + */ + result = gnome_keyring_find_password_sync (GNOME_KEYRING_NETWORK_PASSWORD, Why are you fetching the password you just stored in the keyring? Don't we already know what it is? @@ +304,3 @@ + + /* we have to change password_entry text here so it's set the the init + time */ this comment is a bit incoherent @@ +353,3 @@ + NULL, + display_name, + config->priv->password, This stores the existing password, not the new one @@ +361,3 @@ + + g_free (display_name); + return FALSE; 'password' is leaked here ::: plugins/audioscrobbler/rb-audioscrobbler-config.h @@ +42,3 @@ + GObjectClass parent_class; + + void (*config_changed) (void); Signal handler declarations include the 'self' object - so this should be 'void (*config_changed) (RBAudioscrobblerConfig *config)' ::: plugins/audioscrobbler/rb-audioscrobbler.c @@ -274,3 @@ rb_audioscrobbler_load_queue (audioscrobbler); - - rb_audioscrobbler_import_settings (audioscrobbler); Need to call rb_audioscrobbler_add_timeout here, or nothing will be submitted