Comment 8 for bug 1910262

I reviewed backuppc-rsync 3.1.3.0-3 as checked into hirsute. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

backuppc-rsync is a purpose-specific fork of rsync that is used by the
backuppc tool, to use their format for storing backups.

- CVE History:
  None in our database
- Build-Depends?
  debhelper-compat (= 13), libacl1-dev, libattr1-dev, libpopt-dev, zlib1g-dev,
- pre/post inst/rm scripts?
  None
- init scripts?
  None
- systemd units?
  None
- dbus services?
  None
- setuid binaries?
  None
- binaries in PATH?
  None
- sudo fragments?
  None
- polkit files?
  None
- udev rules?
  None
- unit tests / autopkgtests?
  Pretty sure None
- cron jobs?
  None
- Build logs:
  Some very-old-code kinds of warnings, not great, but probably not very
  useful in the threat model

- Processes spawned?
  Uses arrays
- Memory management?
  there's way too much old-school C stuff
- File IO?
  Quite a lot of it; controlled by remote execution.
- Logging?
  Looked fine
- Environment variable usage?
  Looked fine
- Use of privileged functions?
  Not much, looked fine
- Use of cryptography / random number sources etc?
  None
- Use of temp files?
  None
- Use of networking?
  Some, looked fine
- Use of WebKit?
  None
- Use of PolicyKit?
  None

- Any significant cppcheck results?
  Nothing too bad in the context of this tool
- Any significant Coverity results?
  Nothing too bad, upstream handled the issues quickly
- Any significant shellcheck results?
  None
- Any significant bandit results?
  None

backuppc-rsync is based on some *old* code, which is both good and bad.
It's a time-worn tool with some coding practices that aren't fantastic
today, in a language I wish we'd see less of, but being built on rsync is
no bad thing.

Upstream for backuppc-rsync was responsive, reached out for more
information for the issues I filed, and handled them well. None of the
issues were particularly pressing for the threat model here: it's not
like the remote endpoint is going to be run under control of malicious
entities.

Security team ACK for promoting backuppc-rsync to main.

Some notes I took while reviewing:

bpc_poolRefFileRead() fd leak in multiple error returns

read_delay_line() read() doesn't nul terminate deldelay_buf, but
strchr() expects it to be nul-terminated

bpc_attrib_dirRead() can pass fd to functions without being initialized?
hard to untangle

send_implied_dirs() buffer overwrite? hard to untangle -- 17999

send_extra_file_list() buffer overread? hard to untangle -- 17979

add_dirs_to_tree() buffer overwrite? hard to untangle, -- 17962

bpc_opendir() use-after-free, free(d->entries)

bpc_attrib_dirRead() can use nRead without initializing it first

bpc_attrib_xattrCopy() can leak 'key' if 'value' couldn't be allocated

add_dirs_to_tree() buffer overwrite? hard to untangle -- 17930

send_extra_file_list() buffer overread? hard to untangle -- 17894

bpc_poolWrite_copyToPool() appears to read and write eight bytes at a
time; is this a large operation? If this is responsible for a lot of IO
this could be pretty brutally bad.

bpc_poolWrite_copyToPool() can leak fdWrite if fdRead open() fails

add_cnk_list_entry() may have integer overflow problems in malloc() and
realloc() calls, I don't see any checks to prevent integer overflow.

do_cmd() constructs a command to start on a remote host; I don't see
anything wrong with it, but it's complicated.

create_pid_file() creates the pid file mode 0666, assuming umask allows
it. Probably best to not rely on umask for this.

https://github.com/backuppc/rsync-bpc/issues/23
https://github.com/backuppc/rsync-bpc/issues/24