security team audit of libhybris

Bug #1223586 reported by Seth Arnold on 2013-09-10
258
This bug affects 1 person
Affects Status Importance Assigned to Milestone
libhybris (Ubuntu)
Critical
Unassigned
Saucy
Critical
Unassigned

Bug Description

Since https://bugs.launchpad.net/ubuntu/+source/libhybris/+bug/1188213 is
already labeled "fix-committed", this is a new bug to address issues
raised during the MIR audit.

Here's the comment I filed on the other bug report:

I reviewed libhybris version 0.1.0+git20130606+c5d897a-0ubuntu21 from
saucy. This should not be considered a complete security audit, but
instead a quick gauge of code cleanliness.

- The package provides a shim between Android services and glibc standard
  C library, it hooks a large number of system service functions to
  provide the subtly different semantics expected by Android applications.
- Build-depends upon autotools, gcc, g++, quilt, pkg-config, libgles2-mesa-dev
- Does not itself use cryptography
- Limited use of networking, for the debugger shim
- Does not itself daemonize
- Can run with elevated privileges
- No initscripts
- No dbus services
- No setuid/setgid executables
- Provides several binaries:
  ./usr/bin/test_recorder
  ./usr/bin/test_ui
  ./usr/bin/test_camera
  ./usr/bin/test_egl
  ./usr/bin/test_media
  ./usr/bin/test_sf
  ./usr/bin/test_audio
  ./usr/bin/test_input
  ./usr/bin/test_glesv2
  ./usr/bin/test_sensors
  ./usr/bin/test_gps
  ./usr/bin/test_lights
  ./usr/bin/setprop
  ./usr/bin/getprop
- No sudo fragments
- No cron jobs
- Build logs are troubling:
  - many instances of pointer / integer size mismatch warnings which mean
    this code will require effort to port to a 64 bit environment.
  - hybris/libsync/sync.c calls free(3) without #include <stdlib.h>
    and thus uses the wrong prototype. (C assumes functions without
    prototypes are passing integer arguments, not a pointer.)
  - hybris/egl/egl.c eglGetProcAddress uses ugly return type hack,
    warning message "return from incompatible pointer type" looks
    like the ugly hack hasn't been completely used correctly
  - hybris/common/hooks.c, pthread_attr_getstackaddr and
    pthread_attr_setstackaddr are deprecated, should be replaced with
    pthread_attr_getstack and pthread_attr_setstack.

Severe-looking problem:

- hybris/common/ics/linker.c nothing sets program_is_setuid variable
  This means the environment scrubbing is not performed, fds 0, 1, 2
  aren't opened to /dev/null if they were closed, and LD_LIBRARY_PATH and
  LD_PRELOAD get to freely modify the address space of the setuid
  executable.

Someone needs to investigate if these are problems:

- hybris/common/*/linker.c The fds 0, 1, 2 are only nullified for programs
  with program_is_setuid set. Since programs tend to assume these fds are
  always open at exec(), the linker should probably set them open
  regardless of program_is_setuid value.
- hybris/egl/egl.c _init_androidegl() uses environment variables for
  loading LIBEGL and LIBGLESV2, no safety checks in place
- hybris/egl/ws.c _init_ws() uses environment variable EGL_PLATFORM for
  loading elgplatform_%s.so, no safety checks in place
- hybris/common/hooks_shm.c _hybris_shm_init() uses 0660, is this
  correct? Why not 0600? This code performs no ownership checks, another
  user could create the shm segment, mode 0666, and trouble ensues.
- hybris/common/hooks_shm.c _hybris_shm_fd is left opened beyond needed
  lifetime

The 64-bit-unclean code is not an issue at the moment, though this design
decision may represent a significant technical debt that must be paid in
the future. Ideally, we would not need this library by the time 64 bit
targets are available.

libhybris as it stands should not be in main, though it is close. The
conditions for including libhybris:

The ics/linker.c needs program_is_setuid to be set properly.

All the linkers need to nullify fds on all executables, not just
setuid/setgid/etc executables, unless we can demonstrate there are actual
problems with this approach.

Someone more familiar with the code needs to explain the ramifications
of unchecked library loading via the LIBEGL, LIBGLESV2, EGL_PLATFORM
environment variables. These variables might need to be specially handled
in the linker for safety.

Someone more familiar with the code needs to inspect the shared memory
segment handling and either explain why the relaxed 0660 mode is fine
-- and why there are no owner checks for the already-created case --
or address these issues.

Someone more familiar with the code needs to inspect the _hybris_shm_fd
file descriptor and either explain why the fd cannot be closed (and use
_hybris_shm_data variable instead) or explain why it is not an issue,
or address this issue.

Someone more familiar with the code needs to inspect the libsync/sync.c,
egl/egl.c, and common/hooks.c warning messages and either correct the
warnings or explain why they cannot be corrected.

Please address the above issues before we ship this package.

Thank you

Changed in libhybris (Ubuntu Saucy):
importance: Undecided → Critical
Anders (eddiedog988) on 2014-03-13
Changed in libhybris (Ubuntu):
status: New → Confirmed
Changed in libhybris (Ubuntu Saucy):
status: New → Confirmed
Rolf Leggewie (r0lf) wrote :

saucy has seen the end of its life and is no longer receiving any updates. Marking the saucy task for this ticket as "Won't Fix".

Changed in libhybris (Ubuntu Saucy):
status: Confirmed → Won't Fix
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers