Bug#818377: improve courier-maildrop compatibility

[Osamu Aoki]

Hi,

I thought about this   I now agree with you on the runtime detection
approach you are talking.

On Tue, Jan 02, 2018 at 10:01:06AM +0100, Markus Wanner wrote:
> On 01/02/2018 01:15 AM, Josip Rodin wrote:
> > All of those changes related to HAVE_COURIER sound like something that
> > should be possible to figure out on runtime.

Relying on the existence of other files may not be the best idea.  I
now think of a simpler runtime detection method.

If "maildrop" binary has its file permission with "suid root",
HAVE_COURIER equivalent dynamic variable to be true.  If a malicious
user have right to set such capability to enable this, you system is
already compromised.

> That's exactly what I thought as well and proposed, but upstream
> rejected as an additional security risk.

Maybe it's worth trying again with exact patch.

> > I still don't see a rationale for that. The existence of those measly few
> > lines about the HAVE_COURIER define, that we then have to interpret and
> > reverse-engineer and whatnot - simply don't constitute a valid rationale
> > for adding back a binary with suid root by default.
> 
> Exactly my line of thought as well.

If courier platform doesn't require it for sure, then please don't set
suid root.  If you need suid root for courier, add a courier-maildrop
package which depends on maildrop and have postinst/rm script to set
permission via dpkg-statoverride mechanism on the patched binary as
discussed above.  Or add README.Debian of maildrop to instruct this
dpkg-statoverride trick without adding the courier-maildrop package.
 
> > I think we need to ask Sam to document this properly, and only then proceed
> > with any further considerations.
> 
> That's fine with me.

Yes.

Anyway,  let's forward debian/patches/0003-permanent-err.patch first.
That will reduce one occurrence of HAVE_COURIER.  This basically changes
non-courier maildrop to behave like updated courier one.  I think this
moves key discussion of HAVE_COURIER to a single meaningful point.

Regards,

Osamu

Bug#818377: improve courier-maildrop compatibility

[Markus Wanner]

On 01/02/2018 01:15 AM, Josip Rodin wrote:
> All of those changes related to HAVE_COURIER sound like something that
> should be possible to figure out on runtime.

That's exactly what I thought as well and proposed, but upstream
rejected as an additional security risk.

> I still don't see a rationale for that. The existence of those measly few
> lines about the HAVE_COURIER define, that we then have to interpret and
> reverse-engineer and whatnot - simply don't constitute a valid rationale
> for adding back a binary with suid root by default.

Exactly my line of thought as well.

> I think we need to ask Sam to document this properly, and only then proceed
> with any further considerations.

That's fine with me.

Kind Regards

Markus Wanner

Bug#818377: improve courier-maildrop compatibility

[Josip Rodin]

On Sun, Dec 31, 2017 at 10:09:22AM +0900, Osamu Aoki wrote:
> | AC_DEFINE_UNQUOTED(HAVE_COURIER,1,
> | [ Whether this version of maildrop is part of Courier ])

All of those changes related to HAVE_COURIER sound like something that
should be possible to figure out on runtime. For example, it could detect
some Courier-specific config file somewhere in /etc/, and then make those
few subtle changes in behavior.

> But in the courier MTA use case, the upstream apparently had need to keep
> this program setUID root and added some extra codes to take advantage
> (code before the quoted section seems to be for such purpose) of it and to
> limit it privilege as quoted in the above.

I still don't see a rationale for that. The existence of those measly few
lines about the HAVE_COURIER define, that we then have to interpret and
reverse-engineer and whatnot - simply don't constitute a valid rationale
for adding back a binary with suid root by default.

I think we need to ask Sam to document this properly, and only then proceed
with any further considerations.

-- 
 2. That which causes joy or happiness.

Bug#818377: improve courier-maildrop compatibility

[Osamu Aoki]

Hi,

On Fri, Dec 29, 2017 at 09:55:15PM +0100, Markus Wanner wrote:
> On 12/29/2017 09:17 PM, Josip Rodin wrote:
> > On Sun, Dec 24, 2017 at 09:17:28AM +0900, Osamu Aoki wrote:
> >> Another option is create another wrapper code such as maildrop-suid-root
> >> which is a SUID on root program and let it calls maildrop in upstream.
> >> And make courier call this new code.  This needs upstream cooperation.

I think the upstream is cooperating.  It's mostly Debian internal issue,
I think.

> I brought up the issue on their mailing list, but upstream wasn't
> exactly cooperative. They think of maildrop being available in two
> different flavours: standalone and courier-mta bundle.

Yes.  To be precise, as I understand, it was available in two different
flavours for jessie:

 * maildrop (standalone): SGID mail
 * courier-maildrop (previously available with courier-mta): SUID root

