Bug 242861 - Sendmail's STARTTLS broken for 12.1-RELEASE/12.0-RELEASE/12-STABLE/HEAD
Summary: Sendmail's STARTTLS broken for 12.1-RELEASE/12.0-RELEASE/12-STABLE/HEAD
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 12.1-RELEASE
Hardware: Any Any
: --- Affects Some People
Assignee: Hiroki Sato
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-12-24 15:18 UTC by Dan Lukes
Modified: 2020-03-18 18:22 UTC (History)
2 users (show)

See Also:


Attachments
tls.c::sm_RSA_generate_key() patch, based on revision 463590 of patch-tls.c (829 bytes, patch)
2019-12-24 15:18 UTC, Dan Lukes
no flags Details | Diff
tls.c::sm_RSA_generate_key() patch, simpler version (692 bytes, patch)
2019-12-24 16:03 UTC, Dan Lukes
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 2019-12-24 20:15:58 UTC
Take.
Comment 3 Hiroki Sato freebsd_committer 2020-02-17 19:33:55 UTC
Sorry for the delay.  Staged at https://reviews.freebsd.org/D23734
Comment 4 commit-hook freebsd_committer 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 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