[MIR] glamor-egl

It was suggested this will need a security audit, so assigning to ubuntu-security to look at.

I did a quick peek at this today. Packaging/build/etc seems fine. I wish there were tests though...

You'll need to subscribe a team to bugs. Who's looking after this package?

Still needs security audit too.

I've subscribed ubuntu-x to the bugs, as the team will take care of the package.

I reviewed glamor-egl version 0.5.1-0ubuntu1 as checked into saucy. This
should not be considered a full security audit, but rather a quick gauge
of code quality.

- glamor uses OpenGL primitives to provide corresponding accelerated 2d
  functionality without necessarily requiring hardware support for
  accelerated 2d, nor without requiring specific drivers to use any
  hardware support for 2d acceleration.
- Build-depends on xutils, x11proto-dri2, libdrm, xserver-xorg,
  libgl1-mesa, libpixman-1, libegl1-mesa, libgbm
- Runtime-depends on libEGL, libgbm, libGL
- Does not use cryptography
- Does not daemonize
- Does no networking
- Runs with elevated privileges, though does not manage privileges itself
- No initscripts
- No dbus
- No setuid
- No binaries in bin/
- No sudo fragments
- No cronjobs
- No test suite
- Clean build logs

- No subprocesses spawned
- Some unclean memory management, these two may even be vulnerabilities:
  - _glamor_poly_lines() unchecked malloc() return rects; n == 1 called from
    outside the library might be a problem
  - _pixman_region_init_clipped_rectangles() num_rects passed unchecked to
    malloc(), appears callable from outside the codebase with arbitrary
    arguments, might be a problem
- Several unchecked malloc() return values:
  - glamor_compile_glsl_prog() unchecked malloc() return value
  - glamor_egl_init() unchecked calloc() return value glamor_egl
  - glamor_compute_clipped_regions_ext() unchecked calloc() return value
    result_regions
  - __glamor_compute_clipped_regions() unchecked calloc() return value
    clipped_regions
  - glamor_composite_largepixmap_region() unchecked malloc() return value
    source_pixmap_priv

- Some code is not sufficiently defensive:
  - glamor_create_composite_fs() unguarded divide-by-zero possibility in
    relocate_texture (wh.x, wh.y; wh.xy divide is handled via RepeatNone)
  - glamor_create_composite_fs() unguarded divide-by-zero possibility in
    rel_sampler (wh.xy)
  I followed these call chains far enough to wonder if the necessary
  constraints could always be enforced.

- Logging looked safe
- Environment variable only used for debugging, looked safe
- Does not manage privileges
- Does not use cryptography
- Does not use networking
- Does not use tmp files
- Does not use webkit
- Does not use javascript

- glamor_pixmap_attach_fbo() switch case fall-through without annotation,
  probably safe but lack of annotation is annoying.

Packaging needs attention:
- Extraneous files debian/changelog.{REMOTE,BASE,LOCAL,BACKUP}.26867
- Patches without DEP-3 patch tags
- Files changed outside of patch system

The majority of the code is well-written and moderately defensive. The few
potential issues I've raised here are surprising blemishes given the
quality seen elsewhere in the library.

I'll ask upstream about the potential issues.

Before this can be promoted to main someone needs to fix the packaging
issues: extra changelog files, files changed outside the patch system,
DEP-3 patch tagging.

Security team ACK for promoting to main, conditional upon fixed packaging.

Thanks

thanks for the review, commenting on the packaging issues

> Packaging needs attention:
> - Extraneous files debian/changelog.{REMOTE,BASE,LOCAL,BACKUP}.26867

Git merge/clean fail, won't be on the next revision. At some point I'll learn to use git-buildpackage..

> - Patches without DEP-3 patch tags

reformatted the git patch to have Subject and uploaded.

> - Files changed outside of patch system

Again due to the git branch used, it doesn't have autogenerated files like the tarball does, and no idea why ReleaseNote is not on the tarball.. Pkg-xorg doesn't use source format 3.0(quilt) due to usability issues with git as shown here.

For proof that there are no extra changes outside debian/, this is the diffstat to the clean upstream branch from tag v0.5.1:

:: tjaalton@eldon:~/src/pkg-xorg/driver/glamor.git (ubuntu)> git diff upstream-unstable|diffstat
 README.source | 49 ++++++++++++++++++++++++++++++++
 changelog | 21 +++++++++++++
 compat | 1
 control | 62 +++++++++++++++++++++++++++++++++++++++++
 copyright | 31 ++++++++++++++++++++
 libglamor-dev.install | 4 ++
 libglamor0.install | 1
 patches/fix-fdo65964.diff | 28 ++++++++++++++++++
 patches/series | 1
 rules | 16 ++++++++++
 source/format | 1
 watch | 3 +
 xserver-xorg-glamoregl.install | 2 +
 13 files changed, 220 insertions(+)

:)

OK, seems fine then.

Override component to main
glamor-egl 0.5.1-0ubuntu3 in saucy: universe/x11 -> main
libglamor-dev 0.5.1-0ubuntu3 in saucy amd64: universe/libdevel/optional/100% -> main
libglamor-dev 0.5.1-0ubuntu3 in saucy armhf: universe/libdevel/optional/100% -> main
libglamor-dev 0.5.1-0ubuntu3 in saucy i386: universe/libdevel/optional/100% -> main
libglamor-dev 0.5.1-0ubuntu3 in saucy powerpc: universe/libdevel/optional/100% -> main
libglamor0 0.5.1-0ubuntu3 in saucy amd64: universe/libs/optional/100% -> main
libglamor0 0.5.1-0ubuntu3 in saucy armhf: universe/libs/optional/100% -> main
libglamor0 0.5.1-0ubuntu3 in saucy i386: universe/libs/optional/100% -> main
libglamor0 0.5.1-0ubuntu3 in saucy powerpc: universe/libs/optional/100% -> main
xserver-xorg-glamoregl 0.5.1-0ubuntu3 in saucy amd64: universe/x11/optional/100% -> main
xserver-xorg-glamoregl 0.5.1-0ubuntu3 in saucy armhf: universe/x11/optional/100% -> main
xserver-xorg-glamoregl 0.5.1-0ubuntu3 in saucy i386: universe/x11/optional/100% -> main
xserver-xorg-glamoregl 0.5.1-0ubuntu3 in saucy powerpc: universe/x11/optional/100% -> main
13 publications overridden.