Bug#475733: (no subject)
On Wed, Apr 16, 2008 at 10:21:13PM +0200, Nico Golde wrote: Hi, * [EMAIL PROTECTED] [2008-04-16 22:05]: Thanks for the help. I have made a patch that would fix the possible buffer overflows. Please check the attached patch. [...] if(path[0]!='/') - sprintf(tmp,%s/translations/%s,DATAPATH,path); + snprintf(tmp,302,%s/translations/%s,DATAPATH,path); off-by two. Why don't you just use sizeof(tmp)? And why use sizeof(tmp) with the possibility of truncating the resulting string while we can properly malloc() enough size to hold the whole path ? -- GPG-Key: 0xA3FD0DF7 - 9F73 032E EAC9 F7AD 951F 280E CB66 8E29 A3FD 0DF7 Debian User and Developer. Homepage: www.foolab.org signature.asc Description: Digital signature
Bug#475733: (no subject)
Hi Mohammed, * Mohammed Sameer [EMAIL PROTECTED] [2008-04-17 15:53]: On Wed, Apr 16, 2008 at 10:21:13PM +0200, Nico Golde wrote: * [EMAIL PROTECTED] [2008-04-16 22:05]: Thanks for the help. I have made a patch that would fix the possible buffer overflows. Please check the attached patch. [...] if(path[0]!='/') - sprintf(tmp,%s/translations/%s,DATAPATH,path); + snprintf(tmp,302,%s/translations/%s,DATAPATH,path); off-by two. Why don't you just use sizeof(tmp)? And why use sizeof(tmp) with the possibility of truncating the resulting string while we can properly malloc() enough size to hold the whole path ? Cause you have a maximum length for these values specified by the shell and malloc(foo + somelength) operations often lead to integer overflows (well not in this case). Anyway, the 302 was fine since it was tmp from a different source file where it is specified to have 302 bytes. Kind regards Nico -- Nico Golde - http://www.ngolde.de - [EMAIL PROTECTED] - GPG: 0x73647CFF For security reasons, all text in this mail is double-rot13 encrypted. pgp9S3ZqiMdoV.pgp Description: PGP signature
Bug#475733: (no subject)
On Thu, Apr 17, 2008 at 04:02:25PM +0200, Nico Golde wrote: Hi Mohammed, * Mohammed Sameer [EMAIL PROTECTED] [2008-04-17 15:53]: On Wed, Apr 16, 2008 at 10:21:13PM +0200, Nico Golde wrote: * [EMAIL PROTECTED] [2008-04-16 22:05]: Thanks for the help. I have made a patch that would fix the possible buffer overflows. Please check the attached patch. [...] if(path[0]!='/') - sprintf(tmp,%s/translations/%s,DATAPATH,path); + snprintf(tmp,302,%s/translations/%s,DATAPATH,path); off-by two. Why don't you just use sizeof(tmp)? And why use sizeof(tmp) with the possibility of truncating the resulting string while we can properly malloc() enough size to hold the whole path ? Cause you have a maximum length for these values specified by the shell and malloc(foo + somelength) operations often lead to integer overflows (well not in this case). Anyway, the 302 was fine since it was tmp from a different source file where it is specified to have 302 bytes. A maximum length for $HOME ? Never heard of that. If you malloc(strlen(DATAPATH) + 1); then you won't overflow. Cheers, -- GPG-Key: 0xA3FD0DF7 - 9F73 032E EAC9 F7AD 951F 280E CB66 8E29 A3FD 0DF7 Debian User and Developer. Homepage: www.foolab.org signature.asc Description: Digital signature
Bug#475733: (no subject)
Hi Mohammed, * Mohammed Sameer [EMAIL PROTECTED] [2008-04-17 22:36]: On Thu, Apr 17, 2008 at 04:02:25PM +0200, Nico Golde wrote: Hi Mohammed, * Mohammed Sameer [EMAIL PROTECTED] [2008-04-17 15:53]: On Wed, Apr 16, 2008 at 10:21:13PM +0200, Nico Golde wrote: * [EMAIL PROTECTED] [2008-04-16 22:05]: Thanks for the help. I have made a patch that would fix the possible buffer overflows. Please check the attached patch. [...] if(path[0]!='/') - sprintf(tmp,%s/translations/%s,DATAPATH,path); + snprintf(tmp,302,%s/translations/%s,DATAPATH,path); off-by two. Why don't you just use sizeof(tmp)? And why use sizeof(tmp) with the possibility of truncating the resulting string while we can properly malloc() enough size to hold the whole path ? Cause you have a maximum length for these values specified by the shell and malloc(foo + somelength) operations often lead to integer overflows (well not in this case). Anyway, the 302 was fine since it was tmp from a different source file where it is specified to have 302 bytes. A maximum length for $HOME ? Never heard of that. If you malloc(strlen(DATAPATH) + 1); then you won't overflow. _POSIX_PATH_MAX should fit. Cheers Nico -- Nico Golde - http://www.ngolde.de - [EMAIL PROTECTED] - GPG: 0x73647CFF For security reasons, all text in this mail is double-rot13 encrypted. pgpg9UoViB8nG.pgp Description: PGP signature
Bug#475733: (no subject)
Hello,
Thanks for the help. I have made a patch that would fix the possible
buffer overflows. Please check the attached patch.
On Mon, Apr 14, 2008 at 02:54:21PM +0200, Nico Golde wrote:
Just saw it and I have to admit that I'm not really happy
with it. Please just let the code as it is now and used
snprintf instead with a length of sizeof(tmp). Please also
check the other buffers.
---end quoted text---
--
أحمد المحمودي (Ahmed El-Mahmoudy)
Digital design engineer
SySDSoft, Inc.
GPG KeyID: 0x9DCA0B27 (@ subkeys.pgp.net)
GPG Fingerprint: 087D 3767 8CAC 65B1 8F6C 156E D325 C3C8 9DCA 0B27
Index: acon-1.0.5/acon.c
===
--- acon-1.0.5.orig/acon.c 2008-04-16 20:43:11.0 +0200
+++ acon-1.0.5/acon.c 2008-04-16 21:11:38.0 +0200
@@ -50,7 +50,7 @@
font[0]=translation[0]=keymap[0]=0;
if((env=getenv(HOME)))
- sprintf(tmp,%s/.acon.conf,env);
+ snprintf(tmp,300,%s/.acon.conf,env);
else
strcpy(tmp,/etc/acon.conf);
if((fp=fopen(tmp,r))==NULL)
Index: acon-1.0.5/arabicfont.c
===
--- acon-1.0.5.orig/arabicfont.c 2008-04-16 21:06:32.0 +0200
+++ acon-1.0.5/arabicfont.c 2008-04-16 21:11:28.0 +0200
@@ -613,7 +613,7 @@
y=16; /*Only support 8x16 fonts now*/
if(path[0]!='/')
- sprintf(tmp,%s/fonts/%s,DATAPATH,path);
+ snprintf(tmp,300,%s/fonts/%s,DATAPATH,path);
else
strcpy(tmp,path);
set_user_id();
@@ -699,12 +699,12 @@
if(path)
{
if(path[0]!='/')
- sprintf(tmp,loadkeys %s/keymaps/%s /dev/null,DATAPATH,path);
+ snprintf(tmp,300,loadkeys %s/keymaps/%s /dev/null,DATAPATH,path);
else
- sprintf(tmp,loadkeys %s /dev/null,path);
+ snprintf(tmp,300,loadkeys %s /dev/null,path);
}
else
- sprintf(tmp,loadkeys %s/keymaps/iso8859-6.map,DATAPATH);
+ snprintf(tmp,300,loadkeys %s/keymaps/iso8859-6.map,DATAPATH);
if(my_system(tmp))
printf(Error executing : %s \n,tmp);
@@ -743,7 +743,7 @@
}
if(path[0]!='/')
- sprintf(tmp,%s/translations/%s,DATAPATH,path);
+ snprintf(tmp,302,%s/translations/%s,DATAPATH,path);
else
strcpy(tmp,path);
if((fp=fopen(tmp,r))==NULL)
Index: acon-1.0.5/child.c
===
--- acon-1.0.5.orig/child.c 2008-04-16 21:04:18.0 +0200
+++ acon-1.0.5/child.c 2008-04-16 21:11:53.0 +0200
@@ -101,7 +101,7 @@
printf(Acon: can't know HOME directory\n);
return;
}
- sprintf(tmp,%s/.acon.conf,env);
+ snprintf(tmp,300,%s/.acon.conf,env);
if((fp=fopen(tmp,w))==NULL)
{
printf(Acon: can't save %s\n,tmp);
Bug#475733: (no subject)
Hi, * [EMAIL PROTECTED] [2008-04-16 22:05]: Thanks for the help. I have made a patch that would fix the possible buffer overflows. Please check the attached patch. [...] if(path[0]!='/') - sprintf(tmp,%s/translations/%s,DATAPATH,path); + snprintf(tmp,302,%s/translations/%s,DATAPATH,path); off-by two. Why don't you just use sizeof(tmp)? Kind regards Nico -- Nico Golde - http://www.ngolde.de - [EMAIL PROTECTED] - GPG: 0x73647CFF For security reasons, all text in this mail is double-rot13 encrypted. pgpPVVqaWkJtj.pgp Description: PGP signature
Bug#475733: (no subject)
Hello, On Wed, Apr 16, 2008 at 10:21:13PM +0200, Nico Golde wrote: if(path[0]!='/') - sprintf(tmp,%s/translations/%s,DATAPATH,path); + snprintf(tmp,302,%s/translations/%s,DATAPATH,path); off-by two. Why don't you just use sizeof(tmp)? Kind regards Nico ---end quoted text--- Actually for this one, tmp is declared as: char tmp[302]; I will use sizeof(tmp) anyways. So is this patch enough to close the bug ? -- أحمد المحمودي (Ahmed El-Mahmoudy) Digital design engineer SySDSoft, Inc. GPG KeyID: 0x9DCA0B27 (@ subkeys.pgp.net) GPG Fingerprint: 087D 3767 8CAC 65B1 8F6C 156E D325 C3C8 9DCA 0B27 -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#475733: (no subject)
Hi Mohammed, * Mohammed Sameer [EMAIL PROTECTED] [2008-04-13 18:18]: I think I'm missing something. Why do we need to make it not suid if the daemon drops it (-6 upload) ? Cause it does drop it via seteuid and as long as the buffer overflow exists possible injected shellcode could do seteuid(0) to get it back. Kind regards Nico -- Nico Golde - http://www.ngolde.de - [EMAIL PROTECTED] - GPG: 0x73647CFF For security reasons, all text in this mail is double-rot13 encrypted. pgpEa5Uac9sRQ.pgp Description: PGP signature
Bug#475733: (no subject)
On Mon, Apr 14, 2008 at 02:26:47PM +0200, Nico Golde wrote: Hi Mohammed, * Mohammed Sameer [EMAIL PROTECTED] [2008-04-13 18:18]: I think I'm missing something. Why do we need to make it not suid if the daemon drops it (-6 upload) ? Cause it does drop it via seteuid and as long as the buffer overflow exists possible injected shellcode could do seteuid(0) to get it back. Kind regards Nico aha! I sent a patch earlier as an attempt to fix the buffer overflow vulnerability. I'd appreciate someone reviewing it. I can do an upload if it's OK. Cheers, -- GPG-Key: 0xA3FD0DF7 - 9F73 032E EAC9 F7AD 951F 280E CB66 8E29 A3FD 0DF7 Debian User and Developer. Homepage: www.foolab.org signature.asc Description: Digital signature
Bug#475733: (no subject)
Hi Mohammed, * Mohammed Sameer [EMAIL PROTECTED] [2008-04-14 14:33]: On Mon, Apr 14, 2008 at 02:26:47PM +0200, Nico Golde wrote: Hi Mohammed, * Mohammed Sameer [EMAIL PROTECTED] [2008-04-13 18:18]: I think I'm missing something. Why do we need to make it not suid if the daemon drops it (-6 upload) ? Cause it does drop it via seteuid and as long as the buffer overflow exists possible injected shellcode could do seteuid(0) to get it back. aha! I sent a patch earlier as an attempt to fix the buffer overflow vulnerability. I'd appreciate someone reviewing it. I can do an upload if it's OK. Just saw it and I have to admit that I'm not really happy with it. Please just let the code as it is now and used snprintf instead with a length of sizeof(tmp). Please also check the other buffers. Kind regards Nico -- Nico Golde - http://www.ngolde.de - [EMAIL PROTECTED] - GPG: 0x73647CFF For security reasons, all text in this mail is double-rot13 encrypted. pgpHzrIDKQp2r.pgp Description: PGP signature
Bug#475733: (no subject)
I think I'm missing something. Why do we need to make it not suid if the daemon drops it (-6 upload) ? -- GPG-Key: 0xA3FD0DF7 - 9F73 032E EAC9 F7AD 951F 280E CB66 8E29 A3FD 0DF7 Debian User and Developer. Homepage: www.foolab.org signature.asc Description: Digital signature
