[MIR] gsasl

Bug #1972866 reported by Lukas Märdian
14
This bug affects 1 person
Affects Status Importance Assigned to Milestone
gsasl (Ubuntu)
Fix Released
Undecided
Unassigned
mutt (Debian)
Fix Released
Unknown
mutt (Ubuntu)
Fix Released
Undecided
William Wilson

Bug Description

[Summary]
* Everything seems in order with this package, but it should
be reviewed by the security team due to the nature of the package.
* Build log: https://launchpadlibrarian.net/564514219/buildlog_ubuntu-jammy-amd64.gsasl_1.10.0-5_BUILDING.txt.gz

[Availability]
* The package is already available in Ubuntu universe and builds for the required architectures

[Rationale]
* mutt (which is in main) used to depend on cyrus-sasl. Due to a
licensing conflict between mutt and cyrus-sasl, it has been updated
to use gsasl and drop the dependency on cyrus-sasl. This change
has been made in Debian. Mutt is used by a large part of our
user base, so continuing to provide it is important.

[Security]
* Package gsasl and associated libraries do not have any
security red-flags, but should still be reviewed by
the security team due to the nature of the package (it
authenticates users to servers)
* No CVEs/security issues in this software in the past
* No `suid` or `sgid` binaries
* No executables in `/sbin` and `/usr/sbin`
* Package does not install services, timers or recurring jobs
* Package does not open privileged ports (ports < 1024)

[Quality assurance - function/usage]
* The package works well right after install

[Quality assurance - maintenance]
* The package is maintained well in Debian/Ubuntu and has not too many
and long term critical bugs open
* The package does not deal with exotic hardware we cannot support

[Quality assurance - testing]
* The package runs a test suite on build time, if it fails
it makes the build fail
* The package runs an autopkgtest, and is currently passing

[Quality assurance - packaging]
* debian/watch is present and works
* debian/control defines a correct Maintainer field
* This package does not yield massive lintian Warnings, Errors
* Full output of `lintian --pedantic`:
```
P: gsasl source: update-debian-copyright 2014 vs 2021 [debian/copyright:44]
P: gsasl source: very-long-line-length-in-source-file configure line 13808 is 704 characters long (>512)
P: gsasl source: very-long-line-length-in-source-file examples/openid20/README line 92 is 807 characters long (>512)
P: gsasl source: very-long-line-length-in-source-file examples/saml20/README line 171 is 1396 characters long (>512)
P: gsasl source: very-long-line-length-in-source-file ... use --no-tag-display-limit to see all (or pipe to a file/program)
```
* Lintian overrides are present, but ok because upstream does
not provide source-only tarballs
* This package has no python2 or GTK2 dependencies
* Packaging and build is easy. d/rules is concise and readable

[UI standards]
* Application is end-user facing, Translation is present, via gettext

[Dependencies]
* libgsasl-dev depends on a package from src:libntlm. MIR for
libntlm is here: https://bugs.launchpad.net/ubuntu/+source/libntlm/+bug/1976405

[Standards compliance]
* This package correctly follows FHS and Debian Policy

[Maintenance/Owner]
* Owning Team will be foundations
* Team is not yet, but will subscribe to the package before promotion
* This does not use static builds
* This does not use vendored code
* The package successfully built during the most recent test rebuild

[Background information]
* The Package description explains the package well
* Upstream Name is GNU SASL
* Upstream Link is https://www.gnu.org/software/gsasl/

CVE References

Lukas Märdian (slyon)
description: updated
tags: added: fr-2362
Changed in mutt (Debian):
status: Unknown → Fix Released
Changed in gsasl (Ubuntu):
assignee: nobody → William Wilson (jawn-smith)
Changed in gsasl (Ubuntu):
milestone: none → ubuntu-22.10
description: updated
Changed in gsasl (Ubuntu):
assignee: William Wilson (jawn-smith) → nobody
status: Incomplete → New
tags: added: update-excuse
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Thanks, since it is tracking only I assigned it to you.
So it will be in update-excuses, but not show up in the MIR reports.

Changed in mutt (Ubuntu):
assignee: nobody → William Wilson (jawn-smith)
Changed in gsasl (Ubuntu):
assignee: nobody → Didier Roche (didrocks)
Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :
Download full text (5.2 KiB)

Review for Package: gsasl

