Hi,
We had a user in Debian who was having problems with the
xfce4-mpc-plugin and a long password. It turned out that passwords
longer than about 30 characters were causing buffer overflows.
I looked into it and found a few problems. There are lots of sprintfs
into buffers with strings which contain untrusted input.
I've fixed some in the attached patch against 0.3.3 although I want it
to be reviewed at some point.
I also couldn't see a nice way to return an error message back to the
user and I'm not really a GTK coder in anyway :)
You may well choose to fix these issues in a different way in which case
we'd love to see the patch to get it into Debian.
Anyway, if you have some time to review the patch it'd be great.
Thanks.
--
----------( "Everyone who is alive, please raise your hand. )----------
Simon ----( See, told ya," - Rimmer. )---- Nomis
Htag.pl 0.0.24
diff -urN xfce4-mpc-plugin-0.3.3/debian/changelog xfce4-mpc-plugin-0.3.3.patched/debian/changelog
--- xfce4-mpc-plugin-0.3.3/debian/changelog 2008-10-06 15:41:52.000000000 +0100
+++ xfce4-mpc-plugin-0.3.3.patched/debian/changelog 2008-11-26 11:08:51.000000000 +0000
@@ -1,3 +1,9 @@
+xfce4-mpc-plugin (0.3.3-1huggie) unstable; urgency=low
+
+ * Patch for buffer overflow in password handling
+
+ -- Simon Huggins <[EMAIL PROTECTED]> Wed, 26 Nov 2008 11:08:34 +0000
+
xfce4-mpc-plugin (0.3.3-1) unstable; urgency=low
[ Simon Huggins ]
diff -urN xfce4-mpc-plugin-0.3.3/panel-plugin/simple-libmpd.c xfce4-mpc-plugin-0.3.3.patched/panel-plugin/simple-libmpd.c
--- xfce4-mpc-plugin-0.3.3/panel-plugin/simple-libmpd.c 2008-03-24 19:17:52.000000000 +0000
+++ xfce4-mpc-plugin-0.3.3.patched/panel-plugin/simple-libmpd.c 2008-12-08 23:00:35.000000000 +0000
@@ -37,17 +37,15 @@
#include <errno.h>
#include <fcntl.h>
-#define STRLENGTH 32
-
MpdObj* mpd_new(char* host, int port, char* pass)
{
MpdObj* mo = g_new0(MpdObj,1);
DBG("host=%s, port=%d, pass=%s", host, port, pass);
- mo->host = g_strndup(host,STRLENGTH);
+ mo->host = g_strdup(host);
mo->port = port;
- mo->pass = g_strndup(pass,STRLENGTH);
+ mo->pass = g_strdup(pass);
mo->socket = 0;
mo->status = 0;
mo->repeat = 0;
@@ -508,7 +506,7 @@
char outbuf[15];
/* write setvol 'newvol' to socket */
DBG("!");
- sprintf(outbuf,"setvol %d\n",newvol);
+ snprintf(outbuf, sizeof(outbuf), "setvol %d\n",newvol);
mpd_send_single_cmd(mo,outbuf);
}
@@ -528,7 +526,7 @@
{
char outbuf[15];
DBG("!");
- sprintf(outbuf,"random %d\n",random);
+ snprintf(outbuf, sizeof(outbuf), "random %d\n",random);
return mpd_send_single_cmd(mo,outbuf);
}
@@ -537,7 +535,7 @@
{
char outbuf[15];
DBG("!");
- sprintf(outbuf,"repeat %d\n",repeat);
+ snprintf(outbuf, sizeof(outbuf), "repeat %d\n",repeat);
return mpd_send_single_cmd(mo,outbuf);
}
@@ -584,7 +582,7 @@
{
char outbuf[15];
DBG("!");
- sprintf(outbuf,"playid %d\n",id);
+ snprintf(outbuf, sizeof(outbuf), "playid %d\n",id);
return mpd_send_single_cmd(mo,outbuf);
}
@@ -597,9 +595,16 @@
void mpd_send_password(MpdObj* mo)
{
DBG("!");
- char outbuf[30];
+ char outbuf[256];
/* write password 'password' to socket */
- sprintf(outbuf,"password %s\n",mo->pass);
+ int wrote = snprintf(outbuf, sizeof(outbuf), "password %s\n",mo->pass);
+ if (wrote > 255) {
+ /* the password is too long to fit though there doesn't seem to be a
+ * nice way to report this error :-/ */
+ fprintf(stderr, "xfce4-mpc-plugin: password too long!\n");
+ mo->error = MPD_ERROR_SYSTEM;
+ return;
+ }
mpd_send_single_cmd(mo,outbuf);
}
@@ -607,14 +612,14 @@
{
DBG("! new hostname=%s",host);
g_free(mo->host);
- mo->host = g_strndup(host,STRLENGTH);
+ mo->host = g_strdup(host);
}
void mpd_set_password(MpdObj* mo, char* pass)
{
DBG("! new password=%s",pass);
g_free(mo->pass);
- mo->pass = g_strndup(pass,STRLENGTH);
+ mo->pass = g_strdup(pass);
}
void mpd_set_port(MpdObj* mo,int port)
diff -urN xfce4-mpc-plugin-0.3.3/panel-plugin/xfce4-mpc-plugin.c xfce4-mpc-plugin-0.3.3.patched/panel-plugin/xfce4-mpc-plugin.c
--- xfce4-mpc-plugin-0.3.3/panel-plugin/xfce4-mpc-plugin.c 2008-03-24 19:17:52.000000000 +0000
+++ xfce4-mpc-plugin-0.3.3.patched/panel-plugin/xfce4-mpc-plugin.c 2008-12-08 23:01:31.000000000 +0000
@@ -29,7 +29,6 @@
#define DEFAULT_MPD_HOST "localhost"
#define DEFAULT_MPD_PORT 6600
#define DIALOG_ENTRY_WIDTH 15
-#define STRLENGTH 32
#include "xfce4-mpc-plugin.h"
@@ -107,7 +106,7 @@
mpc->show_frame = xfce_rc_read_bool_entry (rc, "show_frame", TRUE);
mpc->client_appl = g_strdup(xfce_rc_read_entry (rc, "client_appl", ""));
label = gtk_bin_get_child(GTK_BIN(mpc->appl));
- g_sprintf(str, "%s %s", _("Launch"), mpc->client_appl);
+ g_snprintf(str, sizeof(str), "%s %s", _("Launch"), mpc->client_appl);
gtk_label_set_text(GTK_LABEL(label),str);
DBG ("Settings : [EMAIL PROTECTED]:%d\nframe:%d\nappl:%s", mpc->mpd_password, mpc->mpd_host, mpc->mpd_port, mpc->show_frame, mpc->client_appl);
xfce_rc_close (rc);
@@ -165,12 +164,12 @@
char str[30];
t_mpc *mpc = dialog->mpc;
- mpc->mpd_host = g_strndup(gtk_entry_get_text(GTK_ENTRY(dialog->textbox_host)),STRLENGTH);
+ mpc->mpd_host = g_strdup(gtk_entry_get_text(GTK_ENTRY(dialog->textbox_host)));
mpc->mpd_port = atoi(gtk_entry_get_text(GTK_ENTRY(dialog->textbox_port)));
- mpc->mpd_password = g_strndup(gtk_entry_get_text(GTK_ENTRY(dialog->textbox_password)),STRLENGTH);
- mpc->client_appl = g_strndup(gtk_entry_get_text(GTK_ENTRY(dialog->textbox_client_appl)),STRLENGTH);
+ mpc->mpd_password = g_strdup(gtk_entry_get_text(GTK_ENTRY(dialog->textbox_password)));
+ mpc->client_appl = g_strdup(gtk_entry_get_text(GTK_ENTRY(dialog->textbox_client_appl)));
label = gtk_bin_get_child(GTK_BIN(mpc->appl));
- g_sprintf(str, "%s %s", _("Launch"), mpc->client_appl);
+ g_snprintf(str, sizeof(str), "%s %s", _("Launch"), mpc->client_appl);
gtk_label_set_text(GTK_LABEL(label),str);
DBG ("Apply: host=%s, port=%d, passwd=%s, appl=%s", mpc->mpd_host, mpc->mpd_port, mpc->mpd_password, mpc->client_appl);
@@ -311,13 +310,13 @@
{
/* buf may contain stuff, care to append text */
if (!song->artist || !song->title)
- g_sprintf(str,"%s%s", str, song->file);
+ g_snprintf(str, sizeof(str), "%s%s", str, song->file);
else if (!song->album)
- g_sprintf(str,"%s%s - %s", str, song->artist, song->title);
+ g_snprintf(str, sizeof(str), "%s%s - %s", str, song->artist, song->title);
else if (!song->track)
- g_sprintf(str,"%s%s - %s -/- %s", str, song->artist, song->album, song->title);
+ g_snprintf(str, sizeof(str), "%s%s - %s -/- %s", str, song->artist, song->album, song->title);
else
- g_sprintf(str,"%s%s - %s -/- (#%s) %s", str, song->artist, song->album, song->track, song->title);
+ g_snprintf(str, sizeof(str), "%s%s - %s -/- (#%s) %s", str, song->artist, song->album, song->track, song->title);
}
static void
@@ -337,28 +336,28 @@
}
}
- g_sprintf(str, "Volume : %d%%", mpd_status_get_volume(mpc->mo));
+ g_snprintf(str, sizeof(512), "Volume : %d%%", mpd_status_get_volume(mpc->mo));
switch (mpd_player_get_state(mpc->mo))
{
case MPD_PLAYER_PLAY:
- g_sprintf(str, "%s - Mpd Playing\n",str);
+ g_snprintf(str, sizeof(str), "%s - Mpd Playing\n",str);
break;
case MPD_PLAYER_PAUSE:
- g_sprintf(str, "%s - Mpd Paused\n",str);
+ g_snprintf(str, sizeof(str), "%s - Mpd Paused\n",str);
break;
case MPD_PLAYER_STOP:
- g_sprintf(str, "%s - Mpd Stopped\n",str);
+ g_snprintf(str, sizeof(str), "%s - Mpd Stopped\n",str);
break;
default:
- g_sprintf(str, "%s - Mpd state ?\n",str);
+ g_snprintf(str, sizeof(str), "%s - Mpd state ?\n",str);
break;
}
song = mpd_playlist_get_current_song(mpc->mo);
if (song && song->id != -1)
format_song_display(song, str);
else
- g_sprintf(str,"%sFailed to get song info ?", str);
+ g_snprintf(str, sizeof(str), "%sFailed to get song info ?", str);
gtk_check_menu_item_set_active(GTK_CHECK_MENU_ITEM(mpc->random), mpd_player_get_random(mpc->mo));
gtk_check_menu_item_set_active(GTK_CHECK_MENU_ITEM(mpc->repeat), mpd_player_get_repeat(mpc->mo));
_______________________________________________
Pkg-xfce-devel mailing list
[email protected]
http://lists.alioth.debian.org/mailman/listinfo/pkg-xfce-devel
