Re: Permission to upload unrar-nonfree to testing-proposed-updates
On Mon, Feb 12, 2007 at 12:14:07PM -0200, Nelson A. de Oliveira wrote:
> Hi release managers!
> I have uploaded unrar-nonfree 3.7.3 to unstable, fixing CVE-2007-0855.
> Testing version is 3.5.4 and it's also vulnerable. Changes from 3.5.4 to
> 3.7.3 are big: 71 files changed, 1307 insertions and 476 deletions (so
> probably you won't allow version 3.7.3 to propagate to testing).
> Martin have backported the fix for this issue to 3.5.4:
> 2 files changed, 19 insertions and 17 deletions.
> The patch is attached for review.
> If everything is correct, do you allow the upload of a fixed 3.5.4 to
> testing-proposed-updates, please?
Yes, please do upload to t-p-u.
I don't understand some of this diff, though. E.g., why are there changes
to OS defines included in a security fix?
> --- unrar-nonfree-3.5.4/consio.cpp 2005-10-04 08:57:54.000000000 +0100
> +++ unrar-nonfree-3.7.3/consio.cpp 2007-02-02 06:13:40.000000000 +0000
> @@ -4,7 +4,9 @@
> #include "log.cpp"
> #endif
>
> +#if !defined(GUI) && !defined(SILENT)
> static void RawPrint(char *Msg,MESSAGE_TYPE MessageType);
> +#endif
>
> static MESSAGE_TYPE MsgStream=MSG_STDOUT;
> static bool Sound=false;
This doesn't appear security-related.
> @@ -119,11 +121,12 @@
> OemToChar(Str,Str);
> SetConsoleMode(hConIn,ConInMode);
> SetConsoleMode(hConOut,ConOutMode);
> -#elif defined(_EMX) || defined(_BEOS)
> +#elif defined(_EMX) || defined(_BEOS) || defined(__sparc) || defined(sparc) || defined (__VMS)
> fgets(Str,MaxLength-1,stdin);
This is pretty definitely not security-related,
> #else
> - strncpy(Str,getpass(""),MaxLength-1);
> + strncpyz(Str,getpass(""),MaxLength);
> #endif
> + Str[MaxLength-1]=0;
> RemoveLF(Str);
> }
> #endif
while this is.
> @@ -157,7 +160,7 @@
> Alarm();
> while (true)
> {
> - char PromptStr[256];
> + char PromptStr[NM+256];
> #if defined(_EMX) || defined(_BEOS)
> strcpy(PromptStr,St(MAskPswEcho));
> #else
Obvious security implications.
> @@ -166,7 +169,9 @@
> if (Type!=PASSWORD_GLOBAL)
> {
> strcat(PromptStr,St(MFor));
> - strcat(PromptStr,PointToName(FileName));
> + char *NameOnly=PointToName(FileName);
> + if (strlen(PromptStr)+strlen(NameOnly)<ASIZE(PromptStr))
> + strcat(PromptStr,NameOnly);
> }
> eprintf("\n%s: ",PromptStr);
> GetPasswordText(Password,MaxLength);
Ditto.
> @@ -174,19 +179,12 @@
> return(false);
> if (Type==PASSWORD_GLOBAL)
> {
> - strcpy(PromptStr,St(MReAskPsw));
> - eprintf(PromptStr);
> - char CmpStr[256];
> - GetPasswordText(CmpStr,sizeof(CmpStr));
> + eprintf(St(MReAskPsw));
> + char CmpStr[MAXPASSWORD];
> + GetPasswordText(CmpStr,ASIZE(CmpStr));
> if (*CmpStr==0 || strcmp(Password,CmpStr)!=0)
> {
> - strcpy(PromptStr,St(MNotMatchPsw));
> -/*
> -#ifdef _WIN_32
> - CharToOem(PromptStr,PromptStr);
> -#endif
> -*/
> - eprintf(PromptStr);
> + eprintf(St(MNotMatchPsw));
> memset(Password,0,MaxLength);
> memset(CmpStr,0,sizeof(CmpStr));
> continue;
Ditto (though it includes intermingled OS changes that don't seem to
belong?)
> @@ -210,7 +208,7 @@
> for (const char *NextItem=AskStr;NextItem!=NULL;NextItem=strchr(NextItem+1,'_'))
> {
> char *CurItem=Item[NumItems];
> - strncpy(CurItem,NextItem+1,sizeof(Item[0]));
> + strncpyz(CurItem,NextItem+1,ASIZE(Item[0]));
> char *EndItem=strchr(CurItem,'_');
> if (EndItem!=NULL)
> *EndItem=0;
Ditto.
> --- unrar-nonfree-3.5.4/consio.hpp 2005-10-04 08:57:54.000000000 +0100
> +++ unrar-nonfree-3.7.3/consio.hpp 2007-02-02 06:13:42.000000000 +0000
> @@ -25,7 +25,11 @@
> #define mprintf(args...)
> #define eprintf(args...)
> #else
> - inline void mprintf(const char *fmt,const char *a=NULL,const char *b=NULL) {}
> + #ifdef _MSC_VER
> + inline void mprintf(const char *fmt,...) {}
> + #else
> + inline void mprintf(const char *fmt,const char *a=NULL,const char *b=NULL) {}
> + #endif
> inline void eprintf(const char *fmt,const char *a=NULL,const char *b=NULL) {}
> inline void mprintf(const char *fmt,int b) {}
> inline void eprintf(const char *fmt,int b) {}
Platform specific change, irrelevant to Debian and irrelevant to the
security fix.
Thanks,
--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
vorlon@debian.org http://www.debian.org/
Reply to: