Fix for CVE-2020-28007 causes build failure when DMARC is enabled

Bug #1927755 reported by Ian Kelling
262
This bug affects 1 person
Affects Status Importance Assigned to Milestone
exim4 (Ubuntu)
Fix Released
Low
Unassigned
Bionic
Won't Fix
Low
Bryce Harrington
Focal
Fix Released
Low
Unassigned
Hirsute
Fix Released
Low
Unassigned

Bug Description

[Impact]
This is a regression in bionic's exim4 caused by the introduction of a CVE fix last year. It prevents users from accessing custom functionality from the shipped package when reconfiguring/rebuilding.

[Test Case]
To reproduce the build failure on bionic:

1. Save this file to Local/Makefile, to enable building dmarc.c
   https://bugs.launchpad.net/ubuntu/+source/exim4/+bug/1927755/+attachment/5605652/+files/Makefile
2. cp exim_monitor/EDITME Local/eximon.conf
3. sudo apt-get install -y exim4 libopendmarc-dev libspf2-dev
4. sudo apt-get build-dep -y exim4
5. make
    (It should build successfully at this point since patches aren't applied)
6. quilt push -a
    make
    This should fail.

[Where problems could occur]
This change modifies a CVE patch, so the obvious concern would be if this invalidates the CVE. Mitigating this concern is that the change itself was suggested by a Ubuntu server team member, and that the change is isolated to code that is not even compiled for the stock package provided by bionic.

Since this affects code we don't compile, if the fix is incorrect for some reason, our regular CI won't detect it. So bugs related to custom compilation of bionic's exim4 could potentially be worth watching for.

[Original Report]
./debian/patches/sec-202105/0051-CVE-2020-28007-Link-attack-in-Exim-s-log-directory.patch
Backport of:

From 93e9a18fbf09deb59bd133986f4c89aeb2d2d86a Mon Sep 17 00:00:00 2001
From: Qualys Security Advisory <email address hidden>
Date: Tue, 23 Feb 2021 08:33:03 -0800
Subject: [PATCH 51/57] CVE-2020-28007: Link attack in Exim's log directory

We patch this vulnerability by opening (instead of just creating) the
log file in an unprivileged (exim) child process, and by passing this
file descriptor back to the privileged (root) parent process. The two
functions log_send_fd() and log_recv_fd() are inspired by OpenSSH's
functions mm_send_fd() and mm_receive_fd(); thanks!

This patch also fixes:

- a NULL-pointer dereference in usr1_handler() (this signal handler is
  installed before process_log_path is initialized);

- a file-descriptor leak in dmarc_write_history_file() (two return paths
  did not close history_file_fd).

Note: the use of log_open_as_exim() in dmarc_write_history_file() should
be fine because the documentation explicitly states "Make sure the
directory of this file is writable by the user exim runs as."

(cherry picked from commit 2502cc41d1d92c1413eca6a4ba035c21162662bd)
---
 src/src/dmarc.c | 179 ++++++++++++++++++------------------
 src/src/exim.c | 14 +--
 src/src/functions.h | 3 +-
 src/src/log.c | 214 ++++++++++++++++++++++++++++----------------
 test/stderr/0397 | 6 +-
 5 files changed, 234 insertions(+), 182 deletions(-)

dmarc.c is not used in the default build configuration, but the patch
is broken and causes a failed build when it is enabled.

An easy way to test this is to download the source package, edit
the source file src/EDITME:

-# EXPERIMENTAL_SPF=yes
-# CFLAGS += -I/usr/local/include
-# LDFLAGS += -lspf2
+EXPERIMENTAL_SPF=yes
+CFLAGS += -I/usr/local/include
+LDFLAGS += -lspf2

-# EXPERIMENTAL_DMARC=yes
-# DMARC_TLD_FILE= /etc/exim/opendmarc.tlds
-# CFLAGS += -I/usr/local/include
-# LDFLAGS += -lopendmarc
+EXPERIMENTAL_DMARC=yes
+DMARC_TLD_FILE= /etc/exim4/opendmarc.tlds
+CFLAGS += -I/usr/local/include
+LDFLAGS += -lopendmarc

and also:
apt install libopendmarc-dev libspf2-dev

Custom builds are actually supposed to be supported by editing special files,
 README.Debian.html says:

"Additionally, the source package offers infrastructure to build your own custom-tailored exim4-daemon-custom which exactly fits your special local needs. The infrastructure to do so is already in place, see debian/rules for instructions. "

Unfortunately, anyone doing that to enable dmarc will have a failing build.
Trisquel enables dmarc in its build, and also failed its build when pulling the update.
Here is the end of the build output which shows the failure:

gcc dmarc.c
gcc -c -g -O2 -D_LARGEFILE_SOURCE -fno-strict-aliasing -Wall -I/usr/local/include -I/usr/local/include -fvisibility=hidden -I. dmarc.c
dmarc.c: In function 'dmarc_send_forensic_report':
dmarc.c:166:47: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
 if ( dmarc_policy == DMARC_POLICY_REJECT && action == DMARC_RESULT_REJECT
dmarc.c:168:47: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
    || dmarc_policy == DMARC_POLICY_NONE && action == DMARC_RESULT_REJECT
dmarc.c:169:47: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
    || dmarc_policy == DMARC_POLICY_NONE && action == DMARC_RESULT_QUARANTINE
dmarc.c: At top level:
dmarc.c:211:1: error: static declaration of 'dmarc_write_history_file' follows non-static declaration
 dmarc_write_history_file()
 ^~~~~~~~~~~~~~~~~~~~~~~~
In file included from dmarc.c:22:0:
dmarc.h:26:5: note: previous declaration of 'dmarc_write_history_file' was here
 int dmarc_write_history_file();
     ^~~~~~~~~~~~~~~~~~~~~~~~
dmarc.c: In function 'dmarc_write_history_file':
dmarc.c:265:25: error: 'f' undeclared (first use in this function)
       (host_checking || f.running_in_test_harness) ? " (not really)" : "");
                         ^
dmarc.c:265:25: note: each undeclared identifier is reported only once for each function it appears in
Makefile:811: recipe for target 'dmarc.o' failed
make[3]: *** [dmarc.o] Error 1
make[3]: Leaving directory '/nocow/t/bionic-exim/exim4-4.90.1/b-exim4-daemon-heavy/build-Linux-x86_64'
Makefile:35: recipe for target 'all' failed
make[2]: *** [all] Error 2
make[2]: Leaving directory '/nocow/t/bionic-exim/exim4-4.90.1/b-exim4-daemon-heavy'
debian/rules:111: recipe for target 'override_dh_auto_build' failed
make[1]: *** [override_dh_auto_build] Error 2
make[1]: Leaving directory '/nocow/t/bionic-exim/exim4-4.90.1'
debian/rules:293: recipe for target 'build' failed
make: *** [build] Error 2
dpkg-buildpackage: error: debian/rules build subprocess returned exit status 2
debuild: fatal error at line 1152:
dpkg-buildpackage -rfakeroot -us -uc -ui -b failed

Tags: patch

Related branches

CVE References

Revision history for this message
Marc Deslauriers (mdeslaur) wrote :

try changing:

(host_checking || f.running_in_test_harness) ? " (not really)" : "");

