[Serializer] Fix security issue on CsvEncoder about CSV injection #24508

welcoMattic · 2017-10-10T13:14:43Z

Q	A
Branch?	master (4.1)
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
License	MIT

I read this article about CSV injection and I thought it best to update the CsvEncoder so that it does not generate potentially malicious CSV files by default.

sroze · 2017-10-10T13:22:26Z

Great initiative 👍 I'd target 3.4.

javiereguiluz

In addition to prefixing a \t to problematic values, another solution proposed in the comments of the article is to prefix a = before the "..." because that forces Excel to interpret the value as text. Not sure if it's really better.

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php

src/Symfony/Component/Serializer/Tests/Encoder/CsvEncoderTest.php

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php

welcoMattic · 2017-10-11T08:37:45Z

Thanks @chalasr @kaznovac @xabbuh for your comments and review.
I just push fixes. I'm agree to set $escapeFormulas at false by default, after all negative numbers doesn't have to be escaped. And it will avoid BC Break.

Should I replace all assertEquals by assertSame in CsvEncoderTest class?

chalasr · 2017-10-11T08:40:09Z

@welcoMattic Nope, that would create merge conflicts when merging lower branches up to master. Thanks.

xabbuh · 2017-10-11T08:40:58Z

Should we also allow to pass this option through the context so that you do not necessary have to configure this option globally?

welcoMattic · 2017-10-11T08:47:58Z

@xabbuh why not. We should also make the context $escapeFormulas value overrides the global $escapeFormulas value. It could be set to true globally, but for one particular encoding it could be set to false.

chalasr · 2017-10-11T08:49:17Z

👍 to allow overriding this option in the context.

welcoMattic · 2017-10-11T13:49:24Z

@chalasr @xabbuh Overriding is now possible

dunglas · 2017-10-12T05:58:57Z

Great!

But as stated in the article, it's definitely not a security issue on our side. The current implementation respects the RFC (Excel and Google Sheets don't), and most software consuming CSV expect the current behavior.
It should made be clear in the docs PR that this option should only be used when the generated CSV will be imported in spreadsheets or it can cause interoperability issues.

welcoMattic · 2017-10-12T15:58:17Z

Thanks @dunglas!

For documentation, should I create a new page under Serializer section? Named CsvEncoder, and where I explain why this options exists?

ostrolucky · 2017-10-16T01:08:55Z

Alternative approach might be to just add a normalizer. That would help with deserialization of such sanitized csvs too and doesn't pollute __construct arguments.

I don't like having this in core anyway though, as it's security issue in Excel and Google sheets only and this is destructive kind of escaping

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php

fabpot · 2017-12-01T14:50:59Z

Not sure if we need to support that in core. /cc @symfony/mergers

Tobion · 2017-12-10T13:39:03Z

👍 to include that as it's security feature that people should be aware about.

dbu

i think in 95% of the use cases, the serializer will be used to export data, which might include user provided input.

there is only a problem if this component is used to generate sheets with formula. i would add an explaination in the phpdoc to explain what this flag is about and say to set it to true when you don't intend to have formula in the csv you generate.

i am no merger, but +1 on this. it is a relevant security topic and obviously excel/libreoffice/google and whoever else is affected decided they rather not properly fix it, so its up to generators of csv files to ensure security.

i think this should be against 3.3 as that is the branch for security fixes.

dunglas · 2017-12-20T15:07:50Z

👍 to support this feature too. But as it's definitely not a security fix (on our side), it's a new feature that should be merged in 4.1.

there is only a problem if this component is used to generate sheets with formula.

