From 6be722ed9de6fc79620a432e43326262721cad53 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Mon, 21 Jan 2019 19:55:33 -0800 Subject: [PATCH] GUACAMOLE-524: Require usages of SimpleConnection to explicitly request automatic interpretation of parameter tokens. Do not enable by default. Previous implementations of SimpleConnection did not interpret parameter tokens automatically. Adding that behavior now could have security implications for downstream users of the class if parameter values may unexpectedly contain substrings which would be interpreted as tokens, particularly if parameter values are built from untrusted input. --- .../quickconnect/QuickConnectDirectory.java | 2 +- .../simple/SimpleAuthenticationProvider.java | 2 +- .../net/auth/simple/SimpleConnection.java | 80 +++++++++++++++++-- .../net/auth/simple/SimpleUserContext.java | 34 +++++++- 4 files changed, 107 insertions(+), 11 deletions(-) diff --git a/extensions/guacamole-auth-quickconnect/src/main/java/org/apache/guacamole/auth/quickconnect/QuickConnectDirectory.java b/extensions/guacamole-auth-quickconnect/src/main/java/org/apache/guacamole/auth/quickconnect/QuickConnectDirectory.java index 37b07ba33..cec0432b2 100644 --- a/extensions/guacamole-auth-quickconnect/src/main/java/org/apache/guacamole/auth/quickconnect/QuickConnectDirectory.java +++ b/extensions/guacamole-auth-quickconnect/src/main/java/org/apache/guacamole/auth/quickconnect/QuickConnectDirectory.java @@ -107,7 +107,7 @@ public class QuickConnectDirectory extends SimpleDirectory { String name = QCParser.getName(config); // Create a new connection and set the parent identifier. - Connection connection = new SimpleConnection(name, newConnectionId, config); + Connection connection = new SimpleConnection(name, newConnectionId, config, true); connection.setParentIdentifier(QuickConnectUserContext.ROOT_IDENTIFIER); // Place the object in this directory. diff --git a/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleAuthenticationProvider.java b/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleAuthenticationProvider.java index 6d08d99ac..202181ab9 100644 --- a/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleAuthenticationProvider.java +++ b/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleAuthenticationProvider.java @@ -167,7 +167,7 @@ public abstract class SimpleAuthenticationProvider return null; // Return user context restricted to authorized configs - return new SimpleUserContext(this, authenticatedUser.getIdentifier(), configs); + return new SimpleUserContext(this, authenticatedUser.getIdentifier(), configs, true); } diff --git a/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleConnection.java b/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleConnection.java index 3713b6b77..cb9dcde55 100644 --- a/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleConnection.java +++ b/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleConnection.java @@ -54,6 +54,13 @@ public class SimpleConnection extends AbstractConnection { */ private GuacamoleConfiguration fullConfig; + /** + * Whether parameter tokens in the underlying GuacamoleConfiguration should + * be automatically applied upon connecting. If false, parameter tokens + * will not be interpreted at all. + */ + private final boolean interpretTokens; + /** * The tokens which should apply strictly to the next call to * {@link #connect(org.apache.guacamole.protocol.GuacamoleClientInformation)}. @@ -75,28 +82,81 @@ public class SimpleConnection extends AbstractConnection { /** * Creates a completely uninitialized SimpleConnection. The name, * identifier, and configuration of this SimpleConnection must eventually - * be set before the SimpleConnection may be used. + * be set before the SimpleConnection may be used. Parameter tokens within + * the GuacamoleConfiguration eventually supplied with + * {@link #setConfiguration(org.apache.guacamole.protocol.GuacamoleConfiguration)} + * will not be interpreted. */ public SimpleConnection() { + this(false); + } + + /** + * Creates a completely uninitialized SimpleConnection. The name, + * identifier, and configuration of this SimpleConnection must eventually + * be set before the SimpleConnection may be used. Parameter tokens within + * the GuacamoleConfiguration eventually supplied with + * {@link #setConfiguration(org.apache.guacamole.protocol.GuacamoleConfiguration)} + * will not be interpreted unless explicitly requested with + * {@link #setInterpretTokens(boolean)}. + * + * @param interpretTokens + * Whether parameter tokens in the underlying GuacamoleConfiguration + * should be automatically applied upon connecting. If false, parameter + * tokens will not be interpreted at all. + */ + public SimpleConnection(boolean interpretTokens) { + this.interpretTokens = interpretTokens; } /** * Creates a new SimpleConnection having the given identifier and - * GuacamoleConfiguration. + * GuacamoleConfiguration. Parameter tokens within the + * GuacamoleConfiguration will not be interpreted unless explicitly + * requested with {@link #setInterpretTokens(boolean)}. * - * @param name The name to associate with this connection. - * @param identifier The identifier to associate with this connection. - * @param config The configuration describing how to connect to this - * connection. + * @param name + * The name to associate with this connection. + * + * @param identifier + * The identifier to associate with this connection. + * + * @param config + * The configuration describing how to connect to this connection. */ public SimpleConnection(String name, String identifier, GuacamoleConfiguration config) { + this(name, identifier, config, false); + } + + /** + * Creates a new SimpleConnection having the given identifier and + * GuacamoleConfiguration. Parameter tokens will be interpreted if + * explicitly requested. + * + * @param name + * The name to associate with this connection. + * + * @param identifier + * The identifier to associate with this connection. + * + * @param config + * The configuration describing how to connect to this connection. + * + * @param interpretTokens + * Whether parameter tokens in the underlying GuacamoleConfiguration + * should be automatically applied upon connecting. If false, parameter + * tokens will not be interpreted at all. + */ + public SimpleConnection(String name, String identifier, + GuacamoleConfiguration config, boolean interpretTokens) { super.setName(name); super.setIdentifier(identifier); super.setConfiguration(config); this.fullConfig = config; + this.interpretTokens = interpretTokens; } @@ -191,8 +251,14 @@ public class SimpleConnection extends AbstractConnection { // Make received tokens available within the legacy connect() strictly // in context of the current connect() call try { - currentTokens.set(tokens); + + // Automatically filter configurations only if explicitly + // configured to do so + if (interpretTokens) + currentTokens.set(tokens); + return connect(info); + } finally { currentTokens.remove(); diff --git a/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleUserContext.java b/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleUserContext.java index 03e94fbac..a1dce55af 100644 --- a/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleUserContext.java +++ b/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleUserContext.java @@ -59,7 +59,8 @@ public class SimpleUserContext extends AbstractUserContext { * Creates a new SimpleUserContext which provides access to only those * configurations within the given Map. The username is set to the * ANONYMOUS_IDENTIFIER defined by AuthenticatedUser, effectively declaring - * the current user as anonymous. + * the current user as anonymous. Parameter tokens within the given + * GuacamoleConfigurations will not be interpreted. * * @param authProvider * The AuthenticationProvider creating this UserContext. @@ -76,6 +77,8 @@ public class SimpleUserContext extends AbstractUserContext { /** * Creates a new SimpleUserContext for the user with the given username * which provides access to only those configurations within the given Map. + * Parameter tokens within the given GuacamoleConfigurations will not be + * interpreted. * * @param authProvider * The AuthenticationProvider creating this UserContext. @@ -89,6 +92,33 @@ public class SimpleUserContext extends AbstractUserContext { */ public SimpleUserContext(AuthenticationProvider authProvider, String username, Map configs) { + this(authProvider, username, configs, false); + } + + /** + * Creates a new SimpleUserContext for the user with the given username + * which provides access to only those configurations within the given Map. + * Parameter tokens within the given GuacamoleConfigurations will be + * interpreted if explicitly requested. + * + * @param authProvider + * The AuthenticationProvider creating this UserContext. + * + * @param username + * The username of the user associated with this UserContext. + * + * @param configs + * A Map of all configurations for which the user associated with + * this UserContext has read access. + * + * @param interpretTokens + * Whether parameter tokens in the underlying GuacamoleConfigurations + * should be automatically applied upon connecting. If false, parameter + * tokens will not be interpreted at all. + */ + public SimpleUserContext(AuthenticationProvider authProvider, + String username, Map configs, + boolean interpretTokens) { // Produce map of connections from given configs Map connections = new ConcurrentHashMap(configs.size()); @@ -99,7 +129,7 @@ public class SimpleUserContext extends AbstractUserContext { GuacamoleConfiguration config = configEntry.getValue(); // Add as simple connection - Connection connection = new SimpleConnection(identifier, identifier, config); + Connection connection = new SimpleConnection(identifier, identifier, config, true); connection.setParentIdentifier(DEFAULT_ROOT_CONNECTION_GROUP); connections.put(identifier, connection);