Bug 242861

Summary: Sendmail's STARTTLS broken for 12.1-RELEASE/12.0-RELEASE/12-STABLE/HEAD
Product: Base System Reporter: Dan Lukes <dan+freebsd.org>
Component: binAssignee: Hiroki Sato <hrs>
Status: Closed FIXED    
Severity: Affects Some People CC: dinoex, gshapiro, hrs
Priority: --- Keywords: patch
Version: 12.1-RELEASE   
Hardware: Any   
OS: Any   
See Also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=242862
Attachments:
Description Flags
tls.c::sm_RSA_generate_key() patch, based on revision 463590 of patch-tls.c
none
tls.c::sm_RSA_generate_key() patch, simpler version none

Description Dan Lukes 2019-12-24 15:18:44 UTC 
Created attachment 210201 [details]
tls.c::sm_RSA_generate_key() patch, based on revision 463590 of patch-tls.c

https://svnweb.freebsd.org/base?view=revision&revision=339260

Revision 339260 update made sendmail compatibible with OpenSSL 1.1.1. The change has been based on patch-tls.c taken from mail/sendmail port.

It introduces sm_RSA_generate_key() function used replacement of original RSA_generate_key/RSA_generate_key_ex function.

It's code is broken badly. It may use NULL pointer and it returns NULL all the times.

Code fragment of sm_RSA_generate_key() starting with:
 -----
bn_rsa_r4 = BN_new();
rc = BN_set_word(bn_rsa_r4, RSA_F4);
if ((bn_rsa_r4 != NULL) && BN_set_word(bn_rsa_r4, RSA_F4) && (rsa = RSA_new()) != NULL)
 -----
... is broken badly.

It calls BN_set_word(bn_rsa_r4, RSA_F4) twice, first time before the bn_rsa_r4 has been verified non-NULL causing possible NULL dereferrence. 

The "if" body returns NULL if call to RSA_generate_key_ex() has succeeded. It's main issue causing the sm_RSA_generate_key() is returning NULL all the times.

But it's not the only issue - code here is just wrong. It doesn't free bn_rsa_r4 most of time causing memory leak. It doesn't honor function parameters using hardcoded constants instead. The bug has been introduced in revision 463887 of the patch-tls.c port's patch:
https://svnweb.freebsd.org/ports?view=revision&revision=463887

The patch I'm proposing here is based on correct version of port's patch-tls.c (e.g. 463887):
https://svnweb.freebsd.org/ports?view=revision&revision=463590
Comment 1 Dan Lukes 2019-12-24 16:03:13 UTC 
Created attachment 210203 [details]
tls.c::sm_RSA_generate_key() patch, simpler version

Further analysis has discovered all bugs mentioned are caused by two forgotten/excessive lines. The duplicate call of BN_set_word (rc = BN_set_word(bn_rsa_r4, RSA_F4)) and the excessive "return NULL" that should not be here at all.

This version of patch proposes much smaller change than the previous one.
Comment 2 Hiroki Sato freebsd_committer freebsd_triage 2019-12-24 20:15:58 UTC 
Take.
Comment 3 Hiroki Sato freebsd_committer freebsd_triage 2020-02-17 19:33:55 UTC 
Sorry for the delay.  Staged at https://reviews.freebsd.org/D23734
Comment 4 commit-hook freebsd_committer freebsd_triage 2020-02-27 19:40:50 UTC 
A commit references this bug:

Author: hrs
Date: Thu Feb 27 19:40:29 UTC 2020
New revision: 358404
URL: https://svnweb.freebsd.org/changeset/base/358404

Log:
  Fix broken STARTTLS when SharedMemoryKey is enabled.

  OpenSSL 1.1 API patch for sendmail had a bug which
  prevented sm_RSA_generate_key() function from working.
  This function is used to generate a temporary RSA key
  for a shared memory region used for TLS processing.
  Note that 12.0 and 12.1-RELEASE include this bug.

  This affects only if SM_CONF_SHM compile-time
  option (enabled by default) and SharedMemoryKey
  run-time option (not enabled by default) in a .cf file are
  specified.  The latter corresponds to confSHARED_MEMORY_KEY in
  a .mc file.

  PR:		242861
  MFC after:	3 days
  Differential Revision:	https://reviews.freebsd.org/D23734

Changes:
  head/contrib/sendmail/src/tls.c
Comment 5 commit-hook freebsd_committer freebsd_triage 2020-03-18 18:22:18 UTC 
A commit references this bug:

Author: hrs
Date: Wed Mar 18 18:21:59 UTC 2020
New revision: 359081
URL: https://svnweb.freebsd.org/changeset/base/359081

Log:
  MFC of r358404, r358410, r358412, and r358413:

  Fix broken STARTTLS when SharedMemoryKey is enabled.

  OpenSSL 1.1 API patch for sendmail had a bug which
  prevented sm_RSA_generate_key() function from working.
  This function is used to generate a temporary RSA key
  for a shared memory region used for TLS processing.
  Note that 12.0 and 12.1-RELEASE include this bug.

  This affects only if SM_CONF_SHM compile-time
  option (enabled by default) and SharedMemoryKey
  run-time option (not enabled by default) in a .cf file are
  specified.  The latter corresponds to confSHARED_MEMORY_KEY in
  a .mc file.

  Fix style inconsistencies.

  Do not free p and g parameters after calling DH_set0_pqg(3).

  PR:		242861
  Differential Revision:	https://reviews.freebsd.org/D23734

Changes:
_U  stable/12/
  stable/12/contrib/sendmail/src/tls.c