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/30 11:17:10 UTC
[james-project] branch 3.7.x updated: JAMES-1862 Fix several issues with STARTTLS command injection detection [BACKPORT]
This is an automated email from the ASF dual-hosted git repository.
btellier pushed a commit to branch 3.7.x
in repository https://gitbox.apache.org/repos/asf/james-project.git
The following commit(s) were added to refs/heads/3.7.x by this push:
new 85cbb3e JAMES-1862 Fix several issues with STARTTLS command injection detection [BACKPORT]
85cbb3e is described below
commit 85cbb3ead59ca530b7415adcd3dfd227aa3e1e58
Author: Benoit Tellier <bt...@linagora.com>
AuthorDate: Wed Mar 30 12:40:48 2022 +0700
JAMES-1862 Fix several issues with STARTTLS command injection detection [BACKPORT]
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 | 5 ++
.../james/imapserver/netty/NettyImapSession.java | 1 +
.../netty/SwitchableLineBasedFrameDecoder.java | 7 +-
.../james/imapserver/netty/IMAPServerTest.java | 77 ++++++++++++++++++++++
.../src/test/resources/imapServerStartTLS.xml | 2 +-
.../netty/ChannelManageSieveResponseWriter.java | 7 +-
.../netty/ManageSieveChannelUpstreamHandler.java | 1 +
10 files changed, 121 insertions(+), 17 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 0feff45..4c81a7b 100644
--- a/server/protocols/protocols-imap4/pom.xml
+++ b/server/protocols/protocols-imap4/pom.xml
@@ -132,6 +132,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 b582066..ef19ef7 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
@@ -147,6 +147,7 @@ public class NettyImapSession implements ImapSession, NettyConstants {
SslHandler filter = new SslHandler(secure.createSSLEngine(), false);
filter.getEngine().setUseClientMode(false);
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 590ef08..fa267a9 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
@@ -20,6 +20,7 @@
package org.apache.james.imapserver.netty;
import static javax.mail.Folder.READ_WRITE;
+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;
@@ -27,8 +28,10 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy;
import java.io.EOFException;
import java.io.FileNotFoundException;
import java.io.IOException;
+import java.net.InetSocketAddress;
import java.nio.charset.StandardCharsets;
import java.util.Properties;
+import java.util.concurrent.ConcurrentLinkedDeque;
import javax.mail.FetchProfile;
import javax.mail.Folder;
@@ -51,6 +54,7 @@ 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.jwt.OidcTokenFixture;
import org.apache.james.mailbox.MailboxSession;
@@ -80,13 +84,33 @@ import org.junit.jupiter.api.extension.RegisterExtension;
import org.mockserver.integration.ClientAndServer;
import org.mockserver.model.HttpRequest;
import org.mockserver.model.HttpResponse;
+import org.slf4j.LoggerFactory;
import com.sun.mail.imap.IMAPFolder;
+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 nl.altindag.ssl.exception.GenericKeyStoreException;
import nl.altindag.ssl.exception.PrivateKeyParseException;
+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");
@@ -344,11 +368,24 @@ class IMAPServerTest {
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
@@ -366,6 +403,24 @@ class IMAPServerTest {
}
@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);
@@ -386,6 +441,28 @@ class IMAPServerTest {
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
diff --git a/server/protocols/protocols-imap4/src/test/resources/imapServerStartTLS.xml b/server/protocols/protocols-imap4/src/test/resources/imapServerStartTLS.xml
index 127dfb9..c2b8eb8 100644
--- a/server/protocols/protocols-imap4/src/test/resources/imapServerStartTLS.xml
+++ b/server/protocols/protocols-imap4/src/test/resources/imapServerStartTLS.xml
@@ -1,7 +1,7 @@
<imapserver enabled="true">
<jmxName>imapserver</jmxName>
<bind>0.0.0.0:0</bind>
- <tls socketTLS="false" startTLS="false">
+ <tls socketTLS="false" startTLS="true">
<keystore>classpath://keystore</keystore>
<secret>james72laBalle</secret>
<provider>org.bouncycastle.jce.provider.BouncyCastleProvider</provider>
diff --git a/server/protocols/protocols-managesieve/src/main/java/org/apache/james/managesieveserver/netty/ChannelManageSieveResponseWriter.java b/server/protocols/protocols-managesieve/src/main/java/org/apache/james/managesieveserver/netty/ChannelManageSieveResponseWriter.java
index d131efb..0cbe023 100644
--- a/server/protocols/protocols-managesieve/src/main/java/org/apache/james/managesieveserver/netty/ChannelManageSieveResponseWriter.java
+++ b/server/protocols/protocols-managesieve/src/main/java/org/apache/james/managesieveserver/netty/ChannelManageSieveResponseWriter.java
@@ -30,6 +30,7 @@ import org.jboss.netty.handler.stream.ChunkedStream;
public class ChannelManageSieveResponseWriter implements CommandDetectionSession {
private final Channel channel;
private String cumulation = null;
+ private boolean needsCommandInjectionDetection = true;
public ChannelManageSieveResponseWriter(Channel channel) {
this.channel = channel;
@@ -44,17 +45,17 @@ public class ChannelManageSieveResponseWriter implements CommandDetectionSession
@Override
public boolean needsCommandInjectionDetection() {
- return false;
+ return needsCommandInjectionDetection;
}
@Override
public void startDetectingCommandInjection() {
-
+ needsCommandInjectionDetection = true;
}
@Override
public void stopDetectingCommandInjection() {
-
+ needsCommandInjectionDetection = false;
}
public void resetCumulation() {
diff --git a/server/protocols/protocols-managesieve/src/main/java/org/apache/james/managesieveserver/netty/ManageSieveChannelUpstreamHandler.java b/server/protocols/protocols-managesieve/src/main/java/org/apache/james/managesieveserver/netty/ManageSieveChannelUpstreamHandler.java
index bdee262..cf0c5fd 100644
--- a/server/protocols/protocols-managesieve/src/main/java/org/apache/james/managesieveserver/netty/ManageSieveChannelUpstreamHandler.java
+++ b/server/protocols/protocols-managesieve/src/main/java/org/apache/james/managesieveserver/netty/ManageSieveChannelUpstreamHandler.java
@@ -79,6 +79,7 @@ public class ManageSieveChannelUpstreamHandler extends SimpleChannelUpstreamHand
turnSSLon(ctx.getChannel());
manageSieveSession.setSslEnabled(true);
manageSieveSession.setState(Session.State.UNAUTHENTICATED);
+ attachment.stopDetectingCommandInjection();
}
} catch (NotEnoughDataException ex) {
// Do nothing will keep the cumulation
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org