[Summary]
MIR team ACK will be granded once the discussion on the required TODOs (and possible actions) are solved, and as much as possible having a look at the recommended TODOs.
This does not need a security review
List of specific binary packages to be promoted to main: gsasl, libgsasl18, libgsasl-dev, gsasl-doc, gsasl-common
Specific binary packages built, but NOT to be promoted to main: libgsasl7-dev
This package replaces cyrus-sasl which should be demoted once transitioned.

Notes:
Required TODOs:
- Please review the autopkgtests story: it seems quite light (running --version on the binary and building a simple program against the lib which get the version). Is there any integration tests we can use from the testsuite used during package build we can borrow?
- We need to wait for libntlm to be approved and in main first.
Recommended TODOs:
- There is a way override in the rules file with no proper clean up, please check:
override_dh_auto_install:
        dh_auto_install
- There is one warning during the build. It’s harmless as we are using %d decimal and if we can’t allocate a new variables, we have bigger issues ahead :) Still, for the worth of silencing all warnings, it would be good to have upstream fixing it. (One warning would ask for a second one, and so on… until the project build log is unreadable)
../../tests/simple.c: In function 'doit':
../../tests/simple.c:289:3: warning: ignoring return value of 'asprintf' declared with attribute 'warn_unused_result' [-Wunused-result]
  289 | asprintf (&out, "%d.%d.%d", GSASL_VERSION_MAJOR,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  290 | GSASL_VERSION_MINOR, GSASL_VERSION_PATCH);
      | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- The package should get a team bug subscriber before being promoted

[Duplication]
This package replaces cyrus-sasl which should be demoted once transitioned.

[Dependencies]
OK:
- no other Dependencies to MIR due to this apart from libntlm
- gsasl checked with `check-mir`
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring more tests now.

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking
- does not have odd Built-Using entries

OK:
- not a go package, no extra constraints to consider in that regard

[Security]
OK:
- history of CVEs does not look concerning
- does not run a daemon as root
- does not use webkit1,2
- does not use lib*v8 directly
- does not parse data formats
- does not open a port/socket
- 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), etc)
- does not deal with security attestation (secure boot, tpm, signatures)

[Common blockers]
OK:
- does not FTBFS currently
- does have a test suite that runs at build time
- test suite fails will fail the build upon error.
- no new python2 dependency
- Python package, but using dh_python

Problems:
- Please review the autopkgtests story: it seems quite light (running --version on the bin...

Read more...

Changed in gsasl (Ubuntu):
assignee: Didier Roche (didrocks) → nobody
status: New → Incomplete
Revision history for this message
William Wilson (jawn-smith) wrote :

didrocks: I have attached a patch to:

1) increase testing somewhat
2) remove the build time warning
3) remove the useless override_dh_auto_install statement

Please let me know if you think this is ready to upstream and upload to ubuntu.

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

The changes looks good to me. Your patches don’t follow DEP-3 (https://dep-team.pages.debian.net/deps/dep3/), is that desired?

Otherwise, MIR ack on my side once this is uploaded.

Revision history for this message
Simon Josefsson (simon-josefsson) wrote :

Hi. Upstream talking here. Thanks for review! Much appreciated with external eyes on a package. Some comments:

1) Debian autopkgtest is not light, the debian/tests/libgsasl actually runs all of the upstream test suite except for GS2/GSSAPI (which require more infrastructure to test) including CRAM-MD5, DIGEST-MD5, SCRAM and several other internal APIs. It may be easy to overlook it when reading debian/tests/libgsasl though.

2) I've fixed the useless override_dh_auto_install:
https://salsa.debian.org/xmpp-team/gsasl/-/commit/98b21e56ea6e9d7234459769d3a71997659e5ac4

3) I've fixed the asprintf issue:
https://git.savannah.gnu.org/gitweb/?p=gsasl.git;a=commit;h=fd0ff175cd45f55a32e2352cab0de99c0f7c7898

4) I've added the gsasl-scram-pbkdf2 self-test to autopkgtest now too (it was implicitly tested through SCRAM self-tests, but doesn't hurt having in autopkgtest too):
https://salsa.debian.org/xmpp-team/gsasl/-/commit/de8b713fb4e9ebfa64438f177ff900aa18bc2dd9

Btw, the library is in transition in Debian now to the upcoming new stable 2.x.x branch (minimal changes, just dropping obsolete APIs) and we ran into some issues with the libgsasl7->gsasl-common dependency, but it probably isn't relevant to Ubuntu. If anyone has suggestions on better handling, I'm all ears: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1012768

