[Secure-testing-team] Bug#832265: suck: Stack smash if lockfile exists (pid_t cast to long *)

[David Taylor]

Package: suck
Version: 4.3.2-15+b1
Severity: important
Tags: security upstream

The switch to using -fstack-protector-strong in stretch has exposed a 
stack-smashing bug.

The problem affects 64-bit platforms, as the code is assuming 
sizeof(pid_t) == sizeof(long), yet on x86_64:

sizeof(pid_t) == 4
sizeof(long) == 8

The relevant code is in suckutils.c, do_lockfile():

int do_lockfile(PMaster master) {
...
    pid_t pid;
...
        fscanf(f_lock, "%ld", (long *) &pid);
...
}

This writes an 8-byte long to the location of the 4-byte pid variable, 
smashing 4 bytes of the stack.

I doubt this is usefully exploitable (with the stack protector enabled,
it's only overwriting the canary), but it does mean a stale lockfile 
causes suck to crash (but helpfully, the lockfile is unlinked before the 
crash, so the next run will be successful).

This is obviously an upstream bug, but unfortunately there no longer 
appears to be an upstream...

-- System Information:
Debian Release: stretch/sid
  APT prefers testing
  APT policy: (500, 'testing')
Architecture: amd64 (x86_64)

Kernel: Linux 4.6.0-1-amd64 (SMP w/6 CPU cores)
Locale: LANG=en_GB.UTF-8, LC_CTYPE=en_GB.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)

Versions of packages suck depends on:
ii  debconf [debconf-2.0]  1.5.59
ii  libc6                  2.23-1
ii  libssl1.0.2            1.0.2h-1

Versions of packages suck recommends:
ii  perl  5.22.2-2

Versions of packages suck suggests:
ii  inn2 [news-transport-system]  2.6.0-2
ii  slrn [news-reader]            1.0.2-5

-- no debconf information