Review for Source Package: valkey
[Summary]
The essence of the review result from the MIR POV is that valkey is
well-packaged with good systemd hardening, comprehensive build-time tests,
active upstream and Debian/Ubuntu maintenance, and a clean debian/rules. The
main concerns are the significant number of embedded/statically-linked
libraries that increase the security maintenance burden, and its network-facing
nature with complex protocol parsing, which warrant a security team review.
MIR team ACK under the constraint to resolve the below listed required
TODOs and as much as possible having a look at the recommended TODOs.
This does need a security review, so I'll assign ubuntu-security.
Particularly, the embedded packages and the data parsing nature of this
package.
List of specific binary packages to be promoted to main: valkey-
sentinel, valkey-server, valkey-tools
Notes:
Recommended TODOs:
Can the initial MIR submitter please comment on the feasibility of forgoing the
embedded packages in valkey and using the Ubuntu Archive versions instead? The
embedded packages in valkey present additional security maintenance and would
be nice to avoid where possible.
[Rationale, Duplication and Ownership]
There is no other package in main providing the same functionality.
A team is committed to own long term maintenance of this package - Ubuntu Server
The rationale given in the report seems valid and useful for Ubuntu
[Dependencies]
- no other runtime Dependencies to MIR due to this - the package valkey-tools
is in Universe but included in this package as noted in the initial MIR
submission
- no other build-time Dependencies with active code in the final binaries
to MIR due to this
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring
more tests now.
Problems: None
[Embedded sources and static linking]
- Embedded source is present. The deps/ directory contains the following
embedded libraries that are statically linked into the valkey binaries:
- lua 5.1 (modified with security patches, custom hash, and compiled-in
copies of lua_cjson, lua_bit, lua_cmsgpack, lua_struct)
- libvalkey (fork of hiredis, not separately packaged in the archive)
- linenoise (not packaged in the archive)
- hdr_histogram (C version not packaged in the archive)
- fpconv (not packaged in the archive)
- fast_float (available as libfast-float-dev in universe, but embedded
copy is used)
- Archive equivalents exist for lua5.1, lua-cjson, lua-bitop, and
fast_float but are not used.
- does not have unexpected Built-Using entries (C package, not Go/Rust)
- not a go package, no extra constraints to consider in that regard
- not a rust package, no extra constraints to consider in that regard
- debian/copyright documents all embedded sources and their licenses.
Problems:
- The security team should be aware that CVEs in the embedded copies
(especially lua, libvalkey, hdr_histogram) require patching the valkey
source package directly rather than updating a shared library. Can the
submitter please comment on the feasibility of forgoing the embedding and using
the Ubuntu Archive version of these packages?
[Security]
- History of CVEs is notable: as a fork of Redis (~75 CVEs), valkey
inherits that history. Valkey itself has 4 CVEs so far, all medium
severity.
- Runs a daemon as a dedicated system user "valkey" (not root). The user
is created via adduser in postinst as a system user with
--home /var/lib/valkey.
- does not use webkit1,2
- does not use lib*v8 directly
- Does parse data formats from potentially untrusted sources: RESP
protocol over network, RDB/AOF persistence files, Lua scripts,
cluster bus protocol. This is a key part of its attack surface.
- Does expose an external endpoint: TCP port 6379 by default, plus
optional TLS port and Unix socket. Default config binds to
127.0.0.1/::1 only with protected-mode enabled.
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam) — it has its own
ACL system with SHA256-hashed passwords for client authentication
- does not deal with security attestation (secure boot, tpm, signatures)
- Does deal with cryptography: TLS support via OpenSSL for encrypted
client/server and replication connections; SHA256 for ACL password
hashing.
- Makes good use of systemd isolation features: the generated service
files include PrivateTmp, PrivateDevices, ProtectHome,
ProtectSystem=strict, NoNewPrivileges, PrivateUsers,
MemoryDenyWriteExecute, RestrictAddressFamilies, SystemCallFilter,
NoExecPaths, CapabilityBoundingSet= (empty), and more.
- No AppArmor profile is shipped.
- No use of setuid/setgid in source code.
Problems:
- This is a network-facing daemon that parses complex data formats
(RESP protocol, RDB files, Lua scripts) from potentially untrusted
clients. A security review is recommended.
[Common blockers]
- does not FTBFS currently
- does have a test suite that runs at build time
- test suite failures will fail the build upon error.
- Build-time tests include: runtest (unit tests with TLS, skipping
memefficiency and maxmemory), runtest-cluster (with TLS, 30min
timeout), and runtest-sentinel (skipped on armhf due to memory
constraints on 32-bit).
- does have a non-trivial test suite that runs as autopkgtest
- 5 autopkgtests: valkey-cli smoke test, benchmark run, valkey-check-aof
usage check, valkey-check-rdb against a real dump, and cjson Lua
extension validation.
CLI, and key subsystems.
- This does not need special HW for build or test
- no new python2 dependency
- Not a Python or Go package
Problems: None
[Packaging red flags]
OK:
- Ubuntu does carry a delta, but it is reasonable and maintenance under
control
- symbols tracking not applicable for this kind of code.
- debian/watch is present and looks ok (if needed, e.g. non-native)
- Upstream update history is good - several releases in the last year
- Debian/Ubuntu update history is good
- the current release is packaged
- promoting this does not seem to cause issues for MOTUs that so far
maintained the package
- no massive Lintian warnings
- debian/rules is rather clean
- It is not on the lto-disabled list
(fix, or the workaround should be directly in the package,
see https://launchpad.net/ubuntu/+source/lto-disabled-list)
Problems: None
[Upstream red flags]
OK:
- no Errors/warnings during the build
- no incautious use of malloc/sprintf (as far as we can check it)
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH (one mention of
"sudo" is only in a user-facing hint string in valkey-benchmark for
macOS TCP tuning)
- no use of user 'nobody' outside of tests (two occurrences are just
English prose in log messages: "nobody is serving its slots",
"nobody likely winning the")
- no use of setuid / setgid
- no important open bugs (crashers, etc) in Debian or Ubuntu
- no dependency on webkit, qtwebkit or libseed
- not part of the UI for extra checks
- no translation present, but none needed for this case (server daemon
and CLI tools, not user-facing UI)
Problems: None
** Changed in: valkey (Ubuntu)
Assignee: Myles Penner (mylesjp) => (unassigned)
** Changed in: valkey (Ubuntu)
Assignee: (unassigned) => Ubuntu Security Team (ubuntu-security)
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/2142907
Title:
[MIR] valkey
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/valkey/+bug/2142907/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