There is also a problem when the value start with '=', '-', '+', '@' and the parser supports correctly the RFC: a \t will be added, but it should not (actually, when using this flag, it's not really a true CSV file anymore).

welcoMattic · 2017-12-20T15:40:56Z

Thanks @dbu @dunglas for your comments. I think that we can support this "fix" and explain it explicitly in PHPDoc.

Should I update the PR now, or we need to wait other opinions?

dbu · 2017-12-20T15:42:41Z

i'd update the changelog and phpdoc as that will be relevant either way. i hope one of the maintainers can decide which version this should go. i can see the point of kevin, and its a good reason why its not on by default. but i feel it still is security relevant, even if its not a bug of symfony, but symfony helping around lax security of other tools - and as such should go into 3.3 or 3.4.

dunglas · 2018-01-11T10:29:52Z

I think that you can update and rebase the PR @welcoMattic

src/Symfony/Component/Serializer/CHANGELOG.md

welcoMattic · 2018-02-04T18:16:53Z

@nicolas-grekas Done, I also fix @ostrolucky's comment.

nicolas-grekas · 2018-02-04T18:19:23Z

@welcoMattic thanks. Can you rebase to get rid of the merge commit? I invite you to squash while doing so.

Tobion · 2018-02-06T10:14:10Z

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php


-    public function __construct(string $delimiter = ',', string $enclosure = '"', string $escapeChar = '\\', string $keySeparator = '.')
+    public function __construct(string $delimiter = ',', string $enclosure = '"', string $escapeChar = '\\', string $keySeparator = '.', $escapeFormulas = false)


missing bool type declaration for the new argument

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php

welcoMattic · 2018-02-06T10:22:25Z

Thanks for review @Tobion 👍

fabpot · 2018-02-07T05:12:31Z

Thank you @welcoMattic.

…V injection (welcoMattic) This PR was merged into the 4.1-dev branch. Discussion ---------- [Serializer] Fix security issue on CsvEncoder about CSV injection | Q | A | ------------- | --- | Branch? | master (4.1) | Bug fix? | no | New feature? | yes | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | License | MIT I read [this article](http://georgemauer.net/2017/10/07/csv-injection.html) about CSV injection and I thought it best to update the `CsvEncoder` so that it does not generate potentially malicious CSV files by default. Commits ------- a1b0bdb Fix security issue on CsvEncoder

carsonbot added Status: Needs Review Serializer Feature BC Break labels Oct 10, 2017

welcoMattic force-pushed the fix/csv-encoder-formulas-char-escaping branch from 0cfa2ac to baf31e5 Compare October 10, 2017 13:15

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 10, 2017

javiereguiluz approved these changes Oct 10, 2017

View reviewed changes

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php Outdated Show resolved Hide resolved

welcoMattic force-pushed the fix/csv-encoder-formulas-char-escaping branch from 6d7dc03 to e254fcb Compare October 10, 2017 13:34

chalasr reviewed Oct 10, 2017

View reviewed changes

src/Symfony/Component/Serializer/Tests/Encoder/CsvEncoderTest.php Outdated Show resolved Hide resolved

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php Outdated Show resolved Hide resolved

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php Outdated Show resolved Hide resolved

chalasr added Status: Needs Review and removed Status: Reviewed labels Oct 10, 2017

xabbuh added this to the 4.1 milestone Oct 11, 2017

welcoMattic force-pushed the fix/csv-encoder-formulas-char-escaping branch from 527dc6a to fc743a1 Compare October 11, 2017 08:38

chalasr removed the BC Break label Oct 11, 2017

welcoMattic force-pushed the fix/csv-encoder-formulas-char-escaping branch 2 times, most recently from 09a5ed5 to 9c0733d Compare October 11, 2017 13:48

dunglas approved these changes Oct 12, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 12, 2017

ostrolucky reviewed Oct 16, 2017

View reviewed changes

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php Outdated Show resolved Hide resolved

dbu approved these changes Dec 20, 2017

View reviewed changes

nicolas-grekas requested changes Feb 4, 2018

View reviewed changes

src/Symfony/Component/Serializer/CHANGELOG.md Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Reviewed labels Feb 4, 2018

welcoMattic force-pushed the fix/csv-encoder-formulas-char-escaping branch 2 times, most recently from b98b9ab to 414a61d Compare February 4, 2018 18:30

Tobion reviewed Feb 6, 2018

View reviewed changes

src/Symfony/Component/Serializer/Encoder/CsvEncoder.php Outdated Show resolved Hide resolved

Fix security issue on CsvEncoder

a1b0bdb

welcoMattic force-pushed the fix/csv-encoder-formulas-char-escaping branch from 414a61d to a1b0bdb Compare February 6, 2018 10:21

fabpot approved these changes Feb 7, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Feb 7, 2018

fabpot merged commit a1b0bdb into symfony:master Feb 7, 2018

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

welcoMattic deleted the fix/csv-encoder-formulas-char-escaping branch February 4, 2020 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] Fix security issue on CsvEncoder about CSV injection #24508

[Serializer] Fix security issue on CsvEncoder about CSV injection #24508

welcoMattic commented Oct 10, 2017 •

edited

Loading

sroze commented Oct 10, 2017

javiereguiluz left a comment

welcoMattic commented Oct 11, 2017

chalasr commented Oct 11, 2017

xabbuh commented Oct 11, 2017

welcoMattic commented Oct 11, 2017

chalasr commented Oct 11, 2017

welcoMattic commented Oct 11, 2017

dunglas commented Oct 12, 2017

welcoMattic commented Oct 12, 2017

ostrolucky commented Oct 16, 2017 •

edited

Loading

fabpot commented Dec 1, 2017

Tobion commented Dec 10, 2017

dbu left a comment

dunglas commented Dec 20, 2017

welcoMattic commented Dec 20, 2017

dbu commented Dec 20, 2017 •

edited

Loading

dunglas commented Jan 11, 2018

welcoMattic commented Feb 4, 2018

nicolas-grekas commented Feb 4, 2018

Tobion Feb 6, 2018

welcoMattic commented Feb 6, 2018

fabpot commented Feb 7, 2018


		public function __construct(string $delimiter = ',', string $enclosure = '"', string $escapeChar = '\\', string $keySeparator = '.')
		public function __construct(string $delimiter = ',', string $enclosure = '"', string $escapeChar = '\\', string $keySeparator = '.', $escapeFormulas = false)

[Serializer] Fix security issue on CsvEncoder about CSV injection #24508

[Serializer] Fix security issue on CsvEncoder about CSV injection #24508

Conversation

welcoMattic commented Oct 10, 2017 • edited Loading

sroze commented Oct 10, 2017

javiereguiluz left a comment

Choose a reason for hiding this comment

welcoMattic commented Oct 11, 2017

chalasr commented Oct 11, 2017

xabbuh commented Oct 11, 2017

welcoMattic commented Oct 11, 2017

chalasr commented Oct 11, 2017

welcoMattic commented Oct 11, 2017

dunglas commented Oct 12, 2017

welcoMattic commented Oct 12, 2017

ostrolucky commented Oct 16, 2017 • edited Loading

fabpot commented Dec 1, 2017

Tobion commented Dec 10, 2017

dbu left a comment

Choose a reason for hiding this comment

dunglas commented Dec 20, 2017

welcoMattic commented Dec 20, 2017

dbu commented Dec 20, 2017 • edited Loading

dunglas commented Jan 11, 2018

welcoMattic commented Feb 4, 2018

nicolas-grekas commented Feb 4, 2018

Tobion Feb 6, 2018

Choose a reason for hiding this comment

welcoMattic commented Feb 6, 2018

fabpot commented Feb 7, 2018

welcoMattic commented Oct 10, 2017 •

edited

Loading

ostrolucky commented Oct 16, 2017 •

edited

Loading

dbu commented Dec 20, 2017 •

edited

Loading