Closed Bug 173433 Opened 20 years ago Closed 16 years ago

flawfinder warnings in libreg

Categories

(Core :: XPCOM, defect)

x86
Windows NT
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME
Future

People

(Reporter: morse, Assigned: dougt)

References

Details

I run flawfinder (http://www.dwheeler.com/flawfinder) on Mozilla 1.0.1 branch.

flawfinder found 11 warnings in libreg code (1113-1121). Go through
that list and for each warning:

* If it is false positive, comment here why it is not an issue
* If it is a real issue, make patch for it here and let's get them checked in

In addition to checking the branch, also check the trunk.

1113) modules/libreg/src/VerReg.c:603 [4] (buffer) sprintf: does not check for 
buffer overflows. Use snprintf or vsnprintf.

1114) modules/libreg/src/reg.c:3997 [3] (buffer) getenv: Environment variables 
are untrustable input if they can be set by an attacker. They can have any 
content and length, and the same variable can be set more than once.. Check 
environment variables carefully before using them.

1115) modules/libreg/src/vr_stubs.c:562 [3] (buffer) getenv: Environment 
variables are untrustable input if they can be set by an attacker. They can have 
any content and length, and the same variable can be set more than once.. Check 
environment variables carefully before using them.

1116) modules/libreg/src/vr_stubs.c:589 [3] (buffer) getenv: Environment 
variables are untrustable input if they can be set by an attacker. They can have 
any content and length, and the same variable can be set more than once.. Check 
environment variables carefully before using them.

1117) modules/libreg/src/vr_stubs.h:100 [4] (buffer) strcat: does not check for 
buffer overflows. Consider using strncat or strlcat.

1118) modules/libreg/src/vr_stubs.h:103 [4] (buffer) strcpy: does not check for 
buffer overflows. Consider using strncpy or strlcpy.

1119) modules/libreg/src/vr_stubs.h:105 [4] (buffer) sprintf: does not check for 
buffer overflows. Use snprintf or vsnprintf.

1120) modules/libreg/src/vr_stubs.h:207 [4] (buffer) sprintf: does not check for 
buffer overflows. Use snprintf or vsnprintf.

1121) modules/libreg/tests/interp.c:218 [5] (buffer) gets: does not check for 
buffer overflows. Use fgets() instead.
Blocks: 148251
1113)
This string is passed in by the embedder or is defaulted to "Mozilla".  The user
can not change change this string.  We could limit this string so some arbatary
length if required.

1114) 
This getenv is used to check if UNIX_GLOBAL_FLAG is set.  Its content or length
is never looked at.

1115) 
1116)
This getenv is used to indicate where the user's HOME directory is.  It content
and length is required.  I am not sure how to resolve this.  Please advise.

1117)
1118)
1119)
1120)

These are macros which wrap either the nspr PL_ function or Ansi function.  

