QPID-7558: [Java Broker] Restrict the tableNamePrefix values to match validValuePattern in order to prevent SQL injection
The patch was supplied by Lorenz Quack <lquack@apache.org>
diff --git a/broker-core/src/main/java/org/apache/qpid/server/store/AbstractJDBCMessageStore.java b/broker-core/src/main/java/org/apache/qpid/server/store/AbstractJDBCMessageStore.java
index 7dc9004..6aa0367 100644
--- a/broker-core/src/main/java/org/apache/qpid/server/store/AbstractJDBCMessageStore.java
+++ b/broker-core/src/main/java/org/apache/qpid/server/store/AbstractJDBCMessageStore.java
@@ -733,7 +733,7 @@
{
}
- public void setTablePrefix(final String tablePrefix)
+ protected void setTablePrefix(final String tablePrefix)
{
_tablePrefix = tablePrefix;
}
diff --git a/broker-plugins/jdbc-store/src/main/java/org/apache/qpid/server/store/jdbc/JDBCSystemConfig.java b/broker-plugins/jdbc-store/src/main/java/org/apache/qpid/server/store/jdbc/JDBCSystemConfig.java
index acdc71c..722cc77 100644
--- a/broker-plugins/jdbc-store/src/main/java/org/apache/qpid/server/store/jdbc/JDBCSystemConfig.java
+++ b/broker-plugins/jdbc-store/src/main/java/org/apache/qpid/server/store/jdbc/JDBCSystemConfig.java
@@ -46,7 +46,10 @@
@ManagedContextDefault(name = "systemConfig.tableNamePrefix")
String DEFAULT_SYSTEM_CONFIG_TABLE_NAME_PREFIX = "";
- @ManagedAttribute(secure=true, defaultValue = "${systemConfig.tableNamePrefix}")
+ @ManagedAttribute(secure = true,
+ description = "Optional database table prefix so multiple SystemConfigs can share the same database",
+ defaultValue = "${systemConfig.tableNamePrefix}",
+ validValuePattern = "[a-zA-Z_0-9]*")
String getTableNamePrefix();
}
diff --git a/broker-plugins/jdbc-store/src/main/java/org/apache/qpid/server/virtualhost/jdbc/JDBCVirtualHost.java b/broker-plugins/jdbc-store/src/main/java/org/apache/qpid/server/virtualhost/jdbc/JDBCVirtualHost.java
index 095b3fa..be4ffe8 100644
--- a/broker-plugins/jdbc-store/src/main/java/org/apache/qpid/server/virtualhost/jdbc/JDBCVirtualHost.java
+++ b/broker-plugins/jdbc-store/src/main/java/org/apache/qpid/server/virtualhost/jdbc/JDBCVirtualHost.java
@@ -44,7 +44,10 @@
@ManagedContextDefault(name = "jdbcvirtualhost.tableNamePrefix")
String DEFAULT_JDBC_VIRTUALHOST_TABLE_NAME_PREFIX = "";
- @ManagedAttribute(secure=true, defaultValue = "${jdbcvirtualhost.tableNamePrefix}")
+ @ManagedAttribute(secure = true,
+ description = "Optional database table prefix so multiple VirtualHosts can share the same database",
+ defaultValue = "${jdbcvirtualhost.tableNamePrefix}",
+ validValuePattern = "[a-zA-Z_0-9]*")
String getTableNamePrefix();
}
diff --git a/broker-plugins/jdbc-store/src/main/java/org/apache/qpid/server/virtualhostnode/jdbc/JDBCVirtualHostNode.java b/broker-plugins/jdbc-store/src/main/java/org/apache/qpid/server/virtualhostnode/jdbc/JDBCVirtualHostNode.java
index a57456f..30b349c 100644
--- a/broker-plugins/jdbc-store/src/main/java/org/apache/qpid/server/virtualhostnode/jdbc/JDBCVirtualHostNode.java
+++ b/broker-plugins/jdbc-store/src/main/java/org/apache/qpid/server/virtualhostnode/jdbc/JDBCVirtualHostNode.java
@@ -51,6 +51,9 @@
@ManagedContextDefault(name = "jdbcvirtualhostnode.tableNamePrefix")
String DEFAULT_JDBC_VIRTUALHOSTNODE_TABLE_NAME_PREFIX = "";
- @ManagedAttribute(secure=true, defaultValue = "${jdbcvirtualhostnode.tableNamePrefix}")
+ @ManagedAttribute(secure = true,
+ description = "Optional database table prefix so multiple VirtualHostNodes can share the same database",
+ defaultValue = "${jdbcvirtualhostnode.tableNamePrefix}",
+ validValuePattern = "[a-zA-Z_0-9]*")
String getTableNamePrefix();
}
diff --git a/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/store/jdbc/JDBCMessageStoreTest.java b/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/store/jdbc/JDBCMessageStoreTest.java
index ac16212..a3f9271 100644
--- a/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/store/jdbc/JDBCMessageStoreTest.java
+++ b/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/store/jdbc/JDBCMessageStoreTest.java
@@ -39,6 +39,7 @@
public class JDBCMessageStoreTest extends MessageStoreTestCase
{
+ public static final String TEST_TABLE_PREFIX = "TEST_TABLE_PREFIX_";
private String _connectionURL;
@Override
@@ -57,6 +58,16 @@
}
}
+ public void testTablePrefix() throws Exception
+ {
+ Collection<String> expectedTables = ((GenericJDBCMessageStore)getStore()).getTableNames();
+ for (String expectedTable : expectedTables)
+ {
+ assertTrue(String.format("Table '%s' does not start with expected prefix '%s'", expectedTable, TEST_TABLE_PREFIX), expectedTable.startsWith(TEST_TABLE_PREFIX));
+ }
+ assertTablesExist(expectedTables, true);
+ }
+
public void testOnDelete() throws Exception
{
Collection<String> expectedTables = ((GenericJDBCMessageStore)getStore()).getTableNames();
@@ -76,7 +87,7 @@
when(jdbcVirtualHost.getConnectionUrl()).thenReturn(_connectionURL);
when(jdbcVirtualHost.getUsername()).thenReturn("test");
when(jdbcVirtualHost.getPassword()).thenReturn("pass");
- when(jdbcVirtualHost.getTableNamePrefix()).thenReturn("");
+ when(jdbcVirtualHost.getTableNamePrefix()).thenReturn(TEST_TABLE_PREFIX);
return jdbcVirtualHost;
}
diff --git a/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/store/jdbc/JDBCSystemConfigTest.java b/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/store/jdbc/JDBCSystemConfigTest.java
new file mode 100644
index 0000000..a27b45c
--- /dev/null
+++ b/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/store/jdbc/JDBCSystemConfigTest.java
@@ -0,0 +1,73 @@
+/*
+ * 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.qpid.server.store.jdbc;
+
+import static org.mockito.Mockito.mock;
+
+import java.security.Principal;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.configuration.updater.CurrentThreadTaskExecutor;
+import org.apache.qpid.server.configuration.updater.TaskExecutor;
+import org.apache.qpid.server.logging.EventLogger;
+import org.apache.qpid.test.utils.QpidTestCase;
+
+public class JDBCSystemConfigTest extends QpidTestCase
+{
+ public void testInvalidTableNamePrefix() throws Exception
+ {
+ final TaskExecutor taskExecutor = new CurrentThreadTaskExecutor();
+ final EventLogger eventLogger = mock(EventLogger.class);
+ final Principal systemPrincipal = mock(Principal.class);
+ JDBCSystemConfig<?> jdbcSystemConfig = new JDBCSystemConfigImpl(taskExecutor, eventLogger, systemPrincipal,
+ Collections.<String, Object>emptyMap());
+
+ // This list is not exhaustive
+ List<String> knownInvalidPrefixes = Arrays.asList("with\"dblquote",
+ "with'quote",
+ "with-dash",
+ "with;semicolon",
+ "with space",
+ "with%percent",
+ "with|pipe",
+ "with(paren",
+ "with)paren",
+ "with[bracket",
+ "with]bracket",
+ "with{brace",
+ "with}brace");
+ for (String invalidPrefix : knownInvalidPrefixes)
+ {
+ try
+ {
+ jdbcSystemConfig.setAttributes(Collections.<String, Object>singletonMap("tableNamePrefix",
+ invalidPrefix));
+ fail(String.format("Should not be able to set prefix to '%s'", invalidPrefix));
+ }
+ catch (IllegalConfigurationException e)
+ {
+ // pass
+ }
+ }
+ }
+}
diff --git a/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/virtualhost/jdbc/JDBCVirtualHostTest.java b/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/virtualhost/jdbc/JDBCVirtualHostTest.java
new file mode 100644
index 0000000..3af9000
--- /dev/null
+++ b/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/virtualhost/jdbc/JDBCVirtualHostTest.java
@@ -0,0 +1,111 @@
+/*
+ * 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.qpid.server.virtualhost.jdbc;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.configuration.updater.CurrentThreadTaskExecutor;
+import org.apache.qpid.server.logging.EventLogger;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.ConfiguredObjectFactoryImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.model.VirtualHostNode;
+import org.apache.qpid.test.utils.QpidTestCase;
+
+public class JDBCVirtualHostTest extends QpidTestCase
+{
+ private CurrentThreadTaskExecutor _taskExecutor;
+
+ @Override
+ public void setUp() throws Exception
+ {
+ super.setUp();
+ _taskExecutor = new CurrentThreadTaskExecutor();
+ _taskExecutor.start();
+ }
+
+ @Override
+ public void tearDown() throws Exception
+ {
+ super.tearDown();
+ _taskExecutor.stopImmediately();
+ }
+
+ public void testInvalidTableNamePrefix() throws Exception
+ {
+ final VirtualHostNode vhn = mock(VirtualHostNode.class);
+ when(vhn.getCategoryClass()).thenReturn(VirtualHostNode.class);
+ when(vhn.getChildExecutor()).thenReturn(_taskExecutor);
+ final ConfiguredObjectFactoryImpl factory = new ConfiguredObjectFactoryImpl(BrokerModel.getInstance());
+ when(vhn.getObjectFactory()).thenReturn(factory);
+ when(vhn.getModel()).thenReturn(factory.getModel());
+
+ EventLogger eventLogger = mock(EventLogger.class);
+ SystemConfig systemConfig = mock(SystemConfig.class);
+ when(systemConfig.getEventLogger()).thenReturn(eventLogger);
+ Broker broker = mock(Broker.class);
+ when(broker.getParent()).thenReturn(systemConfig);
+ when(vhn.getParent()).thenReturn(broker);
+
+ Map<String, Object> attributes = new HashMap<>();
+ attributes.put(ConfiguredObject.NAME, getTestName());
+ attributes.put(ConfiguredObject.TYPE, JDBCVirtualHostImpl.VIRTUAL_HOST_TYPE);
+ attributes.put("connectionUrl", "jdbc://example.com");
+ JDBCVirtualHost<?> jdbcVirtualHost = new JDBCVirtualHostImpl(attributes, vhn);
+
+ // This list is not exhaustive
+ List<String> knownInvalidPrefixes = Arrays.asList("with\"dblquote",
+ "with'quote",
+ "with-dash",
+ "with;semicolon",
+ "with space",
+ "with%percent",
+ "with|pipe",
+ "with(paren",
+ "with)paren",
+ "with[bracket",
+ "with]bracket",
+ "with{brace",
+ "with}brace");
+ for (String invalidPrefix : knownInvalidPrefixes)
+ {
+ try
+ {
+ jdbcVirtualHost.setAttributes(Collections.<String, Object>singletonMap("tableNamePrefix",
+ invalidPrefix));
+ fail(String.format("Should not be able to set prefix to '%s'", invalidPrefix));
+ }
+ catch (IllegalConfigurationException e)
+ {
+ // pass
+ }
+ }
+ }
+}
diff --git a/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/virtualhostnode/jdbc/JDBCVirtualHostNodeTest.java b/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/virtualhostnode/jdbc/JDBCVirtualHostNodeTest.java
new file mode 100644
index 0000000..6160dee
--- /dev/null
+++ b/broker-plugins/jdbc-store/src/test/java/org/apache/qpid/server/virtualhostnode/jdbc/JDBCVirtualHostNodeTest.java
@@ -0,0 +1,103 @@
+/*
+ * 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.qpid.server.virtualhostnode.jdbc;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.configuration.updater.CurrentThreadTaskExecutor;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.ConfiguredObjectFactoryImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.test.utils.QpidTestCase;
+
+public class JDBCVirtualHostNodeTest extends QpidTestCase
+{
+ private CurrentThreadTaskExecutor _taskExecutor;
+
+ @Override
+ public void setUp() throws Exception
+ {
+ super.setUp();
+ _taskExecutor = new CurrentThreadTaskExecutor();
+ _taskExecutor.start();
+ }
+
+ @Override
+ public void tearDown() throws Exception
+ {
+ super.tearDown();
+ _taskExecutor.stopImmediately();
+ }
+
+ public void testInvalidTableNamePrefix() throws Exception
+ {
+ SystemConfig systemConfig = mock(SystemConfig.class);
+ Broker broker = mock(Broker.class);
+ final ConfiguredObjectFactoryImpl factory = new ConfiguredObjectFactoryImpl(BrokerModel.getInstance());
+ when(broker.getObjectFactory()).thenReturn(factory);
+ when(broker.getModel()).thenReturn(factory.getModel());
+ when(broker.getChildExecutor()).thenReturn(_taskExecutor);
+ when(broker.getParent()).thenReturn(systemConfig);
+
+ Map<String, Object> attributes = new HashMap<>();
+ attributes.put(ConfiguredObject.NAME, getTestName());
+ attributes.put(ConfiguredObject.TYPE, JDBCVirtualHostNodeImpl.VIRTUAL_HOST_NODE_TYPE);
+ attributes.put("connectionUrl", "jdbc://example.com");
+ JDBCVirtualHostNode<?> jdbcVirtualHostNode = new JDBCVirtualHostNodeImpl(attributes, broker);
+
+ // This list is not exhaustive
+ List<String> knownInvalidPrefixes = Arrays.asList("with\"dblquote",
+ "with'quote",
+ "with-dash",
+ "with;semicolon",
+ "with space",
+ "with%percent",
+ "with|pipe",
+ "with(paren",
+ "with)paren",
+ "with[bracket",
+ "with]bracket",
+ "with{brace",
+ "with}brace");
+ for (String invalidPrefix : knownInvalidPrefixes)
+ {
+ try
+ {
+ jdbcVirtualHostNode.setAttributes(Collections.<String, Object>singletonMap("tableNamePrefix",
+ invalidPrefix));
+ fail(String.format("Should not be able to set prefix to '%s'", invalidPrefix));
+ }
+ catch (IllegalConfigurationException e)
+ {
+ // pass
+ }
+ }
+ }
+}
