benjreinhart commented on a change in pull request #13735: URL: https://github.com/apache/superset/pull/13735#discussion_r601780418
########## File path: tests/utils/csv_tests.py ########## @@ -0,0 +1,60 @@ +# 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. +# pylint: disable=no-self-use +import io + +import pandas as pd +import pytest + +from superset.utils import csv + + +def test_escape_value(): + result = csv.escape_value("value") + assert result == "value" + + result = csv.escape_value("-10") + assert result == "-10" + + result = csv.escape_value("@value") + assert result == "'@value" + + result = csv.escape_value("+value") + assert result == "'+value" + + result = csv.escape_value("-value") + assert result == "'-value" + + result = csv.escape_value("=value") + assert result == "'=value" Review comment: TLDR: it was what I saw recommended the most when googling around. Longer: I saw a few different recommendations but they all seem to be intrusive in one form or another. The approach I saw most often recommended was the single preceding quote. There is also a [tab approach](http://georgemauer.net/2017/10/07/csv-injection.html), but that looks invasive to some degree as well. However, now that I just went back to look this up again, I found [a post](https://asecurityz.blogspot.com/2017/12/csv-injection-mitigations.html) that shows how even this is not sufficient: >One product in my organization fixed a similar issue with 2 layers of defense. For any CSV cell value that starts with +, -, @, = (as suggested in http://georgemauer.net/2017/10/07/csv-injection.html or OWASP) the fix added (1) a preceding TAB char, (2) single quotes around the cell value. But later the we found that adding a single / pair of double quote(s) before the attacker's payload can simply evade the filter to check for the chars +, -, @, =. e.g. if the payload attacker injects is =2+5+cmd|' /C calc'!A0 filter will catch it and mitigate the risks. But if attacker puts the payload ""=2+5+cmd|' /C calc'!A0 filter won't be able to catch it as it was checking for only values starting with +, -, @, =. The end result will be same because MS excel, while rendering the value, simply skips the leading double quotes in CSV values. Their recommendation is: > prepend a SPACE or TAB or SINGLE QUOTE to ALL CSV values before exporting them to file. This mitigation leaves the CSV file human readable but not executable. DO NOT check for leading +, -, @, =, " and prepend to only those values. Does that sound like a valid approach or do you have a preferred alternative? -- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
