affconvert stack buffer overflow write vulnerability

[gsharpsh00ter]

0x01 Description

A stack buffer overflow write vulnerability was found in LIBAFFv3(and prior) toolkit affconvert. The vulnerability exists because of improper calculation of buffer size to copy. Due to the nature of this vulnerability, attackers may cause a denial-of-service status or potentially execute arbitrary code.

0x02 Analysis

The issue exists in affconvert.cpp line 671(maybe also many other positions). The code is like this:

669                 if(cc){
670                         /* Found an extension; copy over mine. */
671                         strlcpy(cc+1,ext,sizeof(outfile)-(cc-outfile));
672                 }
673                 else {
674                         /* No extension; make one */
675                         strlcat(outfile,".",sizeof(outfile));
676                         strlcat(outfile,ext,sizeof(outfile));
677                 }

Code in line 671 tries to copy outfile extension to the buffer that cc+1 indicates, but it misused sizeof(outfile) instead of strlen(outfile) to calculate the extension's length, thus results in a buffer over flow write status:

gdb-peda$ list
664					argc--;
665	
666					/* Copy over the filename and change the extension */
667					strlcpy(outfile,infile,sizeof(outfile));
668					char *cc = strrchr(outfile,'.'); // to strip off extension
669					if(cc){
670							/* Found an extension; copy over mine. */
671							strlcpy(cc+1,ext,sizeof(outfile)-(cc-outfile));
672					}
673					else {
gdb-peda$ print sizeof(outfile)
$10 = 0x401
gdb-peda$ print len
No symbol "len" in current context.
gdb-peda$ print strlen(outfile)
$11 = 0x2b
gdb-peda$ 

[sshock]

Actually, I think using sizeof() is correct here, as strlcpy() takes the size of the buffer as 3rd param.

But there is a problem. The only problem is an off-by-one. What it should be is:
strlcpy(cc+1,ext,sizeof(outfile)-(cc+1-outfile));

[sshock]

Fixed in bdb0ef1