- π³πΏNew Zealand RoSk0 Wellington
I'm curious if the double quote character is the correct one to be used here.
We were recommended to use a single quote character to escape field data starting with equals sign by the security company to avoid potential remote code execution.
- Status changed to Needs review
over 1 year ago 10:46pm 8 May 2023 - last update
over 1 year ago 4 pass - π³πΏNew Zealand RoSk0 Wellington
When setting a cell value,
PhpOffice\PhpSpreadsheet
will use default value binder (\PhpOffice\PhpSpreadsheet\Cell\DefaultValueBinder
), if none provided. This binder is responsible for guessing data type based on its value and this is where string starting from equals sign is getting a formula data type assigned.I don't like current implementation because it modifies original data. I believe there is a better way, or even two.
During testing, I noticed that most office suites would prepend the data with a single quote to treat the value as a plain string and prevent formula processing. At the same time this data would be rendered in a cell without the prepended single quote. We could use this option if we really want to modify original data.
A better option, I believe, would be to tell
PhpOffice\PhpSpreadsheet
, and consuming office software, to treat values starting from equals sign as string and prevent formula processing. The user would than need to explicitly enable formula processing for the affected cell. This is far better position from the security point of view, in my opinion.Tested in:
- LibreOffice
- WPS Office
- SoftMaker Office
- MS Office
Latter option is implemented in this patch.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
The security team agreed to keep this in public
- Status changed to Needs work
over 1 year ago 11:07pm 29 May 2023 - π©πͺGermany FeyP
Note that escaping the equal sign is not enough to prevent CSV injection. OWASP has a list of characters to escape, @larowlan already posted that one. This list is also used by Smyfony in its serialization component: https://symfony.com/blog/cve-2021-41270-prevent-csv-injection-via-formulas So the patch most likely needs to be updated to include the additional characters. Setting to "Needs work" for that.
Quoting relevant portion here:
Symfony now follows the OWASP recommendations and use the single quote
'
to prefix formulas and adds the prefix to cells starting by\t
,\r
as well as=
,+
,-
and@
.I think it would be best to follow the guidelines from OWASP, and to prefix such data with
'
- First commit to issue fork.
- Merge request !18Draft: Issue #3268139 by rosk0: Strings starting with "=" creates unwanted formulas β (Closed) created by ericgsmith
- π³πΏNew Zealand ericgsmith
Moved patch from #6 to MR and rebased against 8.x-1.x.
Setting MR as draft and leaving as needs work as have not looked at suggested changes in above comments 10 to 12 yet.
-
theemstra β
committed 43b50814 on 8.x-1.x
Issue #3268139 by rosk0: Strings starting with "=" creates unwanted...
-
theemstra β
committed 43b50814 on 8.x-1.x
- Status changed to Fixed
4 months ago 12:21pm 4 September 2024 Regarding #11, are there any plans to format values starting with the other recommended characters?
Automatically closed - issue fixed for 2 weeks with no activity.