Thanks,
Simon

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

I'd also object to Didier who said "This does not need a security review".
This clearly parses data, and it does so to encrypt/decrypt it.
Therefore IMHO this clearly falls into the "needs security" pocket IMHO.

Assigning to security ...

@Didier please let me know if you want to discuss (we can do so in tomorrows meeting). Do we need to change the template to clarify (for either of us depending on the outcome of the discussion).

Changed in gsasl (Ubuntu):
assignee: nobody → Ubuntu Security Team (ubuntu-security)
Revision history for this message
Seth Arnold (seth-arnold) wrote :

If there is a transition to 2.x underway, should we delay / redo these checks once the new packages have landed in Unstable?

Thanks

tags: added: sec-1101
Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

@simon-josefsson: ack, thanks for letting me know! Indeed, I missed env MD5FILE=tests/cram-md5.pwd $WORKDIR/foo in the for loop which executes the upstream tests. Thanks for the additional work!

@Christian: you are correct, I don’t know why this failed my grep foo (I was surprised to see no parsing of data due to the nature of the lib, but then, I thought this was relying on another one). As in the report, it was telling that it was not parsing data too, I took this as a confirmation of my findings (or rather, non findings).

Revision history for this message
William Wilson (jawn-smith) wrote :

The new major version 2.x landed in proposed overnight, so I'll re-do some checks. It seems to have all the fixes discussed by didrocks.

Revision history for this message
William Wilson (jawn-smith) wrote :

From the NEWS file:

```
* Noteworthy changes in release 2.0.0 (2022-06-20) [stable]

** Compared to last stable branch 1.10.x the 2.0.0 release
** drops all obsolete APIs, drops the abandoned KERBEROS_V5 mechanism,
** stops shipping a separate tarball for only the library, adds new APIs
** gsasl_mechanism_name_p() and gsasl_property_free().
Numerous other translation improvements, code cleanups, bug fixes,
documentation additions, build improvements and portability
enhancements were made as well.
```

None of these are inherently problematic, and dropping obsolete APIs could even help with the security review. The package still builds without warning (the return value of asprintf is checked), the dh_override_auto_install has been removed, and the testing is sufficient.

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

ACK on the changes. I looked at https://launchpadlibrarian.net/608367114/gsasl_1.11.3-3_2.0.0-1.diff.gz too and nothing is scary.

Let’s what for the security team review now.

Revision history for this message
William Wilson (jawn-smith) wrote :

I also looked at the debdiff and saw nothing of concern.

Revision history for this message
Seth Arnold (seth-arnold) wrote :

I'm not well-versed in the excuses page but I didn't see any mutt results after the new gsasl upload:

https://people.canonical.com/~ubuntu-archive/proposed-migration/update_excuses.html

Migration status for mutt (2.1.4-1build1 to 2.2.4-1build1): BLOCKED: Rejected/violates migration policy/introduces a regression
Issues preventing migration:
mutt/amd64 in main cannot depend on libgsasl18 in universe
...

Does this mean that the tests were entirely skipped? it'd be nice to have some confirmation that mutt at least loads with the new, new sasl. :)

Thanks

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

