You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by bt...@apache.org on 2022/03/31 05:38:56 UTC
[james-project] branch 3.6.x updated: JAMES-1862 Fix STARTTLS command injection detection [BACKPORT 3.6.x] (#944)
This is an automated email from the ASF dual-hosted git repository.
btellier pushed a commit to branch 3.6.x
in repository https://gitbox.apache.org/repos/asf/james-project.git
The following commit(s) were added to refs/heads/3.6.x by this push:
new 891c7b1 JAMES-1862 Fix STARTTLS command injection detection [BACKPORT 3.6.x] (#944)
891c7b1 is described below
commit 891c7b1a2e83b2171238e71171e19e4b5c51eeac
Author: Benoit TELLIER <bt...@linagora.com>
AuthorDate: Thu Mar 31 12:38:49 2022 +0700
JAMES-1862 Fix STARTTLS command injection detection [BACKPORT 3.6.x] (#944)
Original PR: https://github.com/apache/james-project/pull/919
- Parser differential for IMAP STARTTLS
- Concurrency could lead to injected plain commands to be run as SSL encrypted
---
.../apache/james/mpt/session/ExternalSession.java | 10 +-
.../protocols/api/AbstractProtocolTransport.java | 1 +
.../AllButStartTlsLineBasedChannelHandler.java | 27 +++-
server/protocols/protocols-imap4/pom.xml | 10 ++
.../james/imapserver/netty/NettyImapSession.java | 1 +
.../netty/SwitchableLineBasedFrameDecoder.java | 7 +-
.../james/imapserver/netty/IMAPServerTest.java | 139 ++++++++++++++++++++-
.../src/test/resources/imapServerStartTLS.xml | 15 +++
.../protocols-imap4/src/test/resources/keystore | Bin 0 -> 2245 bytes
9 files changed, 196 insertions(+), 14 deletions(-)
diff --git a/mpt/core/src/main/java/org/apache/james/mpt/session/ExternalSession.java b/mpt/core/src/main/java/org/apache/james/mpt/session/ExternalSession.java
index 38c83d5..91f8144 100644
--- a/mpt/core/src/main/java/org/apache/james/mpt/session/ExternalSession.java
+++ b/mpt/core/src/main/java/org/apache/james/mpt/session/ExternalSession.java
@@ -159,15 +159,7 @@ public final class ExternalSession implements Session {
@Override
public void writeLine(String line) throws Exception {
monitor.note("-> " + line);
- monitor.debug("[Writing line]");
- ByteBuffer writeBuffer = US_ASCII.encode(line);
- while (writeBuffer.hasRemaining()) {
- socket.write(writeBuffer);
- }
- lineEndBuffer.rewind();
- while (lineEndBuffer.hasRemaining()) {
- socket.write(lineEndBuffer);
- }
+ socket.write(ByteBuffer.wrap((line + "\r\n").getBytes(US_ASCII)));
monitor.debug("[Done]");
}
diff --git a/protocols/api/src/main/java/org/apache/james/protocols/api/AbstractProtocolTransport.java b/protocols/api/src/main/java/org/apache/james/protocols/api/AbstractProtocolTransport.java
index 3aab58b..661de6f 100644
--- a/protocols/api/src/main/java/org/apache/james/protocols/api/AbstractProtocolTransport.java
+++ b/protocols/api/src/main/java/org/apache/james/protocols/api/AbstractProtocolTransport.java
@@ -53,6 +53,7 @@ public abstract class AbstractProtocolTransport implements ProtocolTransport {
// reset state on starttls
if (startTLS) {
session.resetState();
+ session.stopDetectingCommandInjection();
}
if (response.isEndSession()) {
diff --git a/protocols/netty/src/main/java/org/apache/james/protocols/netty/AllButStartTlsLineBasedChannelHandler.java b/protocols/netty/src/main/java/org/apache/james/protocols/netty/AllButStartTlsLineBasedChannelHandler.java
index fa3f127..8034e26 100644
--- a/protocols/netty/src/main/java/org/apache/james/protocols/netty/AllButStartTlsLineBasedChannelHandler.java
+++ b/protocols/netty/src/main/java/org/apache/james/protocols/netty/AllButStartTlsLineBasedChannelHandler.java
@@ -21,6 +21,7 @@ package org.apache.james.protocols.netty;
import java.nio.charset.StandardCharsets;
import java.util.List;
import java.util.Locale;
+import java.util.Optional;
import org.apache.james.protocols.api.CommandDetectionSession;
import org.jboss.netty.buffer.ChannelBuffer;
@@ -34,6 +35,9 @@ import com.google.common.base.Splitter;
public class AllButStartTlsLineBasedChannelHandler extends LineBasedFrameDecoder {
private static final Boolean FAIL_FAST = true;
+ private static final CharMatcher CRLF_MATCHER = CharMatcher.anyOf("\r\n");
+ private static final Splitter CRLF_SPLITTER = Splitter.on(CRLF_MATCHER).omitEmptyStrings();
+
private final ChannelPipeline pipeline;
private final String pattern;
@@ -48,14 +52,32 @@ public class AllButStartTlsLineBasedChannelHandler extends LineBasedFrameDecoder
CommandDetectionSession session = retrieveSession(ctx, channel);
if (session == null || session.needsCommandInjectionDetection()) {
+ boolean startTlsInFlight = Optional.ofNullable(ctx.getChannel().getAttachment())
+ .filter(Boolean.class::isInstance)
+ .map(Boolean.class::cast)
+ .orElse(false);
String trimedLowerCasedInput = readAll(buffer).trim().toLowerCase(Locale.US);
- if (hasCommandInjection(trimedLowerCasedInput)) {
+ if (hasCommandInjection(trimedLowerCasedInput) || startTlsInFlight) {
throw new CommandInjectionDetectedException();
}
+ // Prevents further reads on this channel to avoid race conditions
+ // Framer can accept IMAP requests sent concurrently while the channel is
+ // not yet encrypted allowing man-in-the-middle attacks relying on race conditions
+ // Disabling auto-read when framing STARTTLS prevents accepting other frames until STARTTLS
+ // is fully active.
+ if (hasStartTLS(trimedLowerCasedInput)) {
+ ctx.getChannel().setAttachment(true);
+ }
}
return super.decode(ctx, channel, buffer);
}
+ protected boolean hasStartTLS(String trimedLowerCasedInput) {
+ List<String> parts = CRLF_SPLITTER.splitToList(trimedLowerCasedInput);
+
+ return parts.stream().anyMatch(s -> s.equalsIgnoreCase(pattern));
+ }
+
protected CommandDetectionSession retrieveSession(ChannelHandlerContext ctx, Channel channel) {
return (CommandDetectionSession) pipeline.getContext(HandlerConstants.CORE_HANDLER).getAttachment();
}
@@ -65,8 +87,7 @@ public class AllButStartTlsLineBasedChannelHandler extends LineBasedFrameDecoder
}
private boolean hasCommandInjection(String trimedLowerCasedInput) {
- List<String> parts = Splitter.on(CharMatcher.anyOf("\r\n")).omitEmptyStrings()
- .splitToList(trimedLowerCasedInput);
+ List<String> parts = CRLF_SPLITTER.splitToList(trimedLowerCasedInput);
return hasInvalidStartTlsPart(parts) || multiPartsAndOneStartTls(parts);
}
diff --git a/server/protocols/protocols-imap4/pom.xml b/server/protocols/protocols-imap4/pom.xml
index 2a826d4..ce5f2cc 100644
--- a/server/protocols/protocols-imap4/pom.xml
+++ b/server/protocols/protocols-imap4/pom.xml
@@ -58,6 +58,11 @@
</dependency>
<dependency>
<groupId>${james.groupId}</groupId>
+ <artifactId>james-server-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
<artifactId>james-server-filesystem-api</artifactId>
</dependency>
<dependency>
@@ -110,6 +115,11 @@
<artifactId>netty</artifactId>
</dependency>
<dependency>
+ <groupId>io.projectreactor.netty</groupId>
+ <artifactId>reactor-netty</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
<groupId>javax.inject</groupId>
<artifactId>javax.inject</artifactId>
</dependency>
diff --git a/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/NettyImapSession.java b/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/NettyImapSession.java
index 30718cd..ba8f3e0 100644
--- a/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/NettyImapSession.java
+++ b/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/NettyImapSession.java
@@ -144,6 +144,7 @@ public class NettyImapSession implements ImapSession, NettyConstants {
filter.getEngine().setEnabledCipherSuites(enabledCipherSuites);
}
channel.getPipeline().addFirst(SSL_HANDLER, filter);
+ stopDetectingCommandInjection();
channel.setReadable(true);
diff --git a/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/SwitchableLineBasedFrameDecoder.java b/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/SwitchableLineBasedFrameDecoder.java
index fb5d58e..e93ea7d 100644
--- a/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/SwitchableLineBasedFrameDecoder.java
+++ b/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/SwitchableLineBasedFrameDecoder.java
@@ -82,9 +82,14 @@ public class SwitchableLineBasedFrameDecoder extends AllButStartTlsLineBasedChan
.anyMatch(line -> line.startsWith(PATTERN) && !line.endsWith(PATTERN));
}
+ @Override
+ protected boolean hasStartTLS(String trimedLowerCasedInput) {
+ return super.hasStartTLS(removeTag(trimedLowerCasedInput));
+ }
+
protected String removeTag(String input) {
String trimmedInput = input.trim();
- int tagEnd = input.indexOf(' ');
+ int tagEnd = trimmedInput.indexOf(' ');
if (tagEnd < 0) {
return input;
}
diff --git a/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerTest.java b/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerTest.java
index e434751..662ac10 100644
--- a/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerTest.java
+++ b/server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerTest.java
@@ -19,26 +19,36 @@
package org.apache.james.imapserver.netty;
+import static org.apache.james.jmap.JMAPTestingConstants.LOCALHOST_IP;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import java.io.EOFException;
import java.io.InputStream;
+import java.net.InetSocketAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.concurrent.ConcurrentLinkedDeque;
import org.apache.commons.configuration2.XMLConfiguration;
import org.apache.commons.configuration2.builder.FileBasedConfigurationBuilder;
import org.apache.commons.configuration2.builder.fluent.Parameters;
import org.apache.commons.configuration2.convert.DisabledListDelimiterHandler;
import org.apache.commons.configuration2.io.FileHandler;
+import org.apache.commons.net.imap.IMAPReply;
+import org.apache.commons.net.imap.IMAPSClient;
import org.apache.james.core.Username;
import org.apache.james.imap.encode.main.DefaultImapEncoderFactory;
import org.apache.james.imap.main.DefaultImapDecoderFactory;
+import org.apache.james.imap.processor.base.AbstractChainedProcessor;
import org.apache.james.imap.processor.main.DefaultImapProcessorFactory;
import org.apache.james.mailbox.inmemory.manager.InMemoryIntegrationResources;
import org.apache.james.mailbox.store.FakeAuthenticator;
import org.apache.james.mailbox.store.FakeAuthorizator;
import org.apache.james.mailbox.store.StoreSubscriptionManager;
import org.apache.james.metrics.tests.RecordingMetricFactory;
+import org.apache.james.server.core.configuration.Configuration;
+import org.apache.james.server.core.filesystem.FileSystemImpl;
import org.apache.james.util.ClassLoaderUtils;
import org.apache.james.utils.TestIMAPClient;
import org.junit.jupiter.api.AfterEach;
@@ -47,8 +57,29 @@ import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.RepeatedTest;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
+import org.slf4j.LoggerFactory;
+
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
+import io.netty.buffer.Unpooled;
+import io.netty.channel.ChannelOption;
+import reactor.core.publisher.Mono;
+import reactor.core.scheduler.Schedulers;
+import reactor.netty.Connection;
+import reactor.netty.tcp.TcpClient;
class IMAPServerTest {
+ public static ListAppender<ILoggingEvent> getListAppenderForClass(Class clazz) {
+ Logger logger = (Logger) LoggerFactory.getLogger(clazz);
+
+ ListAppender<ILoggingEvent> loggingEventListAppender = new ListAppender<>();
+ loggingEventListAppender.start();
+
+ logger.addAppender(loggingEventListAppender);
+ return loggingEventListAppender;
+ }
+
private static final String _129K_MESSAGE = "header: value\r\n" + "012345678\r\n".repeat(13107);
private static final String _65K_MESSAGE = "header: value\r\n" + "012345678\r\n".repeat(6553);
private static final Username USER = Username.of("user@domain.org");
@@ -98,7 +129,12 @@ class IMAPServerTest {
resources.getQuotaRootResolver(),
metricFactory),
new ImapMetrics(metricFactory));
-
+ Configuration configuration = Configuration.builder()
+ .workingDirectory("../")
+ .configurationFromClasspath()
+ .build();
+ FileSystemImpl fileSystem = new FileSystemImpl(configuration.directories());
+ imapServer.setFileSystem(fileSystem);
imapServer.configure(getConfig(ClassLoaderUtils.getSystemResourceAsSharedStream(configurationFile)));
imapServer.init();
@@ -162,6 +198,107 @@ class IMAPServerTest {
}
@Nested
+ class StartTLS {
+ IMAPServer imapServer;
+ private int port;
+ private Connection connection;
+ private ConcurrentLinkedDeque<String> responses;
+
+ @BeforeEach
+ void beforeEach() throws Exception {
+ imapServer = createImapServer("imapServerStartTLS.xml");
+ port = imapServer.getListenAddresses().get(0).getPort();
+ connection = TcpClient.create()
+ .noSSL()
+ .remoteAddress(() -> new InetSocketAddress(LOCALHOST_IP, port))
+ .option(ChannelOption.TCP_NODELAY, true)
+ .connectNow();
+ responses = new ConcurrentLinkedDeque<>();
+ connection.inbound().receive().asString()
+ .doOnNext(s -> System.out.println("A: " + s))
+ .doOnNext(responses::addLast)
+ .subscribeOn(Schedulers.elastic())
+ .subscribe();
+ }
+
+ @AfterEach
+ void tearDown() {
+ imapServer.destroy();
+ }
+
+ @Test
+ void extraLinesBatchedWithStartTLSShouldBeSanitized() throws Exception {
+ IMAPSClient imapClient = new IMAPSClient();
+ imapClient.connect("127.0.0.1", port);
+ assertThatThrownBy(() -> imapClient.sendCommand("STARTTLS\r\nA1 NOOP\r\n"))
+ .isInstanceOf(EOFException.class)
+ .hasMessage("Connection closed without indication.");
+ }
+
+ @Test
+ void extraLFLinesBatchedWithStartTLSShouldBeSanitized() throws Exception {
+ IMAPSClient imapClient = new IMAPSClient();
+ imapClient.connect("127.0.0.1", port);
+ assertThatThrownBy(() -> imapClient.sendCommand("STARTTLS\nA1 NOOP\r\n"))
+ .isInstanceOf(EOFException.class)
+ .hasMessage("Connection closed without indication.");
+ }
+
+ @Test
+ void tagsShouldBeWellSanitized() throws Exception {
+ IMAPSClient imapClient = new IMAPSClient();
+ imapClient.connect("127.0.0.1", port);
+ assertThatThrownBy(() -> imapClient.sendCommand("NOOP\r\n A1 STARTTLS\r\nA2 NOOP"))
+ .isInstanceOf(EOFException.class)
+ .hasMessage("Connection closed without indication.");
+ }
+
+ @Test
+ void lineFollowingStartTLSShouldBeSanitized() throws Exception {
+ IMAPSClient imapClient = new IMAPSClient();
+ imapClient.connect("127.0.0.1", port);
+ assertThatThrownBy(() -> imapClient.sendCommand("STARTTLS A1 NOOP\r\n"))
+ .isInstanceOf(EOFException.class)
+ .hasMessage("Connection closed without indication.");
+ }
+
+ @Test
+ void startTLSShouldFailWhenAuthenticated() throws Exception {
+ // Avoids session fixation attacks as described in https://www.usenix.org/system/files/sec21-poddebniak.pdf
+ // section 6.2
+
+ IMAPSClient imapClient = new IMAPSClient();
+ imapClient.connect("127.0.0.1", port);
+ imapClient.login(USER.asString(), USER_PASS);
+ int imapCode = imapClient.sendCommand("STARTTLS\r\n");
+
+ assertThat(imapCode).isEqualTo(IMAPReply.NO);
+ }
+
+ private void send(String format) {
+ connection.outbound()
+ .send(Mono.just(Unpooled.wrappedBuffer(format
+ .getBytes(StandardCharsets.UTF_8))))
+ .then()
+ .subscribe();
+ }
+
+ @RepeatedTest(10)
+ void concurrencyShouldNotLeadToCommandInjection() throws Exception {
+ ListAppender<ILoggingEvent> listAppender = getListAppenderForClass(AbstractChainedProcessor.class);
+
+ send("a0 STARTTLS\r\n");
+ send("a1 NOOP\r\n");
+
+ Thread.sleep(50);
+
+ assertThat(listAppender.list)
+ .filteredOn(event -> event.getFormattedMessage().contains("Processing org.apache.james.imap.message.request.NoopRequest"))
+ .isEmpty();
+ }
+ }
+
+ @Nested
class Limit {
IMAPServer imapServer;
private int port;
diff --git a/server/protocols/protocols-imap4/src/test/resources/imapServerStartTLS.xml b/server/protocols/protocols-imap4/src/test/resources/imapServerStartTLS.xml
new file mode 100644
index 0000000..5cedad0
--- /dev/null
+++ b/server/protocols/protocols-imap4/src/test/resources/imapServerStartTLS.xml
@@ -0,0 +1,15 @@
+<imapserver enabled="true">
+ <jmxName>imapserver</jmxName>
+ <bind>0.0.0.0:0</bind>
+ <tls socketTLS="false" startTLS="true">
+ <keystore>classpath://keystore</keystore>
+ <secret>james72laBalle</secret>
+ </tls>
+ <connectionBacklog>200</connectionBacklog>
+ <connectionLimit>0</connectionLimit>
+ <connectionLimitPerIP>0</connectionLimitPerIP>
+ <idleTimeInterval>120</idleTimeInterval>
+ <idleTimeIntervalUnit>SECONDS</idleTimeIntervalUnit>
+ <enableIdle>true</enableIdle>
+ <plainAuthDisallowed>false</plainAuthDisallowed>
+</imapserver>
\ No newline at end of file
diff --git a/server/protocols/protocols-imap4/src/test/resources/keystore b/server/protocols/protocols-imap4/src/test/resources/keystore
new file mode 100644
index 0000000..536a6c7
Binary files /dev/null and b/server/protocols/protocols-imap4/src/test/resources/keystore differ
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org