Prevent database connections to sqlite (#9218)

* prevent database connections to sqlite

* tweaks and tests

* add entry to UPDATING.md

UPDATING.md

@@ -23,10 +23,14 @@ assists people when migrating to a new version.
 
 ## Next
 
+* [9218](https://github.com/apache/incubator-superset/pull/9218): SQLite connections have been disabled by default
+for analytics databases. You can optionally enable SQLite by setting `PREVENT_UNSAFE_DB_CONNECTIONS` to `False`.
+It is not recommended to change this setting, as arbitrary SQLite connections can lead to security vulnerabilities.
+
 * [9133](https://github.com/apache/incubator-superset/pull/9133): Security list of permissions and list views has been
-disable by default. You can optionally enable them back again by setting the following config keys: 
-FAB_ADD_SECURITY_PERMISSION_VIEW, FAB_ADD_SECURITY_VIEW_MENU_VIEW, FAB_ADD_SECURITY_PERMISSION_VIEWS_VIEW to True.
- 
+disable by default. You can optionally enable them back again by setting the following config keys:
+`FAB_ADD_SECURITY_PERMISSION_VIEW`, `FAB_ADD_SECURITY_VIEW_MENU_VIEW`, `FAB_ADD_SECURITY_PERMISSION_VIEWS_VIEW` to `True`.
+
 * [9173](https://github.com/apache/incubator-superset/pull/9173): Changes the encoding of the query source from an int to an enum.
 
 * [9120](https://github.com/apache/incubator-superset/pull/9120): Changes the default behavior of ad-hoc sharing of
@@ -49,9 +53,9 @@ timestamp has been added to the query object's cache key to ensure updates to
 datasources are always reflected in associated query results. As a consequence all
 previously cached results will be invalidated when updating to the next version.
 
-* [8699](https://github.com/apache/incubator-superset/pull/8699): A `row_level_security_filters` 
-table has been added, which is many-to-many with `tables` and `ab_roles`.  The applicable filters 
-are added to the sqla query, and the RLS ids are added to the query cache keys. If RLS is enabled in config.py (`ENABLE_ROW_LEVEL_SECURITY = True`; by default, it is disabled), they can be 
+* [8699](https://github.com/apache/incubator-superset/pull/8699): A `row_level_security_filters`
+table has been added, which is many-to-many with `tables` and `ab_roles`.  The applicable filters
+are added to the sqla query, and the RLS ids are added to the query cache keys. If RLS is enabled in config.py (`ENABLE_ROW_LEVEL_SECURITY = True`; by default, it is disabled), they can be
 accessed through the `Security` menu, or when editting a table.
 
 * [8732](https://github.com/apache/incubator-superset/pull/8732): Swagger user interface is now enabled by default.

superset/config.py

@@ -760,6 +760,10 @@ class CeleryConfig:  # pylint: disable=too-few-public-methods
 # SQLALCHEMY_DATABASE_URI by default if set to `None`
 SQLALCHEMY_EXAMPLES_URI = None
 
+# Some sqlalchemy connection strings can open Superset to security risks.
+# Typically these should not be allowed.
+PREVENT_UNSAFE_DB_CONNECTIONS = True
+
 # SIP-15 should be enabled for all new Superset deployments which ensures that the time
 # range endpoints adhere to [start, end). For existing deployments admins should provide
 # a dedicated period of time to allow chart producers to update their charts before

superset/security/analytics_db_safety.py

@@ -0,0 +1,30 @@
+# 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.
+
+
+class DBSecurityException(Exception):
+    """ Exception to prevent a security issue with connecting a DB """
+
+    status = 400
+
+
+def check_sqlalchemy_uri(uri):
+    if uri.startswith("sqlite"):
+        # sqlite creates a local DB, which allows mapping server's filesystem
+        raise DBSecurityException(
+            "SQLite database cannot be used as a data source for security reasons."
+        )

superset/views/core.py

@@ -78,6 +78,10 @@
 from superset.models.slice import Slice
 from superset.models.sql_lab import Query, TabState
 from superset.models.user_attributes import UserAttribute
+from superset.security.analytics_db_safety import (
+    check_sqlalchemy_uri,
+    DBSecurityException,
+)
 from superset.sql_parse import ParsedQuery
 from superset.sql_validators import get_validator_by_name
 from superset.utils import core as utils, dashboard_import_export
@@ -1322,6 +1326,8 @@ def testconn(self):
         db_name = request.json.get("name")
         uri = request.json.get("uri")
         try:
+            if app.config["PREVENT_UNSAFE_DB_CONNECTIONS"]:
+                check_sqlalchemy_uri(uri)
             # if the database already exists in the database, only its safe (password-masked) URI
             # would be shown in the UI and would be passed in the form data.
             # so if the database already exists and the form was submitted with the safe URI,
@@ -1373,6 +1379,9 @@ def testconn(self):
             return json_error_response(
                 _("Connection failed, please check your connection settings."), 400
             )
+        except DBSecurityException as e:
+            logger.warning("Stopped an unsafe database connection. %s", e)
+            return json_error_response(_(str(e)), 400)
         except Exception as e:
             logger.error("Unexpected error %s", e)
             return json_error_response(_("Unexpected error occurred."), 400)

superset/views/database/mixins.py

@@ -20,8 +20,9 @@
 from flask_babel import lazy_gettext as _
 from sqlalchemy import MetaData
 
-from superset import security_manager
+from superset import app, security_manager
 from superset.exceptions import SupersetException
+from superset.security.analytics_db_safety import check_sqlalchemy_uri
 from superset.utils import core as utils
 from superset.views.database.filters import DatabaseFilter
 
@@ -191,6 +192,8 @@ class DatabaseMixin:
     }
 
     def _pre_add_update(self, database):
+        if app.config["PREVENT_UNSAFE_DB_CONNECTIONS"]:
+            check_sqlalchemy_uri(database.sqlalchemy_uri)
         self.check_extra(database)
         self.check_encrypted_extra(database)
         database.set_sqlalchemy_uri(database.sqlalchemy_uri)

tests/core_tests.py

@@ -75,9 +75,11 @@ def setUp(self):
         self.table_ids = {
             tbl.table_name: tbl.id for tbl in (db.session.query(SqlaTable).all())
         }
+        self.original_unsafe_db_setting = app.config["PREVENT_UNSAFE_DB_CONNECTIONS"]
 
     def tearDown(self):
         db.session.query(Query).delete()
+        app.config["PREVENT_UNSAFE_DB_CONNECTIONS"] = self.original_unsafe_db_setting
 
     def test_login(self):
         resp = self.get_resp("/login/", data=dict(username="admin", password="general"))
@@ -444,9 +446,10 @@ def test_misc(self):
         assert self.get_resp("/ping") == "OK"
 
     def test_testconn(self, username="admin"):
+        # need to temporarily allow sqlite dbs, teardown will undo this
+        app.config["PREVENT_UNSAFE_DB_CONNECTIONS"] = False
         self.login(username=username)
         database = utils.get_example_database()
-
         # validate that the endpoint works with the password-masked sqlalchemy uri
         data = json.dumps(
             {
@@ -493,6 +496,28 @@ def test_testconn_failed_conn(self, username="admin"):
             expected_body,
         )
 
+    def test_testconn_unsafe_uri(self, username="admin"):
+        self.login(username=username)
+        app.config["PREVENT_UNSAFE_DB_CONNECTIONS"] = True
+
+        response = self.client.post(
+            "/superset/testconn",
+            data=json.dumps(
+                {
+                    "uri": "sqlite:///home/superset/unsafe.db",
+                    "name": "unsafe",
+                    "impersonate_user": False,
+                }
+            ),
+            content_type="application/json",
+        )
+        self.assertEqual(400, response.status_code)
+        response_body = json.loads(response.data.decode("utf-8"))
+        expected_body = {
+            "error": "SQLite database cannot be used as a data source for security reasons."
+        }
+        self.assertEqual(expected_body, response_body)
+
     def test_custom_password_store(self):
         database = utils.get_example_database()
         conn_pre = sqla.engine.url.make_url(database.sqlalchemy_uri_decrypted)

tests/security/analytics_db_safety_tests.py

@@ -0,0 +1,32 @@
+# 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.
+
+from superset.security.analytics_db_safety import (
+    check_sqlalchemy_uri,
+    DBSecurityException,
+)
+
+from ..base_tests import SupersetTestCase
+
+
+class DBConnectionsTest(SupersetTestCase):
+    def test_check_sqlalchemy_uri_ok(self):
+        check_sqlalchemy_uri("postgres://user:password@test.com")
+
+    def test_check_sqlalchemy_url_sqlite(self):
+        with self.assertRaises(DBSecurityException):
+            check_sqlalchemy_uri("sqlite:///home/superset/bad.db")