But for stretch, courier (0.75.0-15) maintainer changed this:
  ...
  * Kill duplicate courier-maildrop package and just depend on standard
maildrop package
(Closes: #289091, #193650, #567462, #684230)

 -- Ondřej Surý   Wed, 16 Mar 2016 16:45:48 +0100

I suspect this may have been a bad move and thus its broken in
stretch/stable now if I believe bug reports.  (I don't use courier.  If
it is not a problem at all, please close all related bugs.)

They have the same source tree for maildrop but are meant to be build
with differently to accommodate different permission in mind by the
upstream.  Upstream understands Debian maildrop package use case and
kindly accomodeted it by changing build behavior with HAVE_COURIER
variable.  libs/maildrop/confifure.ac has:

| if test -d $srcdir/../../courier
| then
| #
| # This version of maildrop is integrated into Courier mail server
| 
| AC_DEFINE_UNQUOTED(HAVE_COURIER,1,
| [ Whether this version of maildrop is part of Courier ])
| fi

So the binary build in the standalone source tree and in the courier source
tree are different.

Unlike other MTA which uses "root:mail", courier uses different user
"root:courier".  It's postinst hasd the following:

| add_override root courier 4755 /usr/sbin/rmail
| add_override courier courier 2755 /usr/bin/mailq
| add_override courier courier 6755 /usr/bin/cancelmsg
| add_override root courier 2755 /usr/sbin/sendmail
| add_override courier courier 4755 /usr/lib/courier/courier/submitmk
| ...

I am not familiar with the security model of courier-mta, but for some
reason they are using this set up.  If you read maildrop source looking
for HAVE_COURIER, it looks like maildrop source build with this variable
set to 1 to build courier-maildrop has special case handling.  Upstream
make it clear via its comment.  The main difference is found in
libs/maildrop/main.C:
| #if HAVE_COURIER
| "Courier-specific maildrop build. This version of maildrop should 
only be used"
| #if CRLF_TERM
| "\r\n"
| #else
| "\n"
| #endif
| "with Courier, and not any other mail server."
| #if CRLF_TERM
| "\r\n"
| #else
| "\n"
| #endif
| #endif
...

| uid_t   my_u=getuid();
| 
| if (setuid(my_u) < 0)   // Drop any setuid privileges.
| {
| perror("setuid");
| exit(1);
| }
| 
| if (!found)
| {
| #if HAVE_COURIER
| if (!deliverymode)
| #endif
| {
| my_pw=getpwuid(my_u);
| if (!my_pw)
| {
| errexit=EX_TEMPFAIL;
| throw "Cannot determine my username.";
| }
| 
| maildrop.init_home=my_pw->pw_dir;
| maildrop.init_logname=my_pw->pw_name;
| maildrop.init_shell=
| my_pw->pw_shell && *my_pw->pw_shell
| ? my_pw->pw_shell:"/bin/sh";
| maildrop.init_default=
| GetDefaultMailbox(my_pw->pw_name);
| }
| }

(delivery.C difference is actually none after the Debian patch which I
should upstream.)

> >> I don't want to maintain any SUID root program.  Too much
> >> responsibility.  If you are willing to take over this package
> >> maintenance, I can help 2 binary package script.
> > 
> > It doesn't make sense to add a new setuid root binary in the maildrop
> > package. Whatever problem there is to solve, more setuid root by default
> > is simply not a legitimate solution without a big fat obvious rationale
> > about how it's unavoidable. Let's not jump to any such conclusions
> > beforehand.

Upstream basically agrees with your assessment for our standalone use
case and accommodated it.  But in the courier MTA use case, the upstream
apparently had need to keep this program setUID root and added some
extra codes to take 

Bug#818377: improve courier-maildrop compatibility

[Markus Wanner]

On 12/29/2017 09:17 PM, Josip Rodin wrote:
> On Sun, Dec 24, 2017 at 09:17:28AM +0900, Osamu Aoki wrote:
>> Another option is create another wrapper code such as maildrop-suid-root
>> which is a SUID on root program and let it calls maildrop in upstream.
>> And make courier call this new code.  This needs upstream cooperation.

I brought up the issue on their mailing list, but upstream wasn't
exactly cooperative. They think of maildrop being available in two
different flavours: standalone and courier-mta bundle.

>> I don't want to maintain any SUID root program.  Too much
>> responsibility.  If you are willing to take over this package
>> maintenance, I can help 2 binary package script.
> 
> It doesn't make sense to add a new setuid root binary in the maildrop
> package. Whatever problem there is to solve, more setuid root by default
> is simply not a legitimate solution without a big fat obvious rationale
> about how it's unavoidable. Let's not jump to any such conclusions
> beforehand.

I wonder if SUID is related at all. In my own setup, while using virtual
domains in combination with maildrop, it wouldn't even need SGID, I
think. But I'd have to check why it was SUID to begin with. Or if it is
in the variant upstream installs.

Kind Regards

Markus Wanner

Bug#818377: improve courier-maildrop compatibility

[Josip Rodin]

On Sun, Dec 24, 2017 at 09:17:28AM +0900, Osamu Aoki wrote:
> Another option is create another wrapper code such as maildrop-suid-root
> which is a SUID on root program and let it calls maildrop in upstream.
> And make courier call this new code.  This needs upstream cooperation.  
> 
> I don't want to maintain any SUID root program.  Too much
> responsibility.  If you are willing to take over this package
> maintenance, I can help 2 binary package script.

It doesn't make sense to add a new setuid root binary in the maildrop
package. Whatever problem there is to solve, more setuid root by default
is simply not a legitimate solution without a big fat obvious rationale
about how it's unavoidable. Let's not jump to any such conclusions
beforehand.

-- 
 2. That which causes joy or happiness.

Bug#818377: improve courier-maildrop compatibility

[Osamu Aoki]

Hi,

I just uploaded 2.9.3-1.  (CCed recent uploaders)

On Fri, Mar 24, 2017 at 04:40:28PM +0100, Markus Wanner wrote:
> Control: block 822683 by 818377
> 
> Hi,
> 
> I've recently migrated my Courier MTA setup to stretch and had to go
> through a few hoops to get it to work, again.
> 
> An important aspect was the courier-maildrop dump. With the packager's
> hat on, I'm also all for the drop and don't want to re-duplicate
> sources. This however means I'd like maildrop to handle the courier use
> case.
> 
> The good news is: my virtual mail delivery setup via maildrop works if
> only I enable HAVE_COURIER for my custom-built maildrop package.
> 
> Reading the sources, it doesn't seem feasible to just enable
> HAVE_COURIER for the general maildrop build, though. So I'd like to
> discuss some options that spring to mind:

Yes.  Excuse me for slow response.

>  * change HAVE_COURIER into a dynamic flag: this might well have
>security implications that I'm unaware of. Note, however, that
>the courier-maildrop was SUID on root, while maildrop only has
>the SGID bit set for group 'mail'. So courier-maildrop was *more*
>of a security risk, not less.

That is my understanding too.

>This could (or should?) possibly be extended by some mechanism that
>automatically detects whether or not courier is calling the maildrop
>executable. Extended (or different) behaviour could be prohibited
>for a non-courier caller.

I am afraid such package is labeled with "security bug" again.  That was
the reason Debian maildrop split from courier-maildrop.

>  * build two different binaries from the maildrop source, one as it
>is, the other with HAVE_COURIER enabled.

This is one way. Hmmm this seems quite simple to do. 

> Are there other options? I'm certainly willing to help and hope to find
> a solution for stretch that fixes the courier use case.

Another option is create another wrapper code such as maildrop-suid-root
which is a SUID on root program and let it calls maildrop in upstream.
And make courier call this new code.  This needs upstream cooperation.  

I don't want to maintain any SUID root program.  Too much
responsibility.  If you are willing to take over this package
maintenance, I can help 2 binary package script.

Regards,

Osamu

Bug#818377: improve courier-maildrop compatibility

[Markus Wanner]

Control: block 822683 by 818377

Hi,

I've recently migrated my Courier MTA setup to stretch and had to go
through a few hoops to get it to work, again.

An important aspect was the courier-maildrop dump. With the packager's
hat on, I'm also all for the drop and don't want to re-duplicate
sources. This however means I'd like maildrop to handle the courier use
case.

The good news is: my virtual mail delivery setup via maildrop works if
only I enable HAVE_COURIER for my custom-built maildrop package.

Reading the sources, it doesn't seem feasible to just enable
HAVE_COURIER for the general maildrop build, though. So I'd like to
discuss some options that spring to mind:

 * change HAVE_COURIER into a dynamic flag: this might well have
   security implications that I'm unaware of. Note, however, that
   the courier-maildrop was SUID on root, while maildrop only has
   the SGID bit set for group 'mail'. So courier-maildrop was *more*
   of a security risk, not less.

   This could (or should?) possibly be extended by some mechanism that
   automatically detects whether or not courier is calling the maildrop
   executable. Extended (or different) behaviour could be prohibited
   for a non-courier caller.

 * build two different binaries from the maildrop source, one as it
   is, the other with HAVE_COURIER enabled.

Are there other options? I'm certainly willing to help and hope to find
a solution for stretch that fixes the courier use case.

Kind Regards

Markus Wanner



signature.asc
Description: OpenPGP digital signature