[MIR] oath-toolkit

[James Page]

[Availability]
In universe

[Rationale]
New dependency for ceph (radosgw)

[Security]
One CVE found:
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-7322

Resolved in versions in Ubuntu

[Quality assurance]
Upstream tests run as part of package build.

[Dependencies]
All in main

[Standards compliance]
Older style CDBS package but OK.

[Maintenance]
Two non-maintainer uploads in Debian; A new point release is available from 2016
ubuntu-openstack team in Ubuntu

[Mathieu Trudel-Lapierre (cyphermox)]

This package does show some signs of age, and would probably really benefit (maintainability-wise) from having some cleanup/modernizing of the packaging, for instance to update debhelper usage, replace cdbs, etc.

Assigning to the Security team since this includes a PAM module and is generally security-sensitive auth software.

[James Page (james-page)]

Will the security review for this package be completed to allow migration of the ceph upload in cosmic proposed to the release pocket before we release?

[Seth Arnold (seth-arnold)]

oath-toolkit FTBFS for me:

[...]
/bin/bash ../libtool --tag=CC --mode=compile gcc -DHAVE_CONFIG_H -I. -I.. -Wdate-time -D_FORTIFY_SOURCE=2 -fvisibility=hidden -g -O2 -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -c -o fseeko.lo fseeko.c
libtool: compile: gcc -DHAVE_CONFIG_H -I. -I.. -Wdate-time -D_FORTIFY_SOURCE=2 -fvisibility=hidden -g -O2 -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -c fseeko.c -fPIC -DPIC -o .libs/fseeko.o
fseeko.c: In function 'rpl_fseeko':
fseeko.c:110:4: error: #error "Please port gnulib fseeko.c to your platform! Look at the code in fseeko.c, then report this to bug-gnulib."
   #error "Please port gnulib fseeko.c to your platform! Look at the code in fseeko.c, then report this to bug-gnulib."
    ^~~~~
make[7]: *** [Makefile:1402: fseeko.lo] Error 1
make[7]: Leaving directory '/<<PKGBUILDDIR>>/liboath/gl'
make[6]: *** [Makefile:1417: all-recursive] Error 1
make[6]: Leaving directory '/<<PKGBUILDDIR>>/liboath/gl'
make[5]: *** [Makefile:1263: all] Error 2
make[5]: Leaving directory '/<<PKGBUILDDIR>>/liboath/gl'
make[4]: *** [Makefile:1405: all-recursive] Error 1
make[4]: Leaving directory '/<<PKGBUILDDIR>>/liboath'
make[3]: *** [Makefile:1211: all] Error 2
make[3]: Leaving directory '/<<PKGBUILDDIR>>/liboath'
make[2]: *** [Makefile:401: all-recursive] Error 1
make[2]: Leaving directory '/<<PKGBUILDDIR>>'
make[1]: *** [Makefile:357: all] Error 2
make[1]: Leaving directory '/<<PKGBUILDDIR>>'
make: *** [/usr/share/cdbs/1/class/makefile.mk:77: debian/stamp-makefile-build] Error 2
dpkg-buildpackage: error: debian/rules build subprocess returned exit status 2

Does this look familiar to anybody?

Thanks

[James Page (james-page)]

I don't think:

# if defined _IO_ftrylockfile || __GNU_LIBRARY__ == 1

is compat with 2.28 of glibc -

_IO_ftrylockfile -> _IO_EOF_SEEN

looks like it might do the trick.

[James Page (james-page)]

I have a fix for the build failure plus some general package maintenance updates in flight to improve the maintenance posture of this package.

[James Page (james-page)]

Raised bug 1799473 to cover the SRU to fix the ftbfs

[Mathieu Trudel-Lapierre (cyphermox)]

Confirming, we've used this change (_IO_ftrylockfile replaced by _IO_EOF_SEEN) in other packages that FTBFS with the new toolchain.

Please also make sure _IO_IN_BACKUP is defined (to 0x100) if it's being used.

cf. similar changes in coreutils:

Index: coreutils-8.28/lib/fseterr.c
===================================================================
--- coreutils-8.28.orig/lib/fseterr.c
+++ coreutils-8.28/lib/fseterr.c
@@ -29,7 +29,7 @@ fseterr (FILE *fp)
   /* Most systems provide FILE as a struct and the necessary bitmask in
      <stdio.h>, because they need it for implementing getc() and putc() as
      fast macros. */
-#if defined _IO_ftrylockfile || __GNU_LIBRARY__ == 1 /* GNU libc, BeOS, Haiku, Linux libc5 */
+#if defined _IO_EOF_SEEN || __GNU_LIBRARY__ == 1 /* GNU libc, BeOS, Haiku, Linux libc5 */
   fp->_flags |= _IO_ERR_SEEN;
 #elif defined __sferror || defined __DragonFly__ || defined __ANDROID__
   /* FreeBSD, NetBSD, OpenBSD, DragonFly, Mac OS X, Cygwin, Minix 3, Android */
Index: coreutils-8.28/lib/stdio-impl.h
===================================================================
--- coreutils-8.28.orig/lib/stdio-impl.h
+++ coreutils-8.28/lib/stdio-impl.h
@@ -18,6 +18,12 @@
    the same implementation of stdio extension API, except that some fields
    have different naming conventions, or their access requires some casts. */

+/* Glibc 2.28 made _IO_IN_BACKUP private. For now, work around this
+ problem by defining it ourselves. FIXME: Do not rely on glibc
+ internals. */
+#if !defined _IO_IN_BACKUP && defined _IO_EOF_SEEN
+# define _IO_IN_BACKUP 0x100
+#endif

 /* BSD stdio derived implementations. */

[James Page (james-page)]

@cyphermox thanks for the pointer - my proposed patch covers the additional symbol as well.

[James Page (james-page)]

Fixes uploaded to cosmic-proposed under separate bug for SRU team review; I'll deal with updating the package to switch away from cdbs for dd

[Seth Arnold (seth-arnold)]

On Tue, Oct 23, 2018 at 02:41:44PM -0000, Mathieu Trudel-Lapierre wrote:
> Confirming, we've used this change (_IO_ftrylockfile replaced by
> _IO_EOF_SEEN) in other packages that FTBFS with the new toolchain.
>
> Please also make sure _IO_IN_BACKUP is defined (to 0x100) if it's being
> used.

This raises the question *why* it is using glibc internal datastructures.

I'm really skeptical of all the code in the gl/ directories:

https://sources.debian.org/src/oath-toolkit/2.6.1-1.2/oathtool/gl/parse-datetime.y/?hl=1254#L1254

This routine looks pretty subtle, and while it's got the benefit of (a)
starting from Steven M. Bellovin (b) thirty years of refinement, I'm
skeptical how it would handle malicious inputs.

It also appears in gnulib:

https://sources.debian.org/src/gnulib/20140202+stable-3/lib/parse-datetime.y/?hl=1254#L1254

which may or may not have had fixes applied to it over the years.
Certainly it'd be easier to fix it only in one place regardless of if
the two versions are identical or have diverged.

Here's a big gob of disgusting memory allocation wrapper around malloc(3):

https://sources.debian.org/src/oath-toolkit/2.6.1-1.2/oathtool/gl/malloca.c/
https://sources.debian.org/src/oath-toolkit/2.6.1-1.2/oathtool/gl/xalloc.h/

This *looks* like it tries to avoid integer overflow issues in an
nmalloca(n,s) macro:

https://sources.debian.org/src/oath-toolkit/2.6.1-1.2/oathtool/gl/malloca.h/#L78

