From 0a71b037405a7e846ae51cd13be79efb39c9b2c7 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Thu, 27 Feb 2020 12:57:29 -0800 Subject: [PATCH 1/3] prevent database connections to sqlite --- superset/config.py | 4 +++ superset/security/analytics_db_safety.py | 30 +++++++++++++++++++ superset/views/core.py | 9 ++++++ superset/views/database/mixins.py | 5 +++- tests/security/analytics_db_safety_tests.py | 32 +++++++++++++++++++++ 5 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 superset/security/analytics_db_safety.py create mode 100644 tests/security/analytics_db_safety_tests.py diff --git a/superset/config.py b/superset/config.py index 5d9de46b4d22e..6168c51f131c6 100644 --- a/superset/config.py +++ b/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 diff --git a/superset/security/analytics_db_safety.py b/superset/security/analytics_db_safety.py new file mode 100644 index 0000000000000..64c7711f333e8 --- /dev/null +++ b/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." + ) diff --git a/superset/views/core.py b/superset/views/core.py index d8766436978d6..72ef6ea6c83f0 100755 --- a/superset/views/core.py +++ b/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 @@ -1314,6 +1318,8 @@ def testconn(self): db_name = request.json.get("name") uri = request.json.get("uri") try: + if app.config.get("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, @@ -1365,6 +1371,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))) except Exception as e: logger.error("Unexpected error %s", e) return json_error_response(_("Unexpected error occurred."), 400) diff --git a/superset/views/database/mixins.py b/superset/views/database/mixins.py index 219900cdeb61e..98386555d31bc 100644 --- a/superset/views/database/mixins.py +++ b/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.get("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) diff --git a/tests/security/analytics_db_safety_tests.py b/tests/security/analytics_db_safety_tests.py new file mode 100644 index 0000000000000..04a64551e6f18 --- /dev/null +++ b/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") From d0dee1127a4cd8518f169180bc78d45a044ddb63 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Fri, 28 Feb 2020 14:36:11 -0800 Subject: [PATCH 2/3] tweaks and tests --- superset/views/core.py | 4 ++-- superset/views/database/mixins.py | 2 +- tests/core_tests.py | 27 ++++++++++++++++++++++++++- 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/superset/views/core.py b/superset/views/core.py index 72ef6ea6c83f0..2b203f09e347b 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -1318,7 +1318,7 @@ def testconn(self): db_name = request.json.get("name") uri = request.json.get("uri") try: - if app.config.get("PREVENT_UNSAFE_DB_CONNECTIONS"): + 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. @@ -1373,7 +1373,7 @@ def testconn(self): ) except DBSecurityException as e: logger.warning("Stopped an unsafe database connection. %s", e) - return json_error_response(_(str(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) diff --git a/superset/views/database/mixins.py b/superset/views/database/mixins.py index 98386555d31bc..13b5bb3faa82f 100644 --- a/superset/views/database/mixins.py +++ b/superset/views/database/mixins.py @@ -192,7 +192,7 @@ class DatabaseMixin: } def _pre_add_update(self, database): - if app.config.get("PREVENT_UNSAFE_DB_CONNECTIONS"): + if app.config["PREVENT_UNSAFE_DB_CONNECTIONS"]: check_sqlalchemy_uri(database.sqlalchemy_uri) self.check_extra(database) self.check_encrypted_extra(database) diff --git a/tests/core_tests.py b/tests/core_tests.py index c497febb04b81..bb22b16be707d 100644 --- a/tests/core_tests.py +++ b/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")) @@ -433,9 +435,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( { @@ -482,6 +485,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) From 3ef1a0fa1a7485bfe9f573d50ddb5fa2cc107631 Mon Sep 17 00:00:00 2001 From: David Aaron Suddjian Date: Fri, 28 Feb 2020 14:43:05 -0800 Subject: [PATCH 3/3] add entry to UPDATING.md --- UPDATING.md | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 3d20f4904242d..f69f2379118e6 100644 --- a/UPDATING.md +++ b/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.