Re: PATCH: smtpd: don't strcmp() NULL in mta_relay_cmp()

[Gilles Chehade]

On Fri, Sep 13, 2019 at 09:34:33PM +0200, Caspar Schutijser wrote:
> Hi,
> 

Hi,


> smtpd crashed on one of my machines. I attempted to start it again but
> it crashed again after a few seconds.
> 
> [...]
> 
> So I think the problem is that b->backupname is passed to strcmp()
> without checking whether b->backupname != NULL. The diff below adds
> such a check, in line with other string comparisons that are performed
> in mta_relay_cmp(). In the same function, I think the same problem
> exists while comparing a->authlabel and b->authlabel so I added a
> similar check there.
> 
> I'd like you to double-check whether the fix is indeed correct.
> 

Yes, your understanding of the problem is correct and your diff also, so
I committed it a minute ago, thanks !

I'm curious about what configuration allowed you to hit this though, can
you share your smtpd.conf with me ?

-- 
Gilles Chehade @poolpOrg

https://www.poolp.orgpatreon: https://www.patreon.com/gilles

PATCH: smtpd: don't strcmp() NULL in mta_relay_cmp()

[Caspar Schutijser]

Hi,

smtpd crashed on one of my machines. I attempted to start it again but
it crashed again after a few seconds.

After doing
  # sysctl kern.nosuidcoredump=3
  # mkdir -m 700 /var/crash/smtpd
and rebuilding smtpd with DEBUG=-g I obtained the following information:


# egdb /usr/src/usr.sbin/smtpd/smtpd/smtpd 4142.core  
GNU gdb (GDB) 7.12.1
Copyright (C) 2017 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-openbsd6.6".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:
.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /usr/src/usr.sbin/smtpd/smtpd/smtpd...done.
Illegal process-id: 4142.core.
[New process 382296]
Core was generated by `smtpd'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  strcmp () at /usr/src/lib/libc/arch/amd64/string/strcmp.S:47
47  movq8(%rsi),%rdx
(gdb) frame 1
#1  0x06b086ba3478 in mta_relay_cmp (a=0x7f7c5e90, b=0x6b34ad15200) at 
../mta.c:2077
2077if (a->backupname && ((r = strcmp(a->backupname, 
b->backupname
(gdb) print a->backupname
$1 = 0x6b2c800f9a0 "tank.msrv.nl"
(gdb) print b->backupname
$2 = 0x0


So I think the problem is that b->backupname is passed to strcmp()
without checking whether b->backupname != NULL. The diff below adds
such a check, in line with other string comparisons that are performed
in mta_relay_cmp(). In the same function, I think the same problem
exists while comparing a->authlabel and b->authlabel so I added a
similar check there.

I'd like you to double-check whether the fix is indeed correct.

Thanks,
Caspar Schutijser


Index: mta.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v
retrieving revision 1.228
diff -u -p -r1.228 mta.c
--- mta.c   14 Jun 2019 19:55:25 -  1.228
+++ mta.c   13 Sep 2019 19:28:02 -
@@ -2036,6 +2036,10 @@ mta_relay_cmp(const struct mta_relay *a,
return (1);
if (a->authtable && ((r = strcmp(a->authtable, b->authtable
return (r);
+   if (a->authlabel == NULL && b->authlabel)
+   return (-1);
+   if (a->authlabel && b->authlabel == NULL)
+   return (1);
if (a->authlabel && ((r = strcmp(a->authlabel, b->authlabel
return (r);
if (a->sourcetable == NULL && b->sourcetable)
@@ -2071,6 +2075,10 @@ mta_relay_cmp(const struct mta_relay *a,
if (a->ca_name && ((r = strcmp(a->ca_name, b->ca_name
return (r);
 
+   if (a->backupname == NULL && b->backupname)
+   return (-1);
+   if (a->backupname && b->backupname == NULL)
+   return (1);
if (a->backupname && ((r = strcmp(a->backupname, b->backupname
return (r);