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 03:55:41 UTC
[james-project] 02/06: JAMES-1862 STARTTLS command injection checks are vulnerable to concurrency man-in-the-middle attacks
This is an automated email from the ASF dual-hosted git repository.
btellier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit c8afa55e376b582cb119d5aa8ea0efb669bea052
Author: Benoit Tellier <bt...@linagora.com>
AuthorDate: Tue Mar 15 11:05:02 2022 +0700
JAMES-1862 STARTTLS command injection checks are vulnerable to concurrency man-in-the-middle attacks
Attack scenario:
- 1. Clients sends frame F1 activating STARTTLS
- 2. Framer sees a STARTTLS request alone and thinks "that's ok"
- 3. Right after attacker sends a second frame (eg NOOP)
- 4. Framer sees a NOOP request alone and thinks "that's ok"
- 5. Processing of STARTTLS requests starts, SSL is negociated
- 6. Previously accepted NOOP request then gets processed
- 7. The attacker succeeded to execute an injected command!
This attack scenario leverages the fact that concurrency is enforced at the
netty handler level and not at the pipeline level. This can be mitigated by
tracking when a starttls command is received via channel attachments.
The scenario then becames:
- 1. Clients sends frame F1 activating STARTTLS
- 2. Framer sees a STARTTLS request alone and thinks "Hmm careful we are
activating STARTTLS let's turn auto-reads to false so we can't get concurency issues"
- 3. Right after attacker sends a second frame (eg NOOP)
- 4. Framer see the noop but knows it already handles a STARTTLS and thus rejects the
NOOP command.
fixup! JAMES-1862 STARTTLS command injection checks are vulnerable to concurrency man-in-the-middle attacks
---
.../protocols/api/AbstractProtocolTransport.java | 1 +
.../AllButStartTlsLineBasedChannelHandler.java | 28 +++++++++++++++++++---
.../james/imapserver/netty/NettyImapSession.java | 2 +-
.../netty/SwitchableLineBasedFrameDecoder.java | 5 ++++
4 files changed, 32 insertions(+), 4 deletions(-)
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 82b843e..c767264 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;
@@ -36,6 +37,9 @@ import io.netty.util.AttributeKey;
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 static final AttributeKey<Object> ATTRIBUTE_KEY = AttributeKey.valueOf("startTlsInFlight");
private final ChannelPipeline pipeline;
private final String pattern;
@@ -54,9 +58,22 @@ public class AllButStartTlsLineBasedChannelHandler extends LineBasedFrameDecoder
if (session == null || session.needsCommandInjectionDetection()) {
String trimedLowerCasedInput = readAll(buffer).trim().toLowerCase(Locale.US);
- if (hasCommandInjection(trimedLowerCasedInput)) {
+ Boolean startTlsInFlight = Optional.ofNullable(ctx.channel().attr(ATTRIBUTE_KEY))
+ .map(attr -> attr.get())
+ .filter(Boolean.class::isInstance)
+ .map(Boolean.class::cast)
+ .orElse(false);
+ 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.channel().attr(ATTRIBUTE_KEY).set(true);
+ }
}
return super.decode(ctx, buffer);
}
@@ -65,13 +82,18 @@ public class AllButStartTlsLineBasedChannelHandler extends LineBasedFrameDecoder
return pipeline.context(HandlerConstants.CORE_HANDLER).channel().attr(sessionAttributeKey).get();
}
+ protected boolean hasStartTLS(String trimedLowerCasedInput) {
+ List<String> parts = CRLF_SPLITTER.splitToList(trimedLowerCasedInput);
+
+ return parts.stream().anyMatch(s -> s.equalsIgnoreCase(pattern));
+ }
+
private String readAll(ByteBuf buffer) {
return buffer.toString(StandardCharsets.US_ASCII);
}
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/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 2a5cb0d..09d9e4f 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
@@ -163,7 +163,7 @@ public class NettyImapSession implements ImapSession, NettyConstants {
filter.engine().setUseClientMode(false);
channel.pipeline().addFirst(SSL_HANDLER, filter);
-
+ stopDetectingCommandInjection();
channel.config().setAutoRead(true);
return 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 501ae14..bf4f212 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
@@ -62,4 +62,9 @@ public class SwitchableLineBasedFrameDecoder extends AllButStartTlsLineBasedChan
}
return trimmedInput.substring(tagEnd + 1);
}
+
+ @Override
+ protected boolean hasStartTLS(String trimedLowerCasedInput) {
+ return super.hasStartTLS(removeTag(trimedLowerCasedInput));
+ }
}
\ No newline at end of file
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org