← Back to branch summary

~ubuntu-security/ubuntu-cve-tracker/master

« back to all changes in this revision

Viewing changes to mir_reviews/lsvpd.0/audits/bug.txt

  • Committer: Steve Beattie
  • Date: 2019-02-19 06:18:27 UTC
  • Revision ID: sbeattie@ubuntu.com-20190219061827-oh57fzcfc1u9dlfk
The ubuntu-cve-tracker project has been converted to git.

Please use 'git clone https://git.launchpad.net/ubuntu-cve-tracker' to
get the converted tree.

Show diffs side-by-side

added added

removed removed

Lines of Context:
mir_reviews/lsvpd.0/audits/bug.txt
1
 
This review was made against lsvpd version 1.7.5-0ubuntu1; I think this
2
 
codebase would benefit from ASAN, valgrind, and consistent use of Lindent
3
 
or similar script around indent(1) or similar code formatter.
4
 
 
5
 
There is recurring "hiding" of errno and other specific errors that are
6
 
replaced by less-useful generic errors or incorrect errors. This can
7
 
lead to more difficult debugging of live systems. Sometimes potentially
8
 
incorrect assumptions are drawn based solely on an error return without
9
 
checking errno for potential remediation or accurate error messages. I
10
 
recommend the entire codebase be examined with this in mind, it is so
11
 
frequent.
12
 
 
13
 
There's more issues:
14
 
 
15
 
deleteList() while (!head) should be while (head) -- the current
16
 
 implementation never frees anything, NULL dereference and sigsegv if passed NULL
17
 
__lsvpdInit() does not memset the struct sigaction sigact to zero
18
 
ensureEnv() doesn't check if env is a directory, has expected ownership.
19
 
  has expected permissions; it returns 0 for most conditions and -1 only
20
 
  if the mkdir() fails. I suspect the function is entirely incorrect.
21
 
SysFSTreeCollector::getDevTreePath() misses fgets() error check
22
 
lsvpd_hexify() uses delete instead of delete[]
23
 
hexify() uses delete instead of delete[]
24
 
hexify() leaks ret if the input length is 0
25
 
device_scsi_sg_resp_len() returns uninitialized garbage if evpd isn't 0 or 1
26
 
RtasCollector::rtasGetVPD() may leak locCode via most branches of switch
27
 
RtasCollector::rtasGetVPD() may leak list via error return
28
 
RtasCollector::rtasGetVPD() does not check size += current->size; loop for
29
 
  overflow, is this a possibility?
30
 
FSWalk::fs_getDirContents() does not validate length of path_t, nor files
31
 
  in the directory, before copying their names into a fixed size buffer;
32
 
  probably the whole function should be re-written to use C++ strings
33
 
  instead
34
 
device_open() uses incorrect <= 0 error return from open(), could leave a
35
 
  device node created and opened
36
 
device_open() error handling is needlessly nested too deep, hard to read
37
 
device_open() calls device_close() if a sprintf() fails? is this correct?
38
 
device_close() doesn't actually close any devices, it only unlinks files
39
 
Why do device_close() and device_open() use /tmp? Is there no better
40
 
  place? Why not use udev-populated /dev/? Easily-guessable /tmp/names
41
 
  lead to denial-of-service possibilities.
42
 
Gatherer::~Gatherer() use-after-free, delete *i; ++i;
43
 
Gatherer::getComponentTree() may leak root or ret on error
44
 
FSWalk::fileScout() len is not reset to 0 on every newline
45
 
extractTagValue() fails to handle the case when a tag ends with :, as in
46
 
  the line "power management:"
47
 
archiveDB() reads an entire database into memory before compressing it,
48
 
  which requires the entire database to fit in RAM + swap, is this fine?