Attached is an updated patch.
I didn't know what to do with the "though it includes intermingled OS
changes that don't seem to belong?" bit, so I've left them as they are.
They may not be specific to linux, but they are related to the fix.
If you want me to strip them out, I can do, but otherwise, attached is
the updated (cleaner) patch
On Mon, 2007-02-12 at 14:41 -0800, Steve Langasek wrote:
> 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,
--- unrar-nonfree-3.5.4/consio.cpp 2005-10-04 08:57:54.000000000 +0100
+++ unrar-nonfree-3.7.3-modified/consio.cpp 2007-02-13 04:17:25.000000000 +0000
@@ -122,8 +124,9 @@
#elif defined(_EMX) || defined(_BEOS)
fgets(Str,MaxLength-1,stdin);
#else
- strncpy(Str,getpass(""),MaxLength-1);
+ strncpyz(Str,getpass(""),MaxLength);
#endif
+ Str[MaxLength-1]=0;
RemoveLF(Str);
}
#endif
@@ -157,7 +160,7 @@
Alarm();
while (true)
{
- char PromptStr[256];
+ char PromptStr[NM+256];
#if defined(_EMX) || defined(_BEOS)
strcpy(PromptStr,St(MAskPswEcho));
#else
@@ -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);
@@ -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;
@@ -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;
Attachment:
signature.asc
Description: This is a digitally signed message part