I reviewed xdelta3 version 3.0.11-dfsg-1ubuntu1 as checked into zesty.
This should not be considered a full security audit but rather a quick
gauge of maintainability.

There is one CVE in our database; the patch mingled together the
functional change with twenty times more whitespace changes and a test
for the functional change. This isn't ideal but at least the patch was
labeled to fix a buffer overflow.

xdelta3 provides a binary-patch style of interface that is immensely
flexible. It can either be used as very manual style of compression
tool -- one of the inputs can be a dictionary of 'phrases' that would
be discovered automatically by other compression tools -- or it could be
used as a way to distribute a small set of changes against a fixed file.
It does so by building a program to interpret to emit the final results.

No dependencies.

xdelta3 is written in C, and heavily exploits pointer arithmetic, memcpy()
(but oddly enough not memmove() despite comments indicating that there may
be overlaps), and array indexing operations. When a native Rust version is
available please consider switching to it. (The existing VCDIFF crates on
crates.io are bindings for Google's VCDIFF implementation open-vcdiff.)

- xdelta3 provides a command line interface and header files that can be
  used to bake xdelta3 into other software (not via library use but by
  including the headers directly).
- No build deps
- No daemons
- No pre/post inst/rm scripts
- No init scripts
- No dbus services
- No setuid executables
- /usr/bin/xdelta3 binary
- No sudo fragments
- No udev rules
- The test suite is built into the executable and distributed in the
  binary. We should DISABLE these tests as they are very low quality.
- No cron jobs
- One common warning in the build logs "invalid suffix on literal"

- Subprocesses are spawned to support optional compression; execlp() is
  used directly, and while the execution itself looks safe, the
  compressors it executes can have their execution modified via
  environment variables. This is probably not a security issue but could
  be a cause of unreliable behaviour or support trouble. I strongly
  recommend executing xdelta3 with a whitelist of a handful of needed
  environment variables.

- Subprocesses are also spawned in the test suite. This code may be
  suitable for tests but is unacceptable in the executable or in the
  address space of anything that may choose to embed xdelta3. Ideally we'd
  build two executables in the package: one to run tests, and one to ship
  to users in binary packages, and the test binary would not be packaged
  in any binary packages. If this is too much work please just disable
  the tests at build time.

- Memory management has some potential integer multiplication overflows.

- Memory management is very complicated. Bugs have been found before and
  probably bugs still exist. We should fuzz xdelta3 extensively before we
  rely upon it. I'd love to see a re-write in Rust.

- Files are written to, under control of the command line, using simple
  fopen(3) calls.

- Logging is extensive, looked safe.

- The XDELTA environment variable handling is very complicated; I don't
  understand what the code does with this variable. It could be used to
  adjust the execution of xdelta3 in ways that are probably not security
  issues but may be unreliable or unexpected.

- No privileged operations
- No encryption
- Inputs moderately sanitized; fuzzing may be productive
- No privileged portions of code
- No temporary files
- No WebKit
- No JavaScript
- No PolicyKit
- Clean cppcheck


There are eight patches in debian/patches but only three patches in
debian/patches/series. This should be fixed.

Due to unique code layout decisions, the self-test code appears built into
the xdelta3 executable. The self-test code quality is very poor. (This is
common to most test code but most test code doesn't get included into the
shipped binaries.) We should _disable_ build-time tests so that the
low-quality code is not available for abuse.

__xd3_alloc_func() has an unchecked integer multiplication overflow. I
do not know if this is reachable via untrusted inputs but calloc()
should be used instead.

main_alloc() has an unchecked integer multiplication overflow. I do not
know if this is reachable via untrusted inputs but calloc() should be
used instead.

setup_environment() has an unchecked integer multiplication overflow.
It's controlled by the size of the XDELTA environment variable and thus
may not be possible to actually trip in practice. calloc() could be used
instead.

External compression just executes e.g. gzip from the filesystem. This
gzip will interpret the GZIP environment variable which may have
unintended unexpected consequences. We should ensure to set exactly the
minimum set of environment variables that xdelta3 needs to run properly.
While it's hard to say the environment variables of a program shouldn't be
completely trusted this has the potential to be extremely surprising.

Can main_apphead_string() be tricked into trouble if passed an 'x' like
"/dev/foo/"? The return value would point at the NUL at the end of the
string; this gets handed to at least strlen() among other functions.


The security team ACK for promoting xdelta3 to main is conditional upon:
- The self-test code is disabled from the shipping /usr/bin/xdelta3
- The debian/patches/ directory and series is cleaned

Thanks

https://github.com/jmacd/xdelta/issues/232 integer overflows
https://github.com/jmacd/xdelta/issues/233 main_apphead_string()


** Bug watch added: github.com/jmacd/xdelta/issues #232
   https://github.com/jmacd/xdelta/issues/232

** Bug watch added: github.com/jmacd/xdelta/issues #233
   https://github.com/jmacd/xdelta/issues/233

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1647222

Title:
  [MIR] xdelta3

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/xdelta3/+bug/1647222/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Reply via email to