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
