Merge pull request #596 JAMES-1862 Generalize STARTTLS sanitizing fix…

… [BACKPORT]
apache · Aug 27, 2021 · fad40c4 · fad40c4
2 parents 90ec73f + 82f3c18
commit fad40c4
Show file tree

Hide file tree

Showing 25 changed files with 197 additions and 58 deletions.
diff --git a/mpt/impl/smtp/core/src/main/java/org/apache/james/mpt/smtp/SmtpStarttlsCommandTest.java b/mpt/impl/smtp/core/src/main/java/org/apache/james/mpt/smtp/SmtpStarttlsCommandTest.java
@@ -48,6 +48,13 @@ void starttlsShouldWork() throws Exception {
         scriptedTest.run("starttls");
     }
 
+    @Test
+    void startTlsShouldBeRejectedWhenAlreadyAuthenticated() throws Exception {
+        // Avoids session fixation attacks as described in https://www.usenix.org/system/files/sec21-poddebniak.pdf
+        // section 6.2
+        scriptedTest.run("starttls_session_fixation");
+    }
+
     @Test
     void starttlsShouldBeRejectedWhenFollowedByCommand() throws Exception {
         scriptedTest.run("starttls_with_injection");

diff --git a/...smtp/core/src/main/resources/org/apache/james/smtp/scripts/starttls_session_fixation.test b/...smtp/core/src/main/resources/org/apache/james/smtp/scripts/starttls_session_fixation.test
@@ -0,0 +1,26 @@
+S: 220 mydomain.tld smtp
+
+C: ehlo yopmail.com
+S: 250.*
+S: 250-AUTH LOGIN PLAIN
+S: 250-AUTH=LOGIN PLAIN
+S: 250-PIPELINING
+S: 250-ENHANCEDSTATUSCODES
+S: 250-8BITMIME
+S: 250 STARTTLS
+
+C: AUTH LOGIN
+S: 334 VXNlcm5hbWU6
+C: Ym9iQG15ZG9tYWluLnRsZA==
+S: 334 UGFzc3dvcmQ6
+C: c2VjcmV0
+S: 235 Authentication Successful
+
+
+C: mail from:<bob@mydomain.tld>
+C: rcpt to:<matthieu@yopmail.com>
+S: 250 2.1.0 Sender <bob@mydomain.tld> OK
+S: 250 2.1.5 Recipient <matthieu@yopmail.com> OK
+
+C: starttls
+S: 501 .*
diff --git a/protocols/api/src/main/java/org/apache/james/protocols/api/CommandDetectionSession.java b/protocols/api/src/main/java/org/apache/james/protocols/api/CommandDetectionSession.java
@@ -0,0 +1,29 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one   *
+ * or more contributor license agreements.  See the NOTICE file *
+ * distributed with this work for additional information        *
+ * regarding copyright ownership.  The ASF licenses this file   *
+ * to you under the Apache License, Version 2.0 (the            *
+ * "License"); you may not use this file except in compliance   *
+ * with the License.  You may obtain a copy of the License at   *
+ *                                                              *
+ *   http://www.apache.org/licenses/LICENSE-2.0                 *
+ *                                                              *
+ * Unless required by applicable law or agreed to in writing,   *
+ * software distributed under the License is distributed on an  *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY       *
+ * KIND, either express or implied.  See the License for the    *
+ * specific language governing permissions and limitations      *
+ * under the License.                                           *
+ ****************************************************************/
+
+package org.apache.james.protocols.api;
+
+// https://nostarttls.secvuln.info/
+public interface CommandDetectionSession {
+    boolean needsCommandInjectionDetection();
+
+    void startDetectingCommandInjection();
+
+    void stopDetectingCommandInjection();
+}
diff --git a/protocols/api/src/main/java/org/apache/james/protocols/api/ProtocolSession.java b/protocols/api/src/main/java/org/apache/james/protocols/api/ProtocolSession.java
@@ -35,7 +35,7 @@
 /**
  * Session for a protocol. Every new connection generates a new session
  */
-public interface ProtocolSession {
+public interface ProtocolSession extends CommandDetectionSession {
 
     enum State {
         Connection,
@@ -235,5 +235,4 @@ public String toString() {
      * @return size of the pushed line handler
      */
     int getPushedLineHandlerCount();
-
 }
diff --git a/protocols/api/src/main/java/org/apache/james/protocols/api/ProtocolSessionImpl.java b/protocols/api/src/main/java/org/apache/james/protocols/api/ProtocolSessionImpl.java
@@ -43,13 +43,30 @@ public class ProtocolSessionImpl implements ProtocolSession {
     private final Map<AttachmentKey<?>, Object> sessionState;
     private Username username;
     protected final ProtocolConfiguration config;
+    private boolean needsCommandInjectionDetection;
     private static final String DELIMITER = "\r\n";
 
     public ProtocolSessionImpl(ProtocolTransport transport, ProtocolConfiguration config) {
         this.transport = transport;
         this.connectionState = new HashMap<>();
         this.sessionState = new HashMap<>();
         this.config = config;
+        this.needsCommandInjectionDetection = true;
+    }
+
+    @Override
+    public boolean needsCommandInjectionDetection() {
+        return needsCommandInjectionDetection;
+    }
+
+    @Override
+    public void startDetectingCommandInjection() {
+        needsCommandInjectionDetection = true;
+    }
+
+    @Override
+    public void stopDetectingCommandInjection() {
+        needsCommandInjectionDetection = false;
     }
 
     @Override

diff --git a/protocols/imap/src/main/java/org/apache/james/imap/api/process/ImapSession.java b/protocols/imap/src/main/java/org/apache/james/imap/api/process/ImapSession.java
@@ -26,6 +26,7 @@
 import org.apache.james.core.Username;
 import org.apache.james.imap.api.ImapSessionState;
 import org.apache.james.mailbox.MailboxSession;
+import org.apache.james.protocols.api.CommandDetectionSession;
 
 /**
  * Encapsulates all state held for an ongoing Imap session, which commences when
@@ -34,7 +35,7 @@
  * 
  * @version $Revision: 109034 $
  */
-public interface ImapSession {
+public interface ImapSession extends CommandDetectionSession {
     class SessionId {
         private static final RandomStringGenerator RANDOM_STRING_GENERATOR = new RandomStringGenerator.Builder()
             .withinRange('a', 'z')

diff --git a/protocols/imap/src/main/java/org/apache/james/imap/encode/FakeImapSession.java b/protocols/imap/src/main/java/org/apache/james/imap/encode/FakeImapSession.java
@@ -40,6 +40,21 @@ public FakeImapSession() {
         this.attributesByKey = new ConcurrentHashMap<>();
     }
 
+    @Override
+    public boolean needsCommandInjectionDetection() {
+        throw new UnsupportedOperationException();
+    }
+
+    @Override
+    public void startDetectingCommandInjection() {
+
+    }
+
+    @Override
+    public void stopDetectingCommandInjection() {
+
+    }
+
     @Override
     public SessionId sessionId() {
         return sessionId;

diff --git a/protocols/imap/src/main/java/org/apache/james/imap/processor/AbstractAuthProcessor.java b/protocols/imap/src/main/java/org/apache/james/imap/processor/AbstractAuthProcessor.java
@@ -73,6 +73,7 @@ protected void doAuth(AuthenticationAttempt authenticationAttempt, ImapSession s
                     session.setMailboxSession(mailboxSession);
                     provisionInbox(session, mailboxManager, mailboxSession);
                     okComplete(request, responder);
+                    session.stopDetectingCommandInjection();
                 } catch (BadCredentialsException e) {
                     authFailure = true;
                 }

diff --git a/protocols/imap/src/main/java/org/apache/james/imap/processor/AppendProcessor.java b/protocols/imap/src/main/java/org/apache/james/imap/processor/AppendProcessor.java
@@ -64,6 +64,8 @@ protected void processRequest(AppendRequest request, ImapSession session, Respon
         final Flags flags = request.getFlags();
         final MailboxPath mailboxPath = PathConverter.forSession(session).buildFullPath(mailboxName);
 
+        session.stopDetectingCommandInjection();
+
         try {
             final MailboxManager mailboxManager = getMailboxManager();
             final MessageManager mailbox = mailboxManager.getMailbox(mailboxPath, session.getMailboxSession());

diff --git a/protocols/imap/src/main/java/org/apache/james/imap/processor/AuthenticateProcessor.java b/protocols/imap/src/main/java/org/apache/james/imap/processor/AuthenticateProcessor.java
@@ -96,6 +96,7 @@ protected void doPlainAuth(String initialClientResponse, ImapSession session, Im
         } else {
             doAuth(authenticationAttempt, session, request, responder, HumanReadableText.AUTHENTICATION_FAILED);
         }
+        session.stopDetectingCommandInjection();
     }
 
     private AuthenticationAttempt parseDelegationAttempt(String initialClientResponse) {

diff --git a/...llButStartTlsLineBasedChannelHandler.java → ...llButStartTlsLineBasedChannelHandler.java b/...llButStartTlsLineBasedChannelHandler.java → ...llButStartTlsLineBasedChannelHandler.java
@@ -16,13 +16,13 @@
  * specific language governing permissions and limitations      *
  * under the License.                                           *
  ****************************************************************/
-package org.apache.james.protocols.smtp;
+package org.apache.james.protocols.netty;
 
 import java.nio.charset.StandardCharsets;
 import java.util.List;
 import java.util.Locale;
 
-import org.apache.james.protocols.netty.HandlerConstants;
+import org.apache.james.protocols.api.CommandDetectionSession;
 import org.jboss.netty.buffer.ChannelBuffer;
 import org.jboss.netty.channel.Channel;
 import org.jboss.netty.channel.ChannelHandlerContext;
@@ -32,21 +32,20 @@
 import com.google.common.base.CharMatcher;
 import com.google.common.base.Splitter;
 
-
 public class AllButStartTlsLineBasedChannelHandler extends LineBasedFrameDecoder {
-
-    private static final String STARTTLS = "starttls";
     private static final Boolean FAIL_FAST = true;
     private final ChannelPipeline pipeline;
+    private final String pattern;
 
-    public AllButStartTlsLineBasedChannelHandler(ChannelPipeline pipeline, int maxFrameLength, boolean stripDelimiter) {
+    public AllButStartTlsLineBasedChannelHandler(ChannelPipeline pipeline, int maxFrameLength, boolean stripDelimiter, String pattern) {
         super(maxFrameLength, stripDelimiter, !FAIL_FAST);
         this.pipeline = pipeline;
+        this.pattern = pattern;
     }
 
     @Override
     protected Object decode(ChannelHandlerContext ctx, Channel channel, ChannelBuffer buffer) throws Exception {
-        SMTPSession session = (SMTPSession) pipeline.getContext(HandlerConstants.CORE_HANDLER).getAttachment();
+        CommandDetectionSession session = retrieveSession(ctx, channel);
 
         if (session == null || session.needsCommandInjectionDetection()) {
             String trimedLowerCasedInput = readAll(buffer).trim().toLowerCase(Locale.US);
@@ -57,6 +56,10 @@ protected Object decode(ChannelHandlerContext ctx, Channel channel, ChannelBuffe
         return super.decode(ctx, channel, buffer);
     }
 
+    protected CommandDetectionSession retrieveSession(ChannelHandlerContext ctx, Channel channel) {
+        return (CommandDetectionSession) pipeline.getContext(HandlerConstants.CORE_HANDLER).getAttachment();
+    }
+
     private String readAll(ChannelBuffer buffer) {
         return buffer.toString(StandardCharsets.US_ASCII);
     }
@@ -68,13 +71,13 @@ private boolean hasCommandInjection(String trimedLowerCasedInput) {
         return hasInvalidStartTlsPart(parts) || multiPartsAndOneStartTls(parts);
     }
 
-    private boolean multiPartsAndOneStartTls(List<String> parts) {
+    protected boolean multiPartsAndOneStartTls(List<String> parts) {
         return parts.stream()
-            .anyMatch(line -> line.startsWith(STARTTLS)) && parts.size() > 1;
+            .anyMatch(line -> line.startsWith(pattern)) && parts.size() > 1;
     }
 
-    private boolean hasInvalidStartTlsPart(List<String> parts) {
+    protected boolean hasInvalidStartTlsPart(List<String> parts) {
         return parts.stream()
-            .anyMatch(line -> line.startsWith(STARTTLS) && !line.endsWith(STARTTLS));
+            .anyMatch(line -> line.startsWith(pattern) && !line.endsWith(pattern));
     }
 }
diff --git a/...ButStartTlsLineChannelHandlerFactory.java → ...ButStartTlsLineChannelHandlerFactory.java b/...ButStartTlsLineChannelHandlerFactory.java → ...ButStartTlsLineChannelHandlerFactory.java
@@ -16,22 +16,22 @@
  * specific language governing permissions and limitations      *
  * under the License.                                           *
  ****************************************************************/
-package org.apache.james.protocols.smtp;
+package org.apache.james.protocols.netty;
 
-import org.apache.james.protocols.netty.ChannelHandlerFactory;
 import org.jboss.netty.channel.ChannelHandler;
 import org.jboss.netty.channel.ChannelPipeline;
 
 public class AllButStartTlsLineChannelHandlerFactory implements ChannelHandlerFactory {
-
+    private final String pattern;
     private int maxFrameLength;
 
-    public AllButStartTlsLineChannelHandlerFactory(int maxFrameLength) {
+    public AllButStartTlsLineChannelHandlerFactory(String pattern, int maxFrameLength) {
+        this.pattern = pattern;
         this.maxFrameLength = maxFrameLength;
     }
 
     @Override
     public ChannelHandler create(ChannelPipeline pipeline) {
-        return new AllButStartTlsLineBasedChannelHandler(pipeline, maxFrameLength, false);
+        return new AllButStartTlsLineBasedChannelHandler(pipeline, maxFrameLength, false, pattern);
     }
 }
diff --git a/...tp/CommandInjectionDetectedException.java → ...ty/CommandInjectionDetectedException.java b/...tp/CommandInjectionDetectedException.java → ...ty/CommandInjectionDetectedException.java
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations      *
  * under the License.                                           *
  ****************************************************************/
-package org.apache.james.protocols.smtp;
+package org.apache.james.protocols.netty;
 
 public class CommandInjectionDetectedException extends RuntimeException {
 

diff --git a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/SMTPSession.java b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/SMTPSession.java
@@ -57,12 +57,6 @@ public interface SMTPSession extends ProtocolSession {
      * @return the relaying status
      */
     boolean isRelayingAllowed();
-
-    boolean needsCommandInjectionDetection();
-
-    void startDetectingCommadInjection();
-
-    void stopDetectingCommandInjection();
 
     /**
      * Set if reallying is allowed

diff --git a/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/SMTPSessionImpl.java b/protocols/smtp/src/main/java/org/apache/james/protocols/smtp/SMTPSessionImpl.java
@@ -42,20 +42,6 @@ public SMTPSessionImpl(ProtocolTransport transport, SMTPConfiguration config) {
         needsCommandInjectionDetection = true;
     }
 
-    @Override
-    public boolean needsCommandInjectionDetection() {
-        return needsCommandInjectionDetection;
-    }
-
-    @Override
-    public void startDetectingCommadInjection() {
-        needsCommandInjectionDetection = true;
-    }
-
-    @Override
-    public void stopDetectingCommandInjection() {
-        needsCommandInjectionDetection = false;
-    }
 
     @Override
     public boolean isRelayingAllowed() {

diff --git a/...ols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/StartTlsCmdHandler.java b/...ols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/StartTlsCmdHandler.java
@@ -49,6 +49,7 @@ public class StartTlsCmdHandler implements CommandHandler<SMTPSession>, EhloExte
     private static final Response TLS_ALREADY_ACTIVE = new SMTPResponse("500", DSNStatus.getStatus(DSNStatus.PERMANENT, DSNStatus.DELIVERY_INVALID_CMD) + " TLS already active RFC2487 5.2").immutable();
     private static final Response READY_FOR_STARTTLS = new SMTPStartTlsResponse("220", DSNStatus.getStatus(DSNStatus.SUCCESS, DSNStatus.UNDEFINED_STATUS) + " Ready to start TLS").immutable();
     private static final Response SYNTAX_ERROR = new SMTPResponse("501 " + DSNStatus.getStatus(DSNStatus.PERMANENT, DSNStatus.DELIVERY_INVALID_ARG) + " Syntax error (no parameters allowed) with STARTTLS command").immutable();
+    private static final Response ALREADY_AUTH_ERROR = new SMTPResponse("501 " + DSNStatus.getStatus(DSNStatus.PERMANENT, DSNStatus.DELIVERY_INVALID_ARG) + " Syntax error (invalid command in this state) Already authenticated...").immutable();
     private static final Response NOT_SUPPORTED = new SMTPResponse(SMTPRetCode.SYNTAX_ERROR_COMMAND_UNRECOGNIZED, DSNStatus.getStatus(DSNStatus.PERMANENT, DSNStatus.DELIVERY_INVALID_CMD) + " Command " + COMMAND_NAME + " unrecognized.").immutable();
 
     @Override
@@ -67,6 +68,11 @@ public Response onCommand(SMTPSession session, Request request) {
             if (session.isTLSStarted()) {
                 return TLS_ALREADY_ACTIVE;
             } else {
+                if (session.getUsername() != null) {
+                    // Prevents session fixation as described in https://www.usenix.org/system/files/sec21-poddebniak.pdf
+                    // Session 6.2
+                    return ALREADY_AUTH_ERROR;
+                }
                 String parameters = request.getArgument();
                 if ((parameters == null) || (parameters.length() == 0)) {
                     return READY_FOR_STARTTLS;

diff --git a/...smtp/src/test/java/org/apache/james/protocols/smtp/netty/NettyStartTlsSMTPServerTest.java b/...smtp/src/test/java/org/apache/james/protocols/smtp/netty/NettyStartTlsSMTPServerTest.java
@@ -32,8 +32,6 @@
 import javax.mail.internet.InternetAddress;
 import javax.mail.internet.MimeMessage;
 
-import com.sun.mail.smtp.SMTPTransport;
-
 import org.apache.commons.net.smtp.SMTPReply;
 import org.apache.commons.net.smtp.SMTPSClient;
 import org.apache.james.metrics.tests.RecordingMetricFactory;
@@ -47,8 +45,8 @@
 import org.apache.james.protocols.api.utils.BogusTrustManagerFactory;
 import org.apache.james.protocols.api.utils.ProtocolServerUtils;
 import org.apache.james.protocols.netty.AbstractChannelPipelineFactory;
+import org.apache.james.protocols.netty.AllButStartTlsLineChannelHandlerFactory;
 import org.apache.james.protocols.netty.NettyServer;
-import org.apache.james.protocols.smtp.AllButStartTlsLineChannelHandlerFactory;
 import org.apache.james.protocols.smtp.SMTPConfigurationImpl;
 import org.apache.james.protocols.smtp.SMTPProtocol;
 import org.apache.james.protocols.smtp.SMTPProtocolHandlerChain;
@@ -59,6 +57,8 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import com.sun.mail.smtp.SMTPTransport;
+
 public class NettyStartTlsSMTPServerTest {
 
     private static final String LOCALHOST_IP = "127.0.0.1";
@@ -88,7 +88,7 @@ private ProtocolServer createServer(Protocol protocol, Encryption enc) {
         NettyServer server = new NettyServer.Factory(hashedWheelTimer)
                 .protocol(protocol)
                 .secure(enc)
-                .frameHandlerFactory(new AllButStartTlsLineChannelHandlerFactory(AbstractChannelPipelineFactory.MAX_LINE_LENGTH))
+                .frameHandlerFactory(new AllButStartTlsLineChannelHandlerFactory("starttls", AbstractChannelPipelineFactory.MAX_LINE_LENGTH))
                 .build();
         server.setListenAddresses(new InetSocketAddress(LOCALHOST_IP, RANDOM_PORT));
         return server;

diff --git a/protocols/smtp/src/test/java/org/apache/james/protocols/smtp/utils/BaseFakeSMTPSession.java b/protocols/smtp/src/test/java/org/apache/james/protocols/smtp/utils/BaseFakeSMTPSession.java
@@ -43,7 +43,7 @@ public boolean needsCommandInjectionDetection() {
     }
 
     @Override
-    public void startDetectingCommadInjection() {
+    public void startDetectingCommandInjection() {
         throw new UnsupportedOperationException("Unimplemented Stub Method");
     }