mutt has no DEP8 tests :(

Changed in gsasl (Ubuntu):
status: Incomplete → New
Revision history for this message
Mark Esler (eslerm) wrote :

I reviewed gsasl based on 2.0.1-4ubuntu1 as checked into kinetic proposed with Libntlm disabled in d/rules and d/control. This shouldn't be considered a full audit but rather a quick gauge of maintainability. This review focuses on non-Gnulib code.

- CVE History:
  - CVE-2022-2469
    - "Simon Josefsson discovered an out-of-bounds memory read in GNU SASL, an implementation of the Simple Authentication and Security Layer framework, which could result in denial of service."
    - https://www.debian.org/security/2022/dsa-5189
    - please note that the version checked into kinetic is vulnerable, the kinetic proposed version is not
  - relevant issues in https://git.savannah.gnu.org/cgit/gsasl.git/log/
    - commit history was not ported to new gitlab repo
  - https://gsasl.gitlab.io/gsasl/clang-analyzer/
  - several issues were reported to GNU SASL during this audit
- Build-Depends?
  - primarily Gnulib
  - debhelper-compat, gettext, help2man, libgnutls28-dev, libgssglue-dev, libidn11-dev, pkg-config, texinfo, valgrind-if-available
  - linux-vdso.so.1
  - libgsasl.so.7
  - libgnutls.so.30
  - libc.so.6
  - libidn.so.12
  - libntlm.so.0
  - libgssapi_krb5.so.2
  - libp11-kit.so.0
  - libidn2.so.0
  - libunistring.so.2
  - libtasn1.so.6
  - libnettle.so.8
  - libhogweed.so.6
  - libgmp.so.10
  - ld-linux-x86-64.so.2
  - libkrb5.so.3
  - libk5crypto.so.3
  - libcom_err.so.2
  - libkrb5support.so.0
  - libffi.so.8
  - libkeyutils.so.1
  - libresolv.so.2
- pre/post inst/rm scripts?
  - none
- init scripts?
  - none
- systemd units?
  - none
- dbus services?
  - none
- setuid binaries?
  - none
- binaries in PATH?
  - /usr/bin/gsasl
- sudo fragments?
  - none
- polkit files?
  - none
- udev rules?
  - none
- unit tests / autopkgtests?
  - gssapi test is skipped by build/autopkgtests (!)
  - gs2-krb5 test is skipped by build/autopkgtests (!)
- cron jobs?
  - none
- Build logs:
  - okay

- Processes spawned?
  - does not appear to be in binaries
- Memory management?
  - heavy use of memory
  - some issues reported upstream
- File IO?
  - yes, by Gnulib
- Logging?
  - yes
  - besides Gnulib, tests, and examples, mostly in ./src/gsasl.c main()
- Environment variable usage?
  - yes, by Gnulib
- Use of privileged functions?
  - yes, by Gnulib
- Use of cryptography / random number sources etc?
  - implements SASL
  - most crypto functions utilize Gnulib
- Use of temp files?
  - none, besides example
- Use of networking?
  - mostly by Gnulib
  - ./src/gsasl.c main
- Use of WebKit?
  - none
- Use of PolicyKit?
  - none

- Any significant cppcheck results?
  - unknown macros
    - mostly caused when macros defined in #if statements and then used elsewhere
- Any significant Coverity results?
  - many after ignoring example and Gnulib code
- Any significant shellcheck results?
  - none, ignoring build and test hits
- Any significant bandit results?
  - none

Security team ACK for promoting gsasl to main.

Revision history for this message
Mark Esler (eslerm) wrote :

Please see Debian's license conflict between Cyrus SASL and mutt: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=999672

Please see Cyrus SASL's BSD-4-Clause license: https://github.com/cyrusimap/cyrus-sasl/blob/master/COPYING

Please see mutt on GNU SASL: https://marc.info/?l=mutt-dev&m=164349482428205&w=2

Cyrus SASL is a mature codebase with an active community of developers. Coverity found zero warnings when quickly auditing cyrus-sasl2 2.1.28+dfsg-6ubuntu1 as checked into kinetic main.

GNU SASL is written primarily by Simon Josefsson who has written many RFCs on cryptographic implementations and is a principal Gnulib contributor. Coverity found many warnings when quickly auditing gsasl based on kinetic's proposed 2.0.1-4ubuntu1 with Libntlm removed.

An increase in GNU SASL's userbase may increase the interest of security researchers.

I recommend that security focused users use Cyrus SASL until GNU SASL has been proven by Debian.

Changed in gsasl (Ubuntu):
assignee: Ubuntu Security Team (ubuntu-security) → nobody
status: New → In Progress
Revision history for this message
Lukas Märdian (slyon) wrote (last edit ):

This is already pulled in as a dependency in -proposed. I've also dropped the new libgssglue dependency for now (which is basically a NOOP) to avoid that additional component-mismatch and subscribed ~foundations-bugs.

MIR team & security team ACK => Fix Committed

Changed in gsasl (Ubuntu):
status: In Progress → Fix Committed
Revision history for this message
Łukasz Zemczak (sil2100) wrote :
Download full text (3.2 KiB)

$ change-override -c main -S -s kinetic-proposed gsasl
Override component to main
gsasl 2.0.1-4ubuntu1 in kinetic: universe/devel -> main
gsasl 2.0.1-4ubuntu1 in kinetic amd64: universe/devel/optional/100% -> main
gsasl 2.0.1-4ubuntu1 in kinetic arm64: universe/devel/optional/100% -> main
gsasl 2.0.1-4ubuntu1 in kinetic armhf: universe/devel/optional/100% -> main
gsasl 2.0.1-4ubuntu1 in kinetic ppc64el: universe/devel/optional/100% -> main
gsasl 2.0.1-4ubuntu1 in kinetic riscv64: universe/devel/optional/100% -> main
gsasl 2.0.1-4ubuntu1 in kinetic s390x: universe/devel/optional/100% -> main
gsasl-common 2.0.1-4ubuntu1 in kinetic amd64: universe/devel/optional/100% -> main
gsasl-common 2.0.1-4ubuntu1 in kinetic arm64: universe/devel/optional/100% -> main
gsasl-common 2.0.1-4ubuntu1 in kinetic armhf: universe/devel/optional/100% -> main
gsasl-common 2.0.1-4ubuntu1 in kinetic i386: universe/devel/optional/100% -> main
gsasl-common 2.0.1-4ubuntu1 in kinetic ppc64el: universe/devel/optional/100% -> main
gsasl-common 2.0.1-4ubuntu1 in kinetic riscv64: universe/devel/optional/100% -> main
gsasl-common 2.0.1-4ubuntu1 in kinetic s390x: universe/devel/optional/100% -> main
gsasl-doc 2.0.1-4ubuntu1 in kinetic amd64: universe/doc/optional/100% -> main
gsasl-doc 2.0.1-4ubuntu1 in kinetic arm64: universe/doc/optional/100% -> main
gsasl-doc 2.0.1-4ubuntu1 in kinetic armhf: universe/doc/optional/100% -> main
gsasl-doc 2.0.1-4ubuntu1 in kinetic i386: universe/doc/optional/100% -> main
gsasl-doc 2.0.1-4ubuntu1 in kinetic ppc64el: universe/doc/optional/100% -> main
gsasl-doc 2.0.1-4ubuntu1 in kinetic riscv64: universe/doc/optional/100% -> main
gsasl-doc 2.0.1-4ubuntu1 in kinetic s390x: universe/doc/optional/100% -> main
libgsasl-dev 2.0.1-4ubuntu1 in kinetic amd64: universe/libdevel/optional/100% -> main
libgsasl-dev 2.0.1-4ubuntu1 in kinetic arm64: universe/libdevel/optional/100% -> main
libgsasl-dev 2.0.1-4ubuntu1 in kinetic armhf: universe/libdevel/optional/100% -> main
libgsasl-dev 2.0.1-4ubuntu1 in kinetic ppc64el: universe/libdevel/optional/100% -> main
libgsasl-dev 2.0.1-4ubuntu1 in kinetic riscv64: universe/libdevel/optional/100% -> main
libgsasl-dev 2.0.1-4ubuntu1 in kinetic s390x: universe/libdevel/optional/100% -> main
libgsasl18 2.0.1-4ubuntu1 in kinetic amd64: universe/libs/optional/100% -> main
libgsasl18 2.0.1-4ubuntu1 in kinetic arm64: universe/libs/optional/100% -> main
libgsasl18 2.0.1-4ubuntu1 in kinetic armhf: universe/libs/optional/100% -> main
libgsasl18 2.0.1-4ubuntu1 in kinetic ppc64el: universe/libs/optional/100% -> main
libgsasl18 2.0.1-4ubuntu1 in kinetic riscv64: universe/libs/optional/100% -> main
libgsasl18 2.0.1-4ubuntu1 in kinetic s390x: universe/libs/optional/100% -> main
libgsasl7-dev 2.0.1-4ubuntu1 in kinetic amd64: universe/devel/optional/100% -> main
libgsasl7-dev 2.0.1-4ubuntu1 in kinetic arm64: universe/devel/optional/100% -> main
libgsasl7-dev 2.0.1-4ubuntu1 in kinetic armhf: universe/devel/optional/100% -> main
libgsasl7-dev 2.0.1-4ubuntu1 in kinetic ppc64el: universe/devel/optional/100% -> main
libgsasl7-dev 2.0.1-4ubuntu1 in kinetic riscv64: universe/devel/optional/100% -> main
libgsasl7-dev 2.0.1-4ubuntu1 in kinetic s...

Read more...

Lukas Märdian (slyon)
Changed in gsasl (Ubuntu):
status: Fix Committed → Fix Released
Changed in mutt (Ubuntu):
status: New → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.