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:
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.
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: wcscpy( c1,c2); } strcpy( c1,c2); }
#ifdef _XMLWIDECHAR
...
XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) { return (XMLSTR)
...
#else
...
XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) { return (XMLSTR)
...
#endif
This is potentially dangerous if the input is not sanitized. A better, but untested, implementation might be something like: wcsncpy( c1,c2,sizeof( c1));
dest[ sizeof( dest)-1] = L'\0'; strncpy( c1,c2,sizeof( c1));
dest[ sizeof( dest)-1] = '\0';
#ifdef _XMLWIDECHAR
...
XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) {
XMLSTR *dest = (XMLSTR)
return dest;
}
...
#else
...
XMLSTR _tcscpy(XMLSTR c1, XMLCSTR c2) {
XMLSTR *dest = (XMLSTR)
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,
,filename, XMLNode: :getError( pResults. error), pResults. nLine,pResults. nColumn, s1,s2,s3) ;
#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
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.