Comment 3 for bug 1687454

I reviewed nghttp2 version 1.25.0-1 as checked into Debian unstable. This
shouldn't be considered a full security audit but rather a quick gauge of
maintainability.

nghttp2 provides an http/2 protocol interface that can be used by clients
and servers to handle the far more complicated http/2 protocol, compared
to the simpler http 1.1, 1.0, or 0.9 that many tools support.

Our database has three CVEs filed for nghttp2. The links are poor and
contemporaneous documentation on the nghttp2 website is poor. The authors
don't appear to have hidden the vulnerabilities but the entire CVE process
may be new to them or they've perhaps chosen to not assist.

Skimming release notes I think there may be vulnerabilities that still
need CVE numbers.

- The daemonizing code (not intended for main) looked fine
- pre/post inst/rm automatically generated
- initscript for (not intended for main) nghttpx looked fine
- systemd unit file for nghttpx looked simple
- No dbus
- No setuid
- binaries h2load, nghttp, nghttpx, nghttpd -- none should be in main
- no sudo fragments
- no udev rules
- test suite run during build, it says 1 test but I suspect that's just
  one file with many subtests
- no cron jobs
- build logs looked clean

- subprocess spawning (in non-main packages) looked fine
- extensive memory management; http/2 stream handling is extremely
  complicated but this package looked careful at every inspected instance
- logging looked safe
- environment variables seemed to be handled safely
- extensive privileged operations, looked careful at every inspected
  instance
- extensive encryption, openssl and libressl support, with neverbleed
  support too. Inspected instances looked safe.
- no privileged portions of code
- temp files looked to be handled safely
- no webkit
- no javascript
- no policykit
- cppcheck reports three errors in third-party/mruby/

I only made one note while reviewing that might reflect a weakness:

./src/shrpx_client_handler.cc sets aside 39 bytes for text display of ipv6
address -- ipv4 mapped addresses can be 45 bytes long. %interface scope
delimiters can be arbitrary in length. Are either of these possible?

And five comments along the way about the programming style or details

- memcpy() is used when unaligned accesses might be a problem
- memory management is complicated but many functions have precondition checks
- shrpx dropping-privs also supports neverbleed, looks neat
- Very thoughtful comments
- shrpx uses environment variables NGHTTPX_LISTENER4_FD
  NGHTTPX_LISTENER6_FD NGHTTPX_PORT NGHTTP2_UNIX_FD NGHTTP2_UNIX_PATH
  NGHTTPX_ACCEPT_* NGHTTPX_ORIG_PID, NOTIFY_SOCKET

I'm sure this code has more vulnerabilties, it's just too complicated a
protocol to not have more. But the authors have gone to great lengths to
check all calls for error returns, check preconditions, and provide
thoughtful helpful comments with links to source material.

I hope upstream will be willing to work with MITRE or other users to
assign CVEs for issue visibility when next needed.

Security team ACK for promoting nghttp2 (strictly the library packages as
needed) to main. (The non-library packages looked fine but I'd like there
to be a need for them before they are promoted.)

Thanks