require transport security by default in the Ext.Mail SmtpAppender

src/changelog/3.5.0/329-ext-mail-tls-by-default.xml

@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="changed">
+  <issue id="329" link="https://github.com/apache/logging-log4net/pull/329"/>
+  <description format="asciidoc">
+    require transport security by default in the `log4net.Ext.Mail` `SmtpAppender`: its
+ `TransportSecurity` now defaults to `Required` instead of `None`, so body and credentials no
+ longer cross the network in cleartext (CWE-319). A relay without `STARTTLS` stops working on
+ upgrade; set `transportSecurity` to `None`, or `enableSsl` to `false`, to keep the old behaviour
+ (audit da18b6fd-f026, implemented by @FreeAndNil)
+  </description>
+</entry>

src/log4net.Ext.Mail.Tests/Appender/SmtpAppenderTest.cs

@@ -201,30 +201,38 @@ public void NtlmAuthenticationUsesTheNtlmSaslMechanism()
     Assert.That(_transport.SaslMechanism!.Credentials.GetCredential(null, null).UserName, Is.EqualTo("user"));
   }
 
+  /// <summary>An operator who configures nothing must not get a cleartext session.</summary>
   [Test]
-  public void EnableSslOffConnectsWithoutTransportSecurity()
+  public void TransportSecurityDefaultsToRequired()
+    => Assert.That(CreateAppender().TransportSecurity, Is.EqualTo(SmtpTransportSecurity.Required));
+
+  /// <summary>The shorthand follows the default of the property it stands for.</summary>
+  [Test]
+  public void EnableSslDefaultsToTrue()
+    => Assert.That(CreateAppender().EnableSsl, Is.True);
+
+  /// <summary>
+  /// The default port is 25, so the default must mean mandatory STARTTLS, not the downgradable Auto.
+  /// </summary>
+  [Test]
+  public void TheDefaultConnectionRequiresStartTls()
   {
     SmtpAppender appender = CreateAppender();
 
     Append(appender);
 
-    Assert.That(_transport.SecureSocketOptions, Is.EqualTo(SecureSocketOptions.None));
+    Assert.That(_transport.SecureSocketOptions, Is.EqualTo(SecureSocketOptions.StartTls));
   }
 
-  /// <summary>
-  /// SecureSocketOptions.Auto is opportunistic away from port 465, so an attacker who strips
-  /// STARTTLS from the EHLO response downgrades the session to plaintext. Asking for SSL has to
-  /// mean mandatory STARTTLS.
-  /// </summary>
   [Test]
-  public void EnableSslOnRequiresStartTls()
+  public void EnableSslOffConnectsWithoutTransportSecurity()
   {
     SmtpAppender appender = CreateAppender();
-    appender.EnableSsl = true;
+    appender.EnableSsl = false;
 
     Append(appender);
 
-    Assert.That(_transport.SecureSocketOptions, Is.EqualTo(SecureSocketOptions.StartTls));
+    Assert.That(_transport.SecureSocketOptions, Is.EqualTo(SecureSocketOptions.None));
   }
 
   /// <summary>
@@ -281,17 +289,17 @@ public void EnableSslIsAShorthandForTransportSecurity()
   {
     SmtpAppender appender = CreateAppender();
 
+    appender.EnableSsl = false;
     Assert.That(appender.TransportSecurity, Is.EqualTo(SmtpTransportSecurity.None));
-    Assert.That(appender.EnableSsl, Is.False);
 
     appender.EnableSsl = true;
     Assert.That(appender.TransportSecurity, Is.EqualTo(SmtpTransportSecurity.Required));
 
     appender.TransportSecurity = SmtpTransportSecurity.StartTlsWhenAvailable;
     Assert.That(appender.EnableSsl, Is.True);
 
-    appender.EnableSsl = false;
-    Assert.That(appender.TransportSecurity, Is.EqualTo(SmtpTransportSecurity.None));
+    appender.TransportSecurity = SmtpTransportSecurity.None;
+    Assert.That(appender.EnableSsl, Is.False);
   }
 
   [Test]
@@ -619,7 +627,7 @@ public void DefaultConstructorUsesTheMailKitTransport()
     Assert.That(appender.Authentication, Is.EqualTo(SmtpAppender.SmtpAuthentication.None));
     Assert.That(appender.SubjectEncoding, Is.EqualTo(Encoding.UTF8));
     Assert.That(appender.BodyEncoding, Is.EqualTo(Encoding.UTF8));
-    Assert.That(appender.EnableSsl, Is.False);
+    Assert.That(appender.EnableSsl, Is.True);
   }
 
   /// <summary>An unbounded send stalls every thread logging through the appender.</summary>

src/log4net.Ext.Mail/Appender/SmtpAppender.cs

@@ -252,11 +252,11 @@ public string? Bcc
   /// are the same setting, so the one assigned last wins.
   /// </para>
   /// <para>