in the patch to:

(host_checking || running_in_test_harness) ? " (not really)" : "");

to see if that fixes it...

Revision history for this message
Ian Kelling (iank) wrote :

The attached patch fixes things as far as I can tell.
I installed the updated build and sent some email.

Revision history for this message
Ian Kelling (iank) wrote :

the patch wasn't formatted correctly, this one should be better.

Revision history for this message
Marc Deslauriers (mdeslaur) wrote :

Making this bug public in case others have a similar issue.

information type: Private Security → Public Security
Steve Beattie (sbeattie)
Changed in exim4 (Ubuntu):
status: New → Confirmed
Revision history for this message
Ubuntu Foundations Team Bug Bot (crichton) wrote :

The attachment "exim.patch" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

tags: added: patch
Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

Thanks for taking the time to report this bug and make Ubuntu better.

This issue seems to affect only people who are compiling their own versions of exim4. For this reason, the security doesn't plan to act on it. I've subscribed the Ubuntu Server team to this bug, and will set the priority to Low. Unfortunately this usually means that we won't have the time to work on this bug, but given that you have already come up with a patch/workaround for the problem, I think this is mostly fine.

Thank you again!

Changed in exim4 (Ubuntu):
importance: Undecided → Low
Bryce Harrington (bryce)
tags: added: server-todo
Changed in exim4 (Ubuntu Bionic):
importance: Undecided → Low
Changed in exim4 (Ubuntu Hirsute):
importance: Undecided → Low
Changed in exim4 (Ubuntu Focal):
importance: Undecided → Low
Bryce Harrington (bryce)
tags: removed: server-todo
Revision history for this message
Lucas Kanashiro (lucaskanashiro) wrote :

