I reviewed libvpd, git checkout 4d57eb358f6ae45555247b3f8e8160d8f16ab9fa; this shouldn't be considered a full security review.
Some of what I found look like serious reliability issues, perhaps even security issues; some look like standard bugs. I've also highlighted several examples of "assuming the cause of error", where an errno isn't checked in event of an error, and instead the code assumes to know the cause of an error. (This is more frustrating than dangerous.) None of the unpacking code should be called on content supplied by untrusted sources; callers should check owner. group, and permissions, of whichever files supplied the data, before using the unpacking routines. - HelperFunctions::str2chr() doesn't allocate space for NUL terminator, writes a NUL beyond the allocated space, this may even be exploitable. - HelperFunctions::parsePath() 'end' variable is used uninitialzed, it might cause segfaults when run - VpdRetriever::VpdRetriever() does not check system() return value for errors - HelperFunctions::execCmd() hands a string directly to popen(), all callers should be audited - DataItem::pack() does not validate that buf is large enough, is this safe? - HelperFunctions::parseString() stores size_t results into 'int', I'm afraid large strings and malformed strings (via string::npos) may cause string::substr to throw exceptions - DataItem::setValue() code to strip leading spaces probably doesn't work, it doesn't re-test val.at(i) after deleting the char at i, and it might also delete a space after a non-space, e.g. " f example" may be turned into "fexample", and " foo" may be turned into " foo". - unpack_system() doesn't check operations against length of buffer - unpack_system() doesn't check strdup() for NULL return - unpack_dataitem() doesn't check strdup() for NULL return - unpack_dataitem() assumes three back-to-back strings without verifying that it doesn't overstep bounds - VpdDbEnv::fetch() (both versions) fail to call sqlite3_finalize() if ret == NULL - fetch_component() sql injection possible via deviceID, I don't see any sanitization of inputs in call chains, this should use sqlite3_bind_* to prevent security and reliability problems - fetch_system() sql statement could be entirely static, no need for sprintf() Cases of ignoring useful error information: - VpdDbEnv::VpdDbEnv() dbExists - VpdRetriever::VpdRetriever() db == NULL - HelperFunctions::file_exists(), e.g. permissions failures or rlimit number of open files, etc. on a file that does exist Thanks -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1417608 Title: [MIR] ppc64-diag needed in minimal for hotplug capabilities To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/libservicelog/+bug/1417608/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
