From d5cd8bb098aa78d8d62c9645f3c532689ef1cb03 Mon Sep 17 00:00:00 2001
From: Benoit TELLIER <btellier@linagora.com>
Date: Mon, 25 Dec 2023 04:14:20 +0100
Subject: [PATCH]  [FIX] Enforce CRLF as part of SMTP DATA transaction (#1877)

---
 .../org/apache/james/LmtpIntegrationTest.java | 26 +++++++++++++++++++
 .../DataLineJamesMessageHookHandler.java      | 25 ++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/server/apps/memory-app/src/test/java/org/apache/james/LmtpIntegrationTest.java b/server/apps/memory-app/src/test/java/org/apache/james/LmtpIntegrationTest.java
index 134c7fa1869..a11074481a4 100644
--- a/server/apps/memory-app/src/test/java/org/apache/james/LmtpIntegrationTest.java
+++ b/server/apps/memory-app/src/test/java/org/apache/james/LmtpIntegrationTest.java
@@ -85,6 +85,32 @@ void lmtpShouldBeConfigurableToReport(GuiceJamesServer guiceJamesServer) throws
                 "451 4.0.0 Temporary error deliver message <error@domain.tld>");
     }
 
+    @Test
+    void preventSMTPSmugglingAttacksByEnforcingCRLF(GuiceJamesServer guiceJamesServer) throws Exception {
+        SocketChannel server = SocketChannel.open();
+        server.connect(new InetSocketAddress(LOCALHOST_IP, guiceJamesServer.getProbe(LmtpGuiceProbe.class).getLmtpPort()));
+        readBytes(server);
+
+        server.write(ByteBuffer.wrap(("LHLO <" + DOMAIN + ">\r\n").getBytes(StandardCharsets.UTF_8)));
+        readBytes(server);
+        server.write(ByteBuffer.wrap(("MAIL FROM: <user@" + DOMAIN + ">\r\n").getBytes(StandardCharsets.UTF_8)));
+        readBytes(server);
+        server.write(ByteBuffer.wrap(("RCPT TO: <user@" + DOMAIN + ">\r\n").getBytes(StandardCharsets.UTF_8)));
+        readBytes(server);
+        server.write(ByteBuffer.wrap(("DATA\r\n").getBytes(StandardCharsets.UTF_8)));
+        readBytes(server); // needed to synchronize
+        server.write(ByteBuffer.wrap((
+            "header:value\r\n\r\nbody 1\r\n.\nMAIL FROM: <a@toto.com>\r\n" +
+                "RCPT TO: <user@domain.tld>\r\n" +
+                "DATA\r\n" +
+                "header: yolo 2\r\n" +
+                "\r\nbody 2\r\n.\r\n").getBytes(StandardCharsets.UTF_8)));
+        byte[] dataResponse = readBytes(server);
+        server.write(ByteBuffer.wrap(("QUIT\r\n").getBytes(StandardCharsets.UTF_8)));
+
+        assertThat(new String(dataResponse, StandardCharsets.UTF_8))
+            .contains("500 5.0.0 line delimiter must be CRLF");
+    }
 
     private byte[] readBytes(SocketChannel channel) throws IOException {
         ByteBuffer line = ByteBuffer.allocate(1024);
diff --git a/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java b/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java
index 3a09c74848b..6793ff91a3c 100644
--- a/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java
+++ b/server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java
@@ -36,6 +36,7 @@
 import org.apache.james.protocols.api.handler.ExtensibleHandler;
 import org.apache.james.protocols.api.handler.LineHandler;
 import org.apache.james.protocols.api.handler.WiringException;
+import org.apache.james.protocols.netty.CommandInjectionDetectedException;
 import org.apache.james.protocols.smtp.MailEnvelope;
 import org.apache.james.protocols.smtp.SMTPResponse;
 import org.apache.james.protocols.smtp.SMTPRetCode;
@@ -61,6 +62,22 @@
  */
 public class DataLineJamesMessageHookHandler implements DataLineFilter, ExtensibleHandler {
     private static final Logger LOGGER = LoggerFactory.getLogger(DataLineJamesMessageHookHandler.class);
+    public static final boolean DETECT_SMTP_SMUGGLING = System.getProperty("james.prevent.smtp.smuggling", "true").equals("true");
+
+    /*
+    SMTP smuggling: https://sec-consult.com/blog/detail/smtp-smuggling-spoofing-e-mails-worldwide/
+    Strict CRLF enforcement rationals: https://haraka.github.io/barelf
+     */
+    private static void detectSMTPSmuggling(byte[] line) {
+        if (DETECT_SMTP_SMUGGLING) {
+            if (line.length < 2
+                || line[line.length - 2] != '\r'
+                || line[line.length - 1] != '\n') {
+
+                throw new CommandInjectionDetectedException();
+            }
+        }
+    }
 
     private List<JamesMessageHook> messageHandlers;
 
@@ -70,11 +87,13 @@ public class DataLineJamesMessageHookHandler implements DataLineFilter, Extensib
 
     @Override
     public Response onLine(SMTPSession session, byte[] line, LineHandler<SMTPSession> next) {
+
         ExtendedSMTPSession extendedSMTPSession = (ExtendedSMTPSession) session;
         MimeMessageInputStreamSource mmiss = extendedSMTPSession.getMimeMessageWriter();
 
         try {
             OutputStream out = mmiss.getWritableOutputStream();
+            detectSMTPSmuggling(line);
 
             // 46 is "."
             // Stream terminated
@@ -129,6 +148,12 @@ public Response onLine(SMTPSession session, byte[] line, LineHandler<SMTPSession
             SMTPResponse response = new SMTPResponse(SMTPRetCode.LOCAL_ERROR, DSNStatus.getStatus(DSNStatus.TRANSIENT, DSNStatus.UNDEFINED_STATUS) + " Error processing message: " + e.getMessage());
             LOGGER.error("Unknown error occurred while processing DATA.", e);
             return response;
+        } catch (CommandInjectionDetectedException e) {
+            LifecycleUtil.dispose(mmiss);
+            SMTPResponse response = new SMTPResponse(SMTPRetCode.SYNTAX_ERROR_COMMAND_UNRECOGNIZED, DSNStatus.getStatus(DSNStatus.PERMANENT, DSNStatus.UNDEFINED_STATUS) + " line delimiter must be CRLF");
+            LOGGER.info("Use of CRLF, which might indicate SMTP smuggling attempt");
+            return response;
+
         }
         return null;
     }