-  /// When <see langword="true"/>, transport security is required: implicit TLS on port 465 and
-  /// <c>STARTTLS</c> on every other port. Connecting fails if the server does not offer TLS,
-  /// rather than continuing unencrypted, which matches the behaviour of
-  /// <see cref="System.Net.Mail.SmtpClient.EnableSsl"/>. Use <see cref="TransportSecurity"/> when
-  /// the server needs something else.
+  /// When <see langword="true"/>, which is the default, transport security is required: implicit
+  /// TLS on port 465 and <c>STARTTLS</c> on every other port. Connecting fails if the server does
+  /// not offer TLS, rather than continuing unencrypted, which matches the behaviour, though not
+  /// the default, of <see cref="System.Net.Mail.SmtpClient.EnableSsl"/>. Use
+  /// <see cref="TransportSecurity"/> when the server needs something else.
   /// </para>
   /// </remarks>
   public bool EnableSsl
@@ -270,7 +270,8 @@ public bool EnableSsl
   /// </summary>
   /// <value>
   /// One of the <see cref="SmtpTransportSecurity"/> values. The default is
-  /// <see cref="SmtpTransportSecurity.None"/>.
+  /// <see cref="SmtpTransportSecurity.Required"/>; set it to
+  /// <see cref="SmtpTransportSecurity.None"/> to allow a cleartext connection.
   /// </value>
   /// <remarks>
   /// <para>
@@ -279,7 +280,7 @@ public bool EnableSsl
   /// <c>STARTTLS</c> is possible.
   /// </para>
   /// </remarks>
-  public SmtpTransportSecurity TransportSecurity { get; set; } = SmtpTransportSecurity.None;
+  public SmtpTransportSecurity TransportSecurity { get; set; } = SmtpTransportSecurity.Required;
 
   /// <summary>
   /// Gets or sets the reply-to e-mail address.

src/log4net.Ext.Mail/README.md

@@ -64,10 +64,10 @@ Then configure log4net in your code:
 | Feature | Built-in | log4net.Ext.Mail.Appender.SmtpAppender |
 |---------|----------|----------------------------------------|
 | SMTP Client | `System.Net.Mail.SmtpClient` (deprecated) | MailKit |
-| TLS Support | Limited | Full (Auto, Explicit, Implicit) |
+| TLS Support | Limited | Required, ImplicitTls, StartTls, StartTlsWhenAvailable |
 | Modern Servers | Often fails | ✓ Recommended |
 | `smtpHost` | Optional | Required |
-| `enableSsl` | true/false | true/false (negotiates Auto/None) |
+| `enableSsl` | true/false (default: false) | true/false (default: true) |
 | `Ntlm` auth | Uses Windows logon | Requires explicit credentials |
 
 ## Configuration Options
@@ -83,7 +83,8 @@ Then configure log4net in your code:
 - `bcc` - blind carbon copy recipients
 - `replyTo` - reply-to address
 - `port` - SMTP port (default: 25)
-- `enableSsl` - negotiate TLS/STARTTLS (true/false, default: false)
+- `enableSsl` - require TLS/STARTTLS (true/false, default: true)
+- `transportSecurity` - `None`, `Required` (default), `ImplicitTls`, `StartTls` or `StartTlsWhenAvailable`
 - `authentication` - `None`, `Basic`, or `Ntlm` (default: `None`)
 - `username` - username for authentication
 - `password` - password for authentication

src/site/antora/modules/ROOT/pages/manual/configuration/appenders/smtpappender.adoc

@@ -194,11 +194,11 @@ This example authenticates against a mail server that requires an encrypted conn
 The mail goes out under the appender lock, so an unresponsive server would otherwise stall logging.
 
 |`enableSsl`
-|Whether to require transport security. Defaults to `false`.
+|Whether to require transport security. Defaults to `true`.
 Shorthand for `transportSecurity`: `true` selects `Required`, `false` selects `None`.
 
 |`transportSecurity`
-|How the connection is secured. One of `None` (default), `Required`, `ImplicitTls`, `StartTls` or `StartTlsWhenAvailable`.
+|How the connection is secured. One of `Required` (default), `None`, `ImplicitTls`, `StartTls` or `StartTlsWhenAvailable`.
 See xref:#mailkit-smtpappender-transport-security[].
 
 |`authentication`
@@ -253,11 +253,11 @@ The two properties cannot disagree: whichever is assigned last wins.
 |Value |Description
 
 |`None`
-|The connection is not encrypted. Equivalent to `enableSsl` set to `false`, and the default.
+|The connection is not encrypted. Equivalent to `enableSsl` set to `false`, and the opt-out from the secure default.
 
 |`Required`
 |Transport security is required and the mechanism follows the port: implicit TLS on port 465,
-`STARTTLS` on every other port. Equivalent to `enableSsl` set to `true`.
+`STARTTLS` on every other port. Equivalent to `enableSsl` set to `true`, and the default.
 
 |`ImplicitTls`
 |The session is encrypted before the SMTP greeting, whatever the port.
@@ -298,7 +298,8 @@ server no longer stalls logging. Failures are reported to the error handler as b
 the logging call has returned, and mail still queued is lost if the process is killed.
 `sendQueueSize` and `enqueueTimeoutMillis` bound the queue.
 
-`enableSsl` keeps its meaning, so a migrated configuration secures the connection exactly as before.
+`enableSsl` keeps its meaning but not its default: it is `true` here and `false` in the legacy
+appender, so a configuration that set neither option now requires transport security.
 If the legacy appender reached your server with `enableSsl` set to `true`, so does this one.
 The new `transportSecurity` option is only needed for a server that the legacy appender could not
 reach either, such as one expecting implicit TLS on a port other than 465.