[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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: