[MIR] ppc64-diag needed in minimal for hotplug capabilities

I reviewed ppc64-diag version 2.6.7-0ubuntu2 from vivid. This shouldn't be
considered a full security audit but rather a quick gauge of
maintainability.

The collection of daemons and tools in this package read
platform-dependant information and report it for sysadmins to use.

The code quality was widely variable; almost all functions checked error
returns correctly and the handful of exceptions are common. However,
many complicated idioms are used in place of simpler functions such as
strdup(3), error handling often neglects to clean up acquired resources,
and unsafe shell-based execution mechanisms are used pervasively.

This code would benefit from removing almost every use of system(3) and
popen(3); some cases can be easily replaced with small wrappers around
fork(2) and execve(2) but some should be replaced with C-native functions
such as rename(2), unlink(2), and nftw(3).

Some of the programs process data from /var/log/messages or
/var/log/syslog; both of these files should be considered hostile as
it is trivial for potentially malicious local users to inject nearly
arbitrary contents into the logs. Because the programs run as root without
dropping privileges they must demonstrate strong data hygiene.

While the code is straightforward enough that the security team could fix
issues as they are raised, there's probably a week's work to handle this
(non-exhaustive) list of issues.

These issues may be security relevant or lead to unreliable behaviour:

- print_mtms_struct() write beyond the end of the model buffer, may not
  trip stack protection mitigations
- servevent() can leak entry on encl == NULL case
- safe_overwrite() can leak f on fputs(data.c_str(), f) == EOF case
- write_log() may not NUL-terminate dump_suffix, appears to read the data
  from a file on disk
- find_opal_errd_dir() can leak errd_path via two early returns
- process_elog() can leak buf, output_dir via early returns
- mem_drcindex_to_drcname() leaks dir in final return
- get_dt_status() leaks fp1, fp2 via early returns
- add_drconf_phandles() leaks fd via early return
- MatchVariant::compute_regex_text() may have shell injection via popen(),
  hard to verify that it's not an issue
- tail_message_file() shell injection; bad_chars misses many shell
  metachars. The least-effort patch involves quoting all ' chars in the
  input then wrapping it with ' chars. (The better approach is to use the
  array-based exec() with manual pipe-assembly, but this is error-prone
  code to write. Given how many places use popen() in this code, it would
  be a good investment.)
- get_dt_status() hardcoded /tmp/get_dt_files filename, unsafe on systems
  without symlink and hardlink restrictions -- seems like a lot of work to
  avoid using nftw(3)
- get_ses_indicator() uses popen() rather than pipe()+fork()+exec(), no
  protections against shell metachar injection via loc->dev or fru_loc
- set_ses_indicator() uses system() rather than fork()+exec(), no
  protections against shell metachar injection via loc->dev or fru_loc
- loc_code_device() may have shell injection via popen(); the parameter
  may come from argument parsing in main(), but if the encl_led tool is
  used by other tools,...

Adam has said he'll work with IBM on a timeline for fixing the above issues; security team ACK on promoting ppc64-diag to main on the condition that an SRU is prepared with the fixes when they are available.

Thanks

Hi Seth,

We are working internally to solve these problems and make a new release for this package.. Since ppc64-diag depends on some other packages, I understand that the other packages will need to be installed by default, and I am wondering if it might be a problem also:

These are the packages that need to be installed by default also:
 * lsvpd
 * servicelog
 * libvpd-2.2-2

And

 * powerpc-ibm-utils (Already part of the default installation)
 * librtas1 (dependency for powerpc-ibm-utils)
 * librtasevent1 (dependency for powerpc-ibm-utils)

Thanks for the catch, Breno. In my haste to file the bug, I didn't check and notice that all those extra deps were also in universe. This probably means we're cutting it too close for those packages to end up installed by default with the 14.04.2 installer, but we can still change the ubuntu-minimal metpackage to pull them in on upgrade after we've sorted out all the required reviews and promotions.

Seth , Adam,

 We have fixed all the security related issues and released v2.6.8 [1]. Could you please take a look?

[1] http://sourceforge.net/projects/linux-diag/files/ppc64-diag/v2.6.8/ppc64-diag-2.6.8.tar.gz/download

-Vasant

This review was made against lsvpd version 1.7.5-0ubuntu1; I think this
codebase would benefit from ASAN, valgrind, and consistent use of Lindent
or similar script around indent(1) or similar code formatter.

There is recurring "hiding" of errno and other specific errors that are
replaced by less-useful generic errors or incorrect errors. This can
lead to more difficult debugging of live systems. Sometimes potentially
incorrect assumptions are drawn based solely on an error return without
checking errno for potential remediation or accurate error messages. I
recommend the entire codebase be examined with this in mind, it is so
frequent.

There's more issues:

deleteList() while (!head) should be while (head) -- the current
 implementation never frees anything, NULL dereference and sigsegv if passed NULL
__lsvpdInit() does not memset the struct sigaction sigact to zero
ensureEnv() doesn't check if env is a directory, has expected ownership.
  has expected permissions; it returns 0 for most conditions and -1 only
  if the mkdir() fails. I suspect the function is entirely incorrect.
SysFSTreeCollector::getDevTreePath() misses fgets() error check
lsvpd_hexify() uses delete instead of delete[]
hexify() uses delete instead of delete[]
hexify() leaks ret if the input length is 0
device_scsi_sg_resp_len() returns uninitialized garbage if evpd isn't 0 or 1
RtasCollector::rtasGetVPD() may leak locCode via most branches of switch
RtasCollector::rtasGetVPD() may leak list via error return
RtasCollector::rtasGetVPD() does not check size += current->size; loop for
  overflow, is this a possibility?
FSWalk::fs_getDirContents() does not validate length of path_t, nor files
  in the directory, before copying their names into a fixed size buffer;
  probably the whole function should be re-written to use C++ strings
  instead
device_open() uses incorrect <= 0 error return from open(), could leave a
  device node created and opened
device_open() error handling is needlessly nested too deep, hard to read
device_open() calls device_close() if a sprintf() fails? is this correct?
device_close() doesn't actually close any devices, it only unlinks files
Why do device_close() and device_open() use /tmp? Is there no better
  place? Why not use udev-populated /dev/? Easily-guessable /tmp/names
  lead to denial-of-service possibilities.
Gatherer::~Gatherer() use-after-free, delete *i; ++i;
Gatherer::getComponentTree() may leak root or ret on error
FSWalk::fileScout() len is not reset to 0 on every newline
extractTagValue() fails to handle the case when a tag ends with :, as in
  the line "power management:"
archiveDB() reads an entire database into memory before compressing it,
  which requires the entire database to fit in RAM + swap, is this fine?

Thanks

Seth,
  Thanks for the reviewing lsvpd package.. I did went through lsvpd/libvpd package sometime back and we found some of these issues. Currently we are working on fixing them. We will go through this list as well and fix them as soon as possible.

Between its nice if you can review other packages as well. I think its better to review upstream package rather than what we have in Ubuntu ... as I know we have few issues across packages!

libvpd : git://git.code.sf.net/p/linux-diag/libservicelog linux-diag-libvpd (next branch)
libservicelog : git://git.code.sf.net/p/linux-diag/libservicelog linux-diag-libservicelog (next branch)
servicelog: git://git.code.sf.net/p/linux-diag/libservicelog linux-diag-servicelog (next branch)
ppc64-diag: git://git.code.sf.net/p/linux-diag/libservicelog linux-diag-ppc64-diag (next branch) <- Hopefully we have addressed all the issues you reported earlier.

-Vasant

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

I reviewed libservicelog commit ebf842c43e0ad16e975db1f4a10b4f83ccbfdf72;
this sholdn't be considered a full security audit.

Nearly everything I found is an instance of one of two issues:

- Using snprintf() rether than sqlite3_bind_*() and prepared statements --
  sometimes the parameters are supplied from static strings elsewhere in
  the program and can't be supplied via sqlite3_bind_*() functions, but
  many parameters may come from elsewhere.

  I think the code would be improved to clearly delineate what comes from
  within the program -- and can safely be constructed via snprintf() --
  and what comes from outside the program and should be handled with
  prepared statements. This may look like a complication but I think the
  clear difference between the two types of parameters would improve the
  program clarity.

- snprintf() is never checked for error returns; when populating
  log->error or slog->error, it doesn't really matter -- a best-effort is
  all that's called for -- but many times it's being used with potentially
  untrusted inputs to construct SQL queries. I also found one case where
  the buffer isn't large enough to handle the query if constructed with
  fairly large -- yet legal -- ids. (A prepared statement would probably
  be the better fix.)

Here's the functions that seemed likely to suffer the most from lacking
prepared statements:

- servicelog_event_log() doesn't use sqlite3_bind_*() functions to prevent
  sql injection
- servicelog_event_log() doesn't check snprintf() error returns,
  sqlite3_bind_*() functions wouldn't require snprintf() steps
- servicelog_event_query(), doesn't check snprintf() error returns,
  doesn't use sqlite3_bind_*() functions to prevent sql injection
- servicelog_event_repair() doesn't check snprintf() error returns, buffer
  isn't long enough (I came up with 86 chars needed)
- delete_row() doesn't check snprintf() error return, doesn't use
  sqlite3_bind_*() functions to prevent sql injection
- servicelog_event_delete() doesn't check snprintf() error returns
- insert_addl_data_os() doesn't check snprintf() error returns, doesn't
  use sqlite3_bind_*() functions to prevent sql injection
- insert_addl_data_enclosure() doesn't check snprintf() error returns,
  doesn't use sqlite3_bind_*() functions to prevent sql injection
- servicelog_repair_log() doesn't check snprintf() error returns, doesn't
  use sqlite3_bind_*() functions to prevent sql injection
- servicelog_notify_log() doesn't check snprintf() error returns,
  doesn't use sqlite3_bind_*() functions to prevent sql injection
- servicelog_notify_update() doesn't check snprintf() error returns,
  doesn't use sqlite3_bind_*() functions to prevent sql injection

And some small miscellaneous bits:

- format_text_to_insert() isn't needed if it can be replaced by
  sqlite3_bind_*() functions
- servicelog_truncate() consider adding VACUUM call

I'd like to see sqlite3_bind_*() used whenever string inputs aren't
static.

Thanks

I reviewed servicelog git version 4cb0d4987ad00d4b0d5359ddb495fd20fcd7e01a;
this shouldn't be considered a full security audit.

- main() in log_repair_action.c doesn't check fread() error returns, a
  zero return could cause out-of-buffer write
- main() in log_repair_action.c doesn't check strdup() error returns
- main() in log_repair_action.c uses popen("date ..."), is this ever
  called from other programs? It's fine if it's just for administrators,
  but if other tools can use it, it ought to be fixed.

Cases of "assuming the error":
- main() in log_repair_action.c, stat() error can happen for more reasons
  than just ENOENT
- main() in log_repair_action.c, S_IXUSR test does not match execve() tests

Slightly odd:
- main() in log_repair_action.c suggests to mail support when a wrong
  option is provided, rather than just print the usage.

Thanks

Thanks for your patience with these reviews.

We would like fixes for the issues I've raised above as soon as practical; I'm sorry that this feedback is too late to make the 14.04.3 images, but hopefully we can have updates available near the time of release.

The smoothest way forward would be new releases, or cherry-picked patches, that simply address these issues without adding features. The wiki page at https://wiki.ubuntu.com/StableReleaseUpdates describes the style that we prefer.

Thanks

Status changed to 'Confirmed' because the bug affects multiple users.

Seth,

We have addressed most of the concerned raised by you. Can you please review below packages and let us know if you have any other concerns to take this packages to default ISO.

libservicelog v1.1.16 https://sourceforge.net/projects/linux-diag/files/libservicelog/1.1.16/libservicelog-1.1.16.tar.gz/download
servicelog v.1.14 https://sourceforge.net/projects/linux-diag/files/servicelog/1.1.14/servicelog-1.1.14.tar.gz/download
libvpd v2.2.5 http://sourceforge.net/projects/linux-diag/files/libvpd/2.2.5/libvpd-2.2.5.tar.gz/download
lsvpd v1.7.6 http://sourceforge.net/projects/linux-diag/files/lsvpd-new/1.7.6/lsvpd-1.7.6.tar.gz/download
ppc64-diag 2.7.0 http://sourceforge.net/projects/linux-diag/files/ppc64-diag/v2.7.0/ppc64-diag-2.7.0.tar.gz/download

-Vasant

Of the listed packages, we have the following in xenial today:
  ppc64-diag 2.7.0
  lsvpd 1.7.6

libservicelog, servicelog, libvpd are still at earlier versions and would need to be upgraded to include these fixes.

We have request to update all the packages ..But I tagged upstream version just after Xenial code cutoff date..These are Power system specific packages and we take care of validating. Hence if possible can you please update these packages?

libservicelog : https://bugs.launchpad.net/ubuntu/+source/libservicelog/+bug/1522418
servicelog : https://bugs.launchpad.net/ubuntu/+source/servicelog/+bug/1522419
libvpd : https://bugs.launchpad.net/ubuntu/+source/libvpd/+bug/1521679

-Vasant

libservicelog 1.1.16 and servicelog 1.1.14 have now been reviewed and uploaded to xenial.

libvpd 2.2.5 has now also been uploaded.

Steve,

Thanks a lot.

-Vasant

I understand that these packages are currently blocked on a security team re-review following the inclusion of the upstream updates in the archive. Tentatively assigning back to Seth.

Vasant, Breno, thank you and your teams for working on these issues. I didn't read these as closely as you deserve but what I saw impressed me. Well done.

Security team ACK for promoting libservicelog libvpd lsvpd ppc64-diag servicelog to main.

Thanks Seth. What are the next steps here? Are we planning to have these packages migrated to 'main' in 16.10 or future releases?

Breno, yes, this is now in Adam's hands to make the necessary changes to the installer and seeds for inclusion of these packages. It is also a candidate for retroactively including in main for 16.04 provided we have the right versions there.

Hi Steve, I am glad to hear that. Thank you!

ppc64-diag has now been seeded.

Override component to main
servicelog 1.1.14-1 in yakkety: universe/misc -> main
servicelog 1.1.14-1 in yakkety powerpc: universe/misc/extra/100% -> main
servicelog 1.1.14-1 in yakkety ppc64el: universe/misc/extra/100% -> main
3 publications overridden.

Override component to main
libservicelog 1.1.16-2 in yakkety: universe/libdevel -> main
libservicelog-1.1-1 1.1.16-2 in yakkety powerpc: universe/libdevel/extra/100% -> main
libservicelog-1.1-1 1.1.16-2 in yakkety ppc64el: universe/libdevel/extra/100% -> main
libservicelog-dev 1.1.16-2 in yakkety powerpc: universe/libdevel/extra/100% -> main
libservicelog-dev 1.1.16-2 in yakkety ppc64el: universe/libdevel/extra/100% -> main
5 publications overridden.

Override component to main
lsvpd 1.7.7-1 in yakkety: universe/misc -> main
lsvpd 1.7.7-1 in yakkety powerpc: universe/misc/extra/100% -> main
lsvpd 1.7.7-1 in yakkety ppc64el: universe/misc/extra/100% -> main
3 publications overridden.

Override component to main
libvpd 2.2.5-2 in yakkety: universe/libs -> main
libvpd-2.2-2 2.2.5-2 in yakkety powerpc: universe/libs/extra/100% -> main
libvpd-2.2-2 2.2.5-2 in yakkety ppc64el: universe/libs/extra/100% -> main
libvpd-dev 2.2.5-2 in yakkety powerpc: universe/libdevel/extra/100% -> main
libvpd-dev 2.2.5-2 in yakkety ppc64el: universe/libdevel/extra/100% -> main
5 publications overridden.

component-mismatches reminds us that iprutils is also a dependency of lsvpd and will need to be MIRed. opening a bug task.

Package makes sense in main as a dependency of lsvpd, and as a component of hardware support in its own right. Meets the MIR requirements. There is no upstream test suite. The package ships three daemons for hardware support , which will be enabled by default when installing the package - but probably should not be (or should not be pulled in automatically as a dependency) on systems without the relevant hardware.

http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=iprutils shows no results. http://people.canonical.com/~ubuntu-security/cve/universe.html shows no results.

The issue of service auto-starting has been addressed by Red Hat here: https://rhn.redhat.com/errata/RHBA-2015-0385.html

Given the lateness of this change in the 16.10 cycle, the daemons being unconditionally started (which AIUI is being worked on, but not fixed in the current package), and the status of bug #1537116, I will upload lsvpd to re-drop the dep on iprutils for now.

Please could somebody test this upload to unstable?

iprutils (2.4.13.1-2) unstable; urgency=medium

  * When using systemd, only start services when the hardware is present and
    after the ipr driver is loaded (based on a Fedora patch; see LP
    #1417608).

 -- Colin Watson <email address hidden> Sun, 09 Oct 2016 18:40:51 +0100

Colin,

I will ask my test team to verify iprutils ASAP.

-Vasant

Not very clear on "unstable" repo path for 16.10.
Can someone post the "unstable" complete repo path that can be added to sources.list.

Hi,

Tried adding "deb http://ddebs.ubuntu.com/ yakkety-proposed main restricted universe multiverse" still getting iprutils version as "2.4.13.1-1".

Thanks,
Pavithra

Kalpana, Pavithra, "unstable" is from Debian. Launchpad mirrors Debian's packages, which can be retrieved by hand if that's easier.

For e.g. the iprutils source package in Ubuntu:
https://launchpad.net/ubuntu/+source/iprutils
The corresponding source packages in Debian are mirrored to:
https://launchpad.net/debian/+source/iprutils

The corresponding apt sources should be something like:
deb http://ftp.us.debian.org/debian/ sid main
deb-src http://ftp.us.debian.org/debian/ sid main

Be careful mixing Debian and Ubuntu sources if you test this on an Ubuntu system.

Thanks

Hi,

Tested iprutils of version 2.4.13.1-2 on ubuntu 16.10, no issues are observed on Tuleta machine. But devices are not listed on BMC based systems [Habanero, Garisson]. Please let me know if this is expected behavior.

On Tuleta
----------

                             Display Hardware Status

Type option, press Enter.
  1=Display hardware resource information details

OPT Name PCI/SCSI Location Description Status
--- ------ ------------------------- ---------------------------- -------------
           0001:04:00.0/0: PCI-E SAS RAID Adapter Operational
    sdg 0001:04:00.0/0:0:9:0 Physical Disk Active
    sdb 0001:04:00.0/0:2:1:0 RAID 0 Array Optimized
           0001:04:00.0/0:0:1:0 RAID 0 Array Member Active
    sde 0001:04:00.0/0:2:4:0 RAID 0 Array Optimized
           0001:04:00.0/0:0:4:0 RAID 0 Array Member Active
    sda 0001:04:00.0/0:2:0:0 RAID 0 Array Optimized
           0001:04:00.0/0:0:0:0 RAID 0 Array Member Active
    sdd 0001:04:00.0/0:2:3:0 RAID 0 Array Optimized
           0001:04:00.0/0:0:3:0 RAID 0 Array Member Active
    sdc 0001:04:00.0/0:2:2:0 RAID 0 Array Optimized
           0001:04:00.0/0:0:2:0 RAID 0 Array Member Active
    sdf 0001:04:00.0/0:2:5:0 RAID 0 Array Optimized
           0001:04:00.0/0:0:5:0 RAID 0 Array Member Active
                                                                        More...
e=Exit q=Cancel r=Refresh t=Toggle f=PageDn b=PageUp

On Habanero
------------

                             Display Hardware Status

Type option, press Enter.
  1=Display hardware resource information details

OPT Name PCI/SCSI Location Description Status
--- ------ ------------------------- ---------------------------- -------------

No devices found

e=Exit q=Cancel r=Refresh t=Toggle

Thanks,
Pavithra

@Pavitra,

If you don't have device it will not be listed. I think its expected behaviour on BMC based system.

-Vasant

setting the MIR status to incomplete

> setting the MIR status to incomplete

Why? What information are you waiting for that's not covered in https://bugs.launchpad.net/ubuntu/+source/ppc64-diag/+bug/1417608/comments/34 ?

iprutils:
 - the service auto-starting is implemented in 2.4.15.1-1
 - no open issues in Debian and Ubuntu
 - according to the security team no security review necessary
 - minor nit: copyright years in debian/copyright are outdated

Override component to main
iprutils 2.4.15.1-1 in artful: universe/admin -> main
iprutils 2.4.15.1-1 in artful ppc64el: universe/admin/extra/100% -> main
iprutils-udeb 2.4.15.1-1 in artful ppc64el: universe/debian-installer/extra/100% -> main
3 publications overridden.

Thanks; I've fixed the copyright years nit in iprutils 2.4.15.1-2, just
uploaded to unstable.