But why on earth wouldn't you just use calloc(3) and let the system
library handle it? (I can't tell you if it's actually done the math
correctly or not. It's subtle.)

Did I overlook something from gl/ that *wasn't* from gnulib?

While I don't want to just drag gnulib into main on a whim, I wonder how
many other packages might have vendored in some or all of gnulib anyway.
There's 35 packages that have a function named parse_datetime in C sources
on Debian Code Search:

https://codesearch.debian.net/search?q=parse_datetime+filetype%3Ac&perpkg=1

Of that list, coreutils, dovecot, tar, patch, libdbi-drivers, unixodbc,
python-numpy, lftp, gnutls28, bluez, cpio, findutils, libdbi, are in main.

Maybe gnulib is vendored in a lot more places than we thought?

I've so far decided to skip reviewing everything in gl/ directories
because (a) it's entirely too subtle and likely too brittle (b) if it *is*
"just" gnulib then maybe we can or should remove all of it anyway.

Thanks

[Mathieu Trudel-Lapierre (cyphermox)]

Many packages vendor gnulib; this is a known issue. coreutils, gzip, m4, etc. are just some of the few that needed an immediate update for new glibc 2.28 changes. Let's discuss gnulib separately if necessary.

Is the gl/ code absolutely necessary? Can we refine the MIR to not include gl/ code?

[Seth Arnold (seth-arnold)]

I reviewed oath-toolkit version 2.6.1-1.2 as checked into cosmic. This
isn't a full security audit but rather a quick gauge of maintainability.
I especially did not audit the cryptography or xmlsec containers.

- seems dead upstream -- more than two years since the last checkin, which
  failed CI
- gitorious.org url in the README is dead
- FTBFS for me
- Large amount of very fragile-looking vendored code
- Appears to log passwords

- One CVE in our database: reported in December 2013 and fixed in
  February 2014.

- oath-toolkit provides totp, hotp, and Portable Symmetric Key Container
  (PSKC) tools -- library, PAM module, utilities
- links against xmlsec1; includes md5, sha1, sha2, sha3, aes, perhaps
  other algorithms
- Build-Depends: cdbs, debhelper, libpam0g-dev, datefudge, gtk-doc-tools,
  dblatex, libxml2-utils, libxmlsec1-dev, dh-autoreconf
- Does not daemonize
- No networking
- postinst/prerm scripts run pam-auth-update, ldconfig
- No initscript
- No systemd unit files
- No setuid files
- oathtool executable in the path
- No sudo fragments
- No udev rules
- Some tests are run as part of the build, including the gl/ directories.
  Tests look like smoke-tests rather than full regressions.
- No cron jobs
- No systemd timers
- Build logs show incorrect code in tests

- No subprocesses spawned
- The gl/ memory allocation routines are way too fragile and intricate.
  The memory management in the rest of the program is mixed, there's more
  integer multiplicates handed to malloc(3) than I like to see, but the
  ones I inspected all counted a number of objects first, which likely
  makes them safe, if not ideal.
- Logging is mixed -- these tools may log passwords (!) while in use.
  (The format strings are in the binaries. This is troubling.)
- No use of environment
- No privileged syscalls
- I did not inspect the cryptographic primitives or protocols
- I did not notice privileged portions of the code
- No temporary files
- No WebKit
- No PolicyKit
- cppcheck reports several issues, some in gl/tests/ and two false
  positives in gl/xmalloc (cppcheck doesn't understand the intention of
  the routines).

- query_prompt is allocated too short, no space is set aside for a NUL
  terminating byte. This may need a CVE.

- MAX_OTP_LEN check doesn't *always* constrain the length of a password,
  it feels likely to be a mistake. This may need a CVE.

- This appears to log a password: DBG (("conv returned: %s", resp->resp));
- This appears to log a password: DBG (("OTP too short: %s", password));
- This appears to log a password: DBG (("OTP shorter than digits=%d: %s", cfg.digits, password));
- This appears to log a password: DBG (("OTP too long (and no digits=): %s", password));
- This appears to log a password: DBG (("Password: %s ", onlypasswd));
- This appears to log a password: DBG (("authenticate rc %d (%s: %s) last otp %s", rc,
- ./oathtool/oathtool.c appears to log passwords:
  - "password \"%s\" not found in range %ld .. %ld",
  - "password \"%s\" not found in range %ld .. %ld",

This may need a CVE.

- gengetopt feels awkward at best:
  "replacement of strdup, which is not standard" -- strdup is in posix.1-2001
  so I have severe doubts that...

[James Page (james-page)]

Hi Seth

Thank-you for the review.

The fix for the FTBFS is in cosmic-proposed (resolving part of your feedback on the current state of the package).

I agree that the activity in the project upstream is worrying.

Re code vendoring of gnulib - indeed I'm not a fan for vendoring either however this does seem to be a pattern that gnulib promotes based on the comments in #11 (and the fact they have a tool todo this vendoring).

I did dig into whether we could disable this feature in ceph (which relates to the use of MFA in the Ceph RADOS Gateway - see http://docs.ceph.com/docs/mimic/radosgw/mfa/) however its not currently an optional component - we could work upstream to see if this would be possible, but that's not going to help us in the short term.

Right now we have a bit of a situation in that Ceph in bionic is 12.2.7 and Ceph in the cosmic release pocket is 12.2.4 which creates issues with upgrades (bug 1800526) so we need to resolve this one way or the other fairly quickly.

If there is not a path forward that involves use of oath-toolkit from Ceph, then we need to switch back to the 12.2.x series to resolve the current issues with upgrades whilst this is resolved.

[Seth Arnold (seth-arnold)]

On Wed, Oct 31, 2018 at 10:46:31AM -0000, James Page wrote:
> Right now we have a bit of a situation in that Ceph in bionic is 12.2.7
> and Ceph in the cosmic release pocket is 12.2.4 which creates issues
> with upgrades (bug 1800526) so we need to resolve this one way or the
> other fairly quickly.
>
> If there is not a path forward that involves use of oath-toolkit from
> Ceph, then we need to switch back to the 12.2.x series to resolve the
> current issues with upgrades whilst this is resolved.

Thanks for the detailed explanation, James.

I'll raise my concerns with oath-toolkit to the ceph security team.
Hopefully we can find a solution before 20.04 LTS.

Security team ACK on promoting liboath0 to main.

I'd really like to keep the other binary packages in universe. I know
this is a hassle. But I don't think we want this PAM module installed
by accident.

Thanks

[Matthias Klose (doko)]

Override component to main
oath-toolkit 2.6.1-1.2ubuntu0.18.10.1 in disco: universe/devel -> main
1 publication overridden.

Override component to main
liboath0 2.6.1-1.2ubuntu0.18.10.1 in disco amd64: universe/libs/optional/100% -> main
liboath0 2.6.1-1.2ubuntu0.18.10.1 in disco arm64: universe/libs/optional/100% -> main
liboath0 2.6.1-1.2ubuntu0.18.10.1 in disco armhf: universe/libs/optional/100% -> main
liboath0 2.6.1-1.2ubuntu0.18.10.1 in disco i386: universe/libs/optional/100% -> main
liboath0 2.6.1-1.2ubuntu0.18.10.1 in disco ppc64el: universe/libs/optional/100% -> main
liboath0 2.6.1-1.2ubuntu0.18.10.1 in disco s390x: universe/libs/optional/100% -> main
6 publications overridden.

This still needs some override excludes ...

[James Page (james-page)]

@doko I'm not quite sure how but if we can please can this be overridden for cosmic as well

[Jamie Strandboge (jdstrand)]

@james-page (cc @doko) - I'll take care of cosmic through a no change rebuild to -proposed and adjusting the override there.

[Jamie Strandboge (jdstrand)]

Actually, 2.6.1-1.2ubuntu0.18.10.1 is in cosmic-proposed already. Adjusting that.

Override component to main
oath-toolkit 2.6.1-1.2ubuntu0.18.10.1 in cosmic: universe/devel -> main
Override [y|N]? y
1 publication overridden.

Override component to main
liboath0 2.6.1-1.2ubuntu0.18.10.1 in cosmic amd64: universe/libs/optional/100% -> main
liboath0 2.6.1-1.2ubuntu0.18.10.1 in cosmic arm64: universe/libs/optional/100% -> main
liboath0 2.6.1-1.2ubuntu0.18.10.1 in cosmic armhf: universe/libs/optional/100% -> main
liboath0 2.6.1-1.2ubuntu0.18.10.1 in cosmic i386: universe/libs/optional/100% -> main
liboath0 2.6.1-1.2ubuntu0.18.10.1 in cosmic ppc64el: universe/libs/optional/100% -> main
liboath0 2.6.1-1.2ubuntu0.18.10.1 in cosmic s390x: universe/libs/optional/100% -> main
Override [y|N]? y
6 publications overridden.