Minor fixes for GnuPG 1.2.3

[David Shaw]

On Fri, Dec 05, 2003 at 04:58:27PM +0100, Christian Biere wrote:
> David Shaw schrieb:
> > On Fri, Dec 05, 2003 at 08:33:52AM +0100, Christian Biere wrote:
> > 
> > >  		iobuf_put( a, '+' );
> > >  	    else {
> > >  		char numbuf[5];
> > > -		sprintf(numbuf, "%%%02X", *p );
> > > +		sprintf(numbuf, "%%%02X", (unsigned char)*p );
> > 
> > "*p" is a "byte *".  byte is already an unsigned char, no?
> 
> Ok, missed that. I've tried to ignore all such occurences where byte
> was used instead of char. Well, on RiscOS byte is just char according
> to include/types.h - I don't know whether char is signed or not on
> that platform.

I don't know either.  The comment in types.h suggests that it might be
a compiler thing?  Perhaps Stefan Bellon knows more. I hope this isn't
a problem on Riscos.  Just doing a grep for %02X and sprintf gives a
number of potential problems.

> > >  	    if(strcmp(line,"-----BEGIN PGP PUBLIC KEY BLOCK-----\n")==0)
> > >  	      {
> > > -		fprintf(output,line);
> > > +		fprintf(output,"%s",line);
> > >  		gotit=1;
> > >  	      }
> 
> > This isn't a bug.  You can't get to the bad printf unless line is
> > confirmed safe.
> 
> I know, read your comment on bugtraq. I still changed it because it's
> bad practice in general. If you use the protected form you don't have
> to understand (much of) the context to see that there's no problem.

Absolutely, and it was already changed in the code (it's changed in
the 1.2.4rc).  It's just my standard bug triage talking:

a) "is it really a bug?"
b) "is it exploitable?"
c) "can it be rewritten so that it is better anyway?"

> > >  	/* Nail that last space */
> > > -	searchkey[strlen(searchkey)-1]='\0';
> > > +	if (*searchkey)
> > > +	  searchkey[strlen(searchkey)-1]='\0';
> 
> > This is safe as well.  You can't get to here unless there is a key to
> > search for, so searchkey will never be zero length.
> 
> The while-loop above it suggested that it *might* be of zero-length. I
> didn't say it's a bug, it simply looked suspicious. So I'd rather
> plug in the cheap fuse.

I agree, and I'll make that change.  This qualifies as a group "c". ;)

David