I (finally) finished this up today. I spot checked various static
buffers and functions that could have potential problems and overall
things seem good. Some areas of note:
1. unsafe use of wcscpy() and strcpy():
msn/xmlParser.cpp has:
#ifdef _XMLWIDECHAR
...
XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) { return (XMLSTR)wcscpy(c1,c2); }
...
#else
...
XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) { return (XMLSTR)strcpy(c1,c2); }
...
#endif
This is potentially dangerous if the input is not sanitized. A better, but
untested, implementation might be something like:
#ifdef _XMLWIDECHAR
...
XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) {
XMLSTR *dest = (XMLSTR)wcsncpy(c1,c2,sizeof(c1));
dest[sizeof(dest)-1] = L'\0';
return dest;
}
...
#else
...
XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) {
XMLSTR *dest = (XMLSTR)strncpy(c1,c2,sizeof(c1));
dest[sizeof(dest)-1] = '\0';
return dest;
}
...
#endif
2. Use of sprintf with a static buffer and no bounds checking:
At line 356 of msn/xmlParser.cpp in XMLNode::openFileHelper():
sprintf(message,
#ifdef _XMLWIDECHAR
"XML Parsing error inside file '%S'.\n%S\nAt line %i, column
%i.\n%s%S%s"
#else
"XML Parsing error inside file '%s'.\n%s\nAt line %i, column
%i.\n%s%s%s"
#endif
,filename,XMLNode::getError(pResults.error),pResults.nLine,pResults.nColumn,s1,s2,s3);
where 'filename' is an unsanitized parameter to
XMLNode::openFileHelper(). The good news is that openFileHelper()
doesn't appear to be used within libmsn. It would be best to fix this to
future-proof its use.
3. Does not check the return value of fread()
In XMLNode::parseFile() from xmlParser.cpp. A short read would result in
buf containing uncleared memory. This function appears to be only used
in XMLNode::openFileHelper(), which, as mentioned, doesn't appear to be
used by libmsn. As before, it would be best to check the return value
for possible future use of XMLNode::openFileHelper().
4. Contains an embedded md5 implementation. Noteworthy, but not a security
concern.
CONCLUSION: Please fix the use of strcpy() and wcscpy() as discussed in point
2. The XML file parsing could be skipped, but it would be nice if it were fixed
also.
** Changed in: libmsn (Ubuntu Jaunty)
Assignee: Jamie Strandboge (jdstrand) => (unassigned)
Status: In Progress => Confirmed
--
Include libmsn in main
https://bugs.launchpad.net/bugs/308060
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
