Comment 24 for bug 2023971

I reviewed libmail-dmarc-perl 1.20230215-1 as checked in noble. This shouldn't be considered a full
audit but rather a quick gauge of maintainability.

libmail-dmarc-perl is a Perl module implementing DMARC. It can be used by:
- MTAs and filtering tools like SpamAssassin to **validate** that incoming messages are aligned with purported sender's policy
- email senders, to **receive DMARC reports** from other mail servers and display them via CLI and web interfaces
- MTA operators to **send DMARC reports** to DMARC author domains

The main use-case and the only reverse dependency of libmail-dmarc-perl in Ubuntu is SpamAssassin.
It requires only the validation functionality by default, thus, as was discussed in the MIR process,
unnecessary dependencies related to reporting functionality were moved to Suggested and will not be
promoted to main at this time, thus limiting the functionality and reducing attack surface in the main archive.

- CVE History
  - None
  - Github issues with potential security implications (and not related to *reporting* functionality):
    - https://github.com/msimerson/mail-dmarc/issues/121 (does not seem exploitable)
  - Dependency issues:
    - https://github.com/rjbs/Email-MIME/issues/66#issuecomment-626260473 (fixed)

- Build-Depends
  - large dependency tree when both validation and reporting functionality is
    required, including DBD::SQLite, Data::Dumper, HTTP::Tiny, IO::Socket::SSL,
    NET::DNS::Resolver, Email::Sender, NET::SSLeay, Sys::Syslog, Net::IDN::Encode...
  - the main use-case (SpamAssassin) only uses the validation functionality,
    which significantly reduced the dependency count (Regexp::Common, Config::Tiny,
    File::ShareDir, Net::DNS::Resolver, Net::IP, Net::IDN::Encode).The original MIR
    proposes splitting dependencies into main (validation) and universe (reporting).
  - Suggested change for including in main is to replace `Net::IDN::Encode` (pure perl
    implementation of IDN, last update 2018, unresponsive maintainer -
    https://github.com/cfaerber/Net-IDN-Encode/pull/11#issuecomment-1919484551)
    with `Net::LibIDN` (perl binding for gnu-libidn, last update 2009, last update for
    gnu-libidn 2024) to avoid adding duplicate functionality to main. This replaces an
    unmaintained single-purpose library for a maintained and common library, but memory
    unsafe, which is an acceptable tradeoff.
  - Suggested change for including in main is to replace `Email::MIME` (last update 2023-1,
    several open issues) with `MIME::Tools` (last update 2024-02, infrequent updates,
    many very old open issues), in an attempt to fix a DoS issue, requiring non-trivial
    modifications to source code (see previous comment in MIR bug report).
- pre/post inst/rm scripts
  - none
- init scripts
  - none
- systemd units
  - none
- dbus services
  - none
- setuid binaries
  - none
- binaries in PATH
  - Several cmdline tools for looking up, receiving, and sending DMARC reports.
    The threat model is the same as for the library itself (reporting functionality)
    and depends on the user context. The tools should ideally not be run by a privileged user.
    - rwxr-xr-x root/root 2212 2023-06-17 17:38 ./usr/bin/dmarc_lookup
    - rwxr-xr-x root/root 1973 2023-06-17 17:38 ./usr/bin/dmarc_receive
    - rwxr-xr-x root/root 387 2023-06-17 17:38 ./usr/bin/dmarc_send_reports
    - rwxr-xr-x root/root 1864 2023-06-17 17:38 ./usr/bin/dmarc_update_public_suffix_list
    - rwxr-xr-x root/root 7846 2023-06-17 17:38 ./usr/bin/dmarc_view_reports
  - The tools are generally not applicable to the Spamassassin validation use-case,
    unless aggregation of reports is enabled in Spamassassin and the Suggested
    dependencies from universe are installed.
- sudo fragments
  - none
- polkit files
  - none
- udev rules
  - none
- unit tests / autopkgtests
  - Unit tests using Perl::More, continuous integration using Travis
  - Coverage is 78% (low mostly due to reporting and DB functionalities)
  - Perl::Critic is evaluated in 'stern' mode for each build
  - Locally all tests pass (required dependencies: libtest-file-sharedir-perl libdbd-sqlite3-perl
    libtest-exception-perl libxml-validator-schema-perl libemail-sender-perl libdbix-simple-perl
    libtest-output-perl)
- cron jobs
  - An example cronjob is provided for updating the public suffix list. It calls
    `dmarc_update_public_suffix_list` as root, which only mirrors the PSL file from
    https://publicsuffix.org/list/effective_tld_names.dat to the path defined in the config file.
    Generally safe, but would be better to run as a dedicated service account.
- Build logs
  - none

- Processes spawned
  - Unsafe use of string-`eval` for loading modules at run time
    (see issue https://github.com/msimerson/mail-dmarc/issues/234)
  - No backticks, `qx`, `system`.
- Memory management
  - none
- File IO
  - Generally ok - all calls to open() use the safe 3-argument version
  - The config file is first searched for in potentially insecure directories
    (order is `MAIL_DMARC_CONFIG_FILE, ./, /usr/local/etc, /opt/local/etc, /etc/, ./etc`)
    (see issue https://github.com/msimerson/mail-dmarc/issues/231).
    In the *very unlikely* event that the malicious user controls the env.var. or working directory,
    this could open attack vectors via malicious configuration options (e.g. command execution via
    the `backend` variable or DoS by using a large PSL file).
  - Public_suffix_file is trusted and read into hash without checking type/size, which could
    lead to DoS in certain unlikely scenarios (see above)
- Logging
  - syslog is used for logging in Sender submodule - no obvious issues
- Environment variable usage
  - MAIL_DMARC_CONFIG_FILE can be used to specify a user-controlled config file. See FileIO.
- Use of privileged functions
  - none
- Use of cryptography / random number sources etc
  - See Networking.
  - Cryptographicaly insecure rand() is used in PurePerl module to generate a percentage (0-100) for
    checking against DMARC PCT value. This is not an issue as it is not security-sensitive use-case.
- Use of temp files
  - none
- Use of networking
  - A CGI HTTP server for viewing reports is bundled with the library but is removed from the Ubuntu
    package and was thus not reviewed.
  - Primary use-case of the library in the context of Spamassassin is DMARC validation, where possible attack
    vectors are via a malicious email header (`header_from_raw`) or malicious dmarc record (`policy->parse()`).
    The possible injection points have been reviewed and are sanitized and thoroughly validated.
  - Aggregate DMARC reports can optionally be stored in a SQL database. Database queries are properly
    parameterized. Few contain concatenated values but these are not user controllable.
  - Reports stored in the DB can be sent using `dmarc_send_reports` program. STARTTLS is used when
    relaying via a "smarthost" SMTP server with authentication (credentials in config file). When
    the report is sent directly to "mailto:", STARTTLS is used if available, otherwise no security,
    which is acceptable due to non-sensitivity of the data.
  - Received DMARC reports are obtained with `dmarc_receive` either from local file, mbox,
    or from IMAP (credentials in config file). TLS via port 993 is used by default, but will downgrade
    to plaintext if Socket::SSL is not loadable (only prints a warning) or will accept any certificate
    if Mozilla::Ca is not loadable (no warning). Upstream issue: https://github.com/msimerson/mail-dmarc/issues/233
- Use of WebKit
  - none
- Use of PolicyKit
  - none

- Any significant cppcheck results
  - none
- Any significant Coverity results
  - none
- Any significant shellcheck results
  - none
- Any significant bandit results
  - none
- Any significant govulncheck results
  - none
- Any significant Semgrep results
  - none
- Other notes
  - HTTP client functionality is implemented for sending reports but it is not used
    internally. The stale code should ideally be removed.
  - Poor code coverage on reporting functionality should be improved.
  - Missing `security.md`.

The threat model for the primary use-case (validation in Spamassassin) includes
the processing of malicious email headers and dns dmarc records, potentially leading to
improper DMARC validation, denial of service or code exection on the host.
The possible injection points are generally well sanitized and validated to prevent abuse.
Minor issues related to file loading were noticed and raised upstream:
https://github.com/msimerson/mail-dmarc/issues/231

The threat model for the reporting functionality extends the above to processing
of malicious received reports (email subject, body, and attachement metadata), as
well as tampering and information disclosure due to insecure connection to external
(authenticated) SMTP/IMAP servers. In this case the user-controlled data is largely
processed with external libraries (IO::Uncompress, Email::Simple, Email::MIME,
XML::LibXML), and is thus outside the scope of this review.

The connection to IMAP was identified to not strictly enforce TLS, and the issue was
raised upstream: https://github.com/msimerson/mail-dmarc/issues/233

Additionally, an unsafe use of string-`eval` was discovered, which allows for
arbitrary code execution and possibly privilege escalation in the unlikely scenario
that the library is executed in a privileged context AND
in a world-writable direcory (e.g. root runs `dmarc_view_reports` in /tmp).
Upstream issue: https://github.com/msimerson/mail-dmarc/issues/234

The upstream maintainer responded promptly is resolving the issues.

Security team ACK for promoting libmail-dmarc-perl to main. This is irrespective
of the decision on whether the Email::MIME dependency is to be replaced or not.