Here are the real conserns:
VerReg.c:1103:    XP_SPRINTF(rcstr, "%d", refcount);
VerReg.c:1171:        XP_STRCPY( regbuf, REG_UNINSTALL_DIR );
VerReg.c:1181:            XP_STRCAT( regbuf, SHAREDSTR );
VerReg.c:1189:            XP_STRCAT( regbuf, gCurstr );
VerReg.c:1193:            XP_STRCAT( regbuf, "/" );
VerReg.c:1204:            XP_STRCAT( regbuf, UNINSTALL_NAV_STR );
VerReg.c:1211:            XP_STRCAT( regbuf, regPackageName );
VerReg.c:1450:                XP_STRCAT(regbuf, SHAREDFILESSTR);
VerReg.c:1501:                XP_STRCAT(regbuf, SHAREDFILESSTR);
VerReg.c:1564:                 XP_STRCAT(regbuf, SHAREDFILESSTR);
VerReg.c:1631:                XP_STRCAT(regbuf, SHAREDFILESSTR);
VerReg.c:1694:                XP_STRCAT(regbuf, SHAREDFILESSTR);
VerReg.c:1778:    XP_STRCPY( regbuf, REG_UNINSTALL_DIR );
VerReg.c:1781:        XP_STRCAT( regbuf, SHAREDSTR );
VerReg.c:1785:        XP_STRCAT( regbuf, gCurstr );
VerReg.c:178:                XP_STRCPY( regbuf, app_dir );
VerReg.c:179:                XP_STRCAT( regbuf, "/registry" );
VerReg.c:1829:        XP_STRCPY(temp, "/");
VerReg.c:1830:        XP_STRCAT(temp, regbuf);
VerReg.c:1832:        XP_STRCPY(regbuf, temp);
VerReg.c:248:            XP_STRCPY( curstr, VERSION_NAME );
VerReg.c:482:                XP_STRCPY(buf, tempBuf);
VerReg.c:598:            XP_STRCPY( regname, installation );
VerReg.c:603:                    sprintf( regname, "%s #%d", installation, nCopy );
VerReg.c:715:        XP_STRCPY( regbuf, programPath );
VerReg.c:716:        XP_STRCAT( regbuf, "registry" );
reg.c:3150:                                XP_STRCPY(buffer, tmpbuf);
reg.c:3805:    XP_STRCPY( tmpname, filename );
reg.c:3825:        XP_STRCPY(filename, tmpname);
reg.c:3947:    XP_STRCPY(tempfilename, reg->filename);
reg.c:3966:    XP_STRCPY(oldfilename, reg->filename);
vr_stubs.c:129:    XP_STRCPY(path, ".");
vr_stubs.c:133:        XP_STRCPY( path+pathlen, WIN_REG );
vr_stubs.c:187:        XP_STRCPY( path+pathlen, WIN_REG );
vr_stubs.c:201:            XP_STRCPY( path+pathlen, WIN_VERREG );
vr_stubs.c:648:          XP_STRCPY(def, home);
vr_stubs.c:649:          XP_STRCAT(def, DEF_REG);
vr_stubs.c:675:                XP_STRCPY(def, home);
vr_stubs.c:676:                XP_STRCAT(def, DEF_VERREG);
vr_stubs.c:726:          XP_STRCPY(def, settings);
vr_stubs.c:727:          XP_STRCAT(def, BEOS_REG);
vr_stubs.c:754:                XP_STRCPY(def, settings);
vr_stubs.c:755:                XP_STRCAT(def, BEOS_VERREG);


1121)
This is a test program.   



I will look at the rest in the next week or so.
I think we can generally ignore getenv warnings, but if you can think of a
situation where they might be an issue for us please let me know.
Target Milestone: --- → Future
VerReg.c:1103 safe, enough space to copy an int
VerReg.c:1171-11694 are safe, we have enough space and we check the lengths

VerReg.c:1778-1781 Is safe now, but if someone later changes the constant
strings or buffer sizes they may cause a problem here. I would advice making
this foolproof by checking that the lengths of the strings will fit in the space
available.

VerReg.c:1785 This is safe now, but again it would be better to protect this
from future changes.

VerReg.c:178-179 Safe, we allocate enough dynamic memory to copy

VerReg.c:1829-1832 This is probably unsafe. regbuf and temp are statically
allocated with the same size. We check the regbuf size above so that we will not
write more than what fits into it. However, _if_ we fill it to capacity, we will
then copy that PLUS one more character into temp, and overflow. We then try to
copy from the overflowed temp back to regbuf, and will overflow again (if we
didn't crash earlier).

VerReg.c:248: Is safe now, but we should future proof it.

VerReg.c:482: Safe, we check lengths

VerReg.c:598: Ugh, this became too tedious to check. I'd just recommend making
it foolproof by checking lengths.
VerReg.c:603: Same as above.

VerReg.c:715: Safe, we allocate enough dynamically.
VerReg.c:716: Safe, we allocate enough dynamically.

reg.c and ver_stubs.c still remain to be checked but I don't have the time right
now.
Target Milestone: Future → ---
-> future
Target Milestone: --- → Future
reg.c:3150 If nr_PathFromMacAlias() does not null terminate the string it
returns in case it does not fit into the supplied space we might create a char
buffer without ending null character which would blow the code elsewhere. Some
code that calls ns_ReadData() specifically sets the last character into null to
prevent this, so I suspect this might be relevant here too. Haven't looked at
the impls.

reg.c:3805Safe until we define RESURRECT_LATER. Assuming MAX_PATH is true, then
this is safe. Would be better to use strncpy though.
reg.c:3825 Safe until we define RESURRECT_LATER. Then this code would be a
potential crasher: there is a while loop that adds '~' to the filename until it
creates a unique filename but there are no checks on how long the filename can
grow, so once we get past MAX_PATH we should crash.
reg.c:3947 Again this RESURRECT_LATER stuff.
reg.c:3966 Again this RESURRECT_LATER stuff.
Closing all open flawfinder bugs as WORKSFORME because we now have much better tools that do the same (well, better) kind of analysis (Coverity, Klocwork).
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → WORKSFORME
Component: XPCOM Registry → XPCOM
QA Contact: doug.turner → xpcom
You need to log in before you can comment on or make changes to this bug.