Hirsute has reached the end of standard support.

Changed in exim4 (Ubuntu Hirsute):
status: New → Won't Fix
Revision history for this message
Bryce Harrington (bryce) wrote :

I am able to reproduce this on bionic, but not on focal or newer.

The issue appears to be that in newer code, a bitmask structure was introduced to hold various boolean settings in the application:

  ## globals.c
  struct global_flags f =
  {
    .acl_temp_details = FALSE,
 ...
 .running_in_test_harness = FALSE,
 ...
  };

However, the exim4 code in bionic predates this refactor, and running_in_test_harness is a simple global.

Patch 0051-CVE-2020-28007-Link-attack-in-Exim-s-log-directory.patch adds code that assumes the f global exists:

+/* Write the contents to the history file */
+DEBUG(D_receive)
+ debug_printf("DMARC logging history data for opendmarc reporting%s\n",
+ (host_checking || f.running_in_test_harness) ? " (not really)" : "");
+if (host_checking || f.running_in_test_harness)
+ {
+ DEBUG(D_receive)
+ debug_printf("DMARC history data for debugging:\n%s", history_buffer);
+ }

So yeah, the patch from comment #3 should be applied to patch 0051 (or added as a followup fixup patch). But this is only needed for bionic.

Changed in exim4 (Ubuntu):
status: Confirmed → Fix Released
Changed in exim4 (Ubuntu Focal):
status: New → Fix Released
Changed in exim4 (Ubuntu Hirsute):
status: Won't Fix → Fix Released
Revision history for this message
Bryce Harrington (bryce) wrote :

To reproduce the build failure on bionic:

1. Save this file to Local/Makefile
2. cp exim_monitor/EDITME Local/eximon.conf
3. sudo apt-get install -y exim4 libopendmarc-dev libspf2-dev
4. sudo apt-get build-dep -y exim4
5. make
    (It should build successfully at this point since patches aren't applied)
6. quilt push -a
    make
    This should fail.

Revision history for this message
Bryce Harrington (bryce) wrote :

The updated patch with changes as suggested.

1. Copy this to debian/patches/sec-202105 over the top of the same-named file
2. quilt push -a
3. make
    Should once again build successfully

Changed in exim4 (Ubuntu Bionic):
status: New → Triaged
Bryce Harrington (bryce)
description: updated
Bryce Harrington (bryce)
Changed in exim4 (Ubuntu Bionic):
assignee: nobody → Bryce Harrington (bryce)
status: Triaged → Fix Committed
Revision history for this message
Robie Basak (racb) wrote :

It is unfortunate that this bug was introduced in the security fix. But it is also understandable, since we can only realistically test the build configuration that we actually ship. So sorry, as unfortunate as this is I don't think this is suitable for SRU. We expect only to fix issues in our sources if they affect the binary builds that we ship.

Somebody said:

>
Custom builds are actually supposed to be supported by editing special files,
 README.Debian.html says:

"Additionally, the source package offers infrastructure to build your own custom-tailored exim4-daemon-custom which exactly fits your special local needs. The infrastructure to do so is already in place, see debian/rules for instructions. "

Unfortunately, anyone doing that to enable dmarc will have a failing build.
>

Unfortunately the word "support" has multiple meanings here. Ubuntu is Free Software and that means that we want users to be able to adjust the source to their needs. But just because the package maintainer has left a mechanism to make that easier, doesn't mean that we're going to patch our sources and rebuild our binaries for bugs not present in our builds.

Given that they're rebuilding from source, affected users should be able to patch it as needed themselves, and maintain that patch over security updates as needed.

All changes risk regressing existing unaffected users to some extent or other, and to mitigate that takes engineering review time. Given that this bug doesn't affect our build, I don't think it's worth spending the review time on this case.

Changed in exim4 (Ubuntu Bionic):
status: Fix Committed → Won't Fix
Revision history for this message
Ian Kelling (iank) wrote :

Thank you bryce for this fix. I understand Robie why it isn't being applied and it makes total sense. For myself, I've skipped forward past Bionic to a release that doesn't have this bug. But, having the patch out there might help someone.

Revision history for this message
Robie Basak (racb) wrote :

> But, having the patch out there might help someone.

Absolutely. I should have said that I appreciate the report being here, because it's good to coordinate and share with anyone else affected. Thank you for doing that.

To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Bug attachments

Remote bug watches

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