[MIR] python-aiosmtpd as dependency of mailman3

[Christian Ehrhardt ]

[Availability]
The package is already universe for quite a while and build/works fine so far.
It builds amd64 only (but binary is arch-all)
This package builds only python3 related binaries.

[Rationale]
This is part of the MIR activity for all dependencies of mailman3
The "main" MIR of it is at bug 1775427:

Mailman (2) has only python2 support, but we strive for python3,
therefore Mailman3 which has python3 support should be promoted to main.

Given this deals with mail transfer it (even more) needs a security review IMHO.

[Security]

No known CVEs found.
But dealign with SMTP this should IMHO get a deeper security evaluation.

[Quality assurance]

As part of the mailman3 stacks as of now (Disco) this installs fine and works fine.
On itself it is useful to (many) other dependencies and does not need a post install configuration on its own.

The package does not ask debconf questions.

Zero known bugs in Ubuntu and Debian for this component.
16 low-medium severity issues reported upstream, but nothing to action on yet.

The package gets somewhat regular updates by upstream and Debian packages them as well as packaging some fixes as needed.

No exotic HW involved.

The package utilizes build time self tests.

d/watch is set up and ok.

No Lintian warning except newer Standards or GPG checks - nothing severe.

The package does not rely on demoted or obsolete packages.
No new gt2k dependencies

[UI standards]

This is a low level library without (a lot) of user visible strings - no translations (needed).
No End-user applications that needs a standard conformant desktop file.

[Dependencies]

Some dependencies are not in main, but we drive MIR for all related packages
that are not in main at the same time.
Please check the list of bugs from the main Mailman3 MIR in bug 1775427 to get an overview.

[Standards compliance]
The package meets the FHS and Debian Policy standards.
The packaging itself is very straight forward and uses dh_* as much as possible - the d/rules fits on one screen.

[Maintenance]

The Server team will subscribe for the package for maintenance, but in
general it seems low on updates and currently is a sync from Debian.

[Background]
The package description explains the general purpose and context of the package well.

[Christian Ehrhardt  (paelzer)]

[Duplication]
No duplication of that functionality in the Archive in general or main in particular.

[Embedded sources and static linking]
This package does not contain embedded library sources.
This package does not statically link to libraries.
No Go package

[Security]
I can confirm that there seems to be no CVE/Security history for this package.
It Does not:
- run a daemon as root
- uses old webkit
- uses lib*v8 directly
- uses centralized online accounts
- integrates arbitrary javascript into the desktop
- deals with system authentication

But it does:
- processes arbitrary web (actually smtp) content
- parse data formats
- open a port (indirectly, but it processes network data)

It is a python implemented SMTP server, therefore is recommended to get a security check.

[Common blockers]
- builds fine at the moment
- server Team committed to subscribe once this gets promoted (enough for now)
- code is not user visible, no translation needed
- dh_python is used
- package produces only python3 binaries
- utilizes build time self tests

[Packaging red flags]
- no current ubuntu Delta to evaluate
- no library with classic symbol tracking
- watch file is present
- Lintian warnings are present but ok
- debian/rules is rather clean
- no usage of Built-Using
- no golang package that would make things harder

[Upstream red flags]
- no suspicious errors during build (a few warnings, but nothing concerning)
  - the flurry of warnings is only on the rst doc and seems no deal breaker
- it is pure python, so no incautious use of malloc/sprintf
- no use of sudo, gksu
- no use of pkexec
- no use of LD_LIBRARY_PATH
- no important open bugs
- no Dependency on webkit, qtwebkit, libgoa-*
- no embedded copies in upstream either

[Summary]
Ack from the MIR-Teams POV, but as outlined above a security review is recommended.
Assigning the security Team.

[Seth Arnold (seth-arnold)]

Eduardo is taking a look at this package for the security team and pointed out that it is doing a setuid to user 'nobody'.

This isn't a safe design. User nobody is strictly for NFS's use and must not be used by any running processes on the system.

This service probably needs its own user account.
I'm not sure what its goals are by changing to nobody, but we probably also need to fix the setuid code. (Far better would be to strip the code out and use systemd's facilities for setting user, group, groups, etc.)

The code currently looks like:

@public
def main(args=None):
    parser, args = parseargs(args=args)

    if args.setuid: # pragma: nomswin
        if pwd is None:
            print('Cannot import module "pwd"; try running with -n option.',
                  file=sys.stderr)
            sys.exit(1)
        nobody = pwd.getpwnam('nobody').pw_uid
        try:
            os.setuid(nobody)
        except PermissionError:
            print('Cannot setuid "nobody"; try running with -n option.',
                  file=sys.stderr)
            sys.exit(1)

The usual practice with changing privs is to set groups, set group, and then set the user.

I'm a bit curious what the usecase of this tool is -- it also appears to start a mainloop that will break on keyboard interrupt -- is it meant to be run in a shell session or something? Why wouldn't it be a daemon? (If it *is* meant to be run from a terminal, then it also needs to prevent TIOCSTI use by the processes running with lowered privileges, if that is indeed why it changed to nobody.)

At a first glance this doesn't feel ready for prime-time.

Thanks

[Christian Ehrhardt  (paelzer)]

On Sat, May 11, 2019 at 5:15 AM Seth Arnold <email address hidden> wrote:
>
> Eduardo is taking a look at this package for the security team and
> pointed out that it is doing a setuid to user 'nobody'.
>
> This isn't a safe design. User nobody is strictly for NFS's use and must
> not be used by any running processes on the system.

Thanks a lot for pointing this out Seth and Eduardo!
We need to get in touch with upstream to find out the real reasons
behind some of them as I'm afraid blindly trying to fix things won't
be good either.

@Seth - I have added setuid use and the non-NFS nobody to the entries
in [1] to be catched earlier in the MIR process if possible.

[1]: https://wiki.ubuntu.com/MIRTeam#Upstream_red_flags

[Eduardo Barretto (ebarretto)]

I reviewed python-aiosmtpd version 1.2-3 as checked into eoan as of this
writing.

This shouldn't be considered a full audit but rather a quick gauge of
maintainability.

python-aiosmtpd is an asyncio based SMTP server.

- Last commit from March
- No CVE history
- Build-depends:
 - debhelper,
 - dh-python,
 - openssl,
 - python3-all,
 - python3-docutils,
 - python3-setuptools,
 - python3-sphinx
- postinst and prerm added automatically
- No init scripts
- No systemd services
- No DBus services
- No setuid
- Binaries in PATH:
 /usr/bin/aiosmtpd
- No sudo fragments
- No udev rules
- Some tests under aiosmtpd/tests/
 - FTBS in debian (from 2017). A test randomly fails, seems to be related to a
   possible race condition in test code. See:
   https://github.com/aio-libs/aiosmtpd/issues/133
 - test SMTP protocol
 - test SMTP over SSL/TLS
 - test server implementation
 - test LMTP protocol
- No cron jobs
- A lot of warnings in the build log:
 - Most warnings are about doc files
 - Some warnings that might be relevant to someone:
test_message (aiosmtpd.tests.test_handlers.TestAsyncMessage) ... /<<PKGBUILDDIR>>/.pybuild/cpython3_3.7_aiosmtpd/build/aiosmtpd/controller.py:64: PendingDeprecationWarning: Task.all_tasks() is deprecated, use asyncio.all_tasks() instead
test_setuid (aiosmtpd.tests.test_main.TestMain) ... /usr/lib/python3.7/asyncio/base_events.py:623: ResourceWarning: unclosed event loop <_UnixSelectorEventLoop running=False closed=False debug=False>
ResourceWarning: Enable tracemalloc to get the object allocation traceback
test_quit_with_arg (aiosmtpd.tests.test_smtp.TestSMTP) ... /usr/lib/python3.7/socket.py:660: ResourceWarning: unclosed <socket.socket fd=7, family=AddressFamily.AF_INET6, type=SocketKind.SOCK_STREAM, proto=6, laddr=('::1', 33640, 0, 0), raddr=('::1', 8025, 0, 0)>
ResourceWarning: Enable tracemalloc to get the object allocation traceback

- No subprocess spawned
- File IO only in setup_helpers.py (helper functions for setup.py).
  Path to file hardcoded in setup.py and conf.py.
- Not so much logging done, mainly in smtp.py
 - uses logging module for logging debug and info messages
 - uses warnings module for logging warnings
 - apparently no logging in case of errors
- Environment variable
 - make use of AIOSMTPD_CONTROLLER_TIMEOUT environment variable, expecting a
   float number
 - if variable not set, falls back to default '1.0'
 - no sanitization of input, but if a float number is not passed, will trigger
   exception
- setuid() server to 'nobody' user. This shouldn't be done, 'nobody' should be
  strictly used for NFS.
- Encryption
 - make use of SSL/TLS
- Networking
 - SMTP server listens on a port specified on command line, or default port
   8025.
- No WebKit
- No polkit
- No shell scripts
- No coverity issues

This is not an ACK or a NACK, we will keep waiting on the setuid to 'nobody' issue.

[Christian Ehrhardt  (paelzer)]

After evaluating dependencies, required further changes and mostly maintainability for security and packaging it was decided there are too many concerns - not about any single package in particular, but the overall Mailman3 stack - about the ability to maintain and monitor it as well as we need it for support in main.

We have closed the primary LP bug already, the MIRs that are already approved will stay that way, but we will make no seed change to pull things in for now. Yet if other needs come up for those they have a prepared MIR already.
Other bugs - like this one - which are not yet completed in terms of review will be closed as Won't Fix.

Even thou it ended being aborted, I think that is a valid outcome of the MIR evaluations. Never the less I want to thank everybody involved for all the work spent in what was nearly a year working through these MIRs.