Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent csv/xls injection (#429) #430

Closed
wants to merge 3 commits into from

Conversation

timwentzell
Copy link

Subject

Suggested security update to address #429

This is a simple PR to filter data cells exported to CSV and XLS that start with dangerous characters that can be interpreted by spreadsheet applications.

I am targeting this branch, because each Writer class is marked as final and the easiest solution is to offer this security update to everyone.

Closes #429.

Changelog

### Security
- Prevent csv/xls injection in Writers (#429)

@greg0ire
Copy link
Contributor

greg0ire commented Jan 6, 2021

Isn't escaping an option? If I open excel and I want to write a formula verbatim, without it being executed, surely there must be a way to do this?

@VincentLanglet
Copy link
Member

If I understand you're replacing "-" by "!-" ?

This would break a lot of code IMHO

@timwentzell
Copy link
Author

Isn't escaping an option? If I open excel and I want to write a formula verbatim, without it being executed, surely there must be a way to do this?

It seems when editing spreadsheets in Excel directly the general "rule-of-thumb" to prevent a cell from attempting to be executed as a formula is to prepend the cell with an apostrophe. I can update my PR to replace the ! with an ' - apparently Excel will hide the single apostrophe, however I can't confirm that since I don't have access to Excel at the moment.

src/Writer/CsvWriter.php Outdated Show resolved Hide resolved
Comment on lines 81 to 85
// prevent xls injection
$patterns = ['/^=/', '/^\+/', '/^-/', '/^@/'];
$replace = ['!=', '!+', '!-', '!@'];
foreach ($data as $value) {
fwrite($this->file, sprintf('<td>%s</td>', $value));
fwrite($this->file, sprintf('<td>%s</td>', preg_replace($patterns, $replace, $value)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please apply the same here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes good point - no need of those variables being set.

@greg0ire
Copy link
Contributor

greg0ire commented Jan 6, 2021

@timwentzell sorry, I failed to notice that you were already doing some kind of escaping.

@VincentLanglet he's only doing that when the dash is at the start of the line.

$replace = ['!=', '!+', '!-', '!@'];
foreach ($data as $key => $value) {
$data[$key] = preg_replace($patterns, $replace, $value);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As pointed out by @VincentLanglet , that can break a lot of things. Here, we don't know that this is going to go through Excel, I don't think this writer should be changed.

foreach ($data as $value) {
fwrite($this->file, sprintf('<td>%s</td>', $value));
fwrite($this->file, sprintf('<td>%s</td>', preg_replace($patterns, $replace, $value)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case people actually rely on the values not being escaped, should their be an option to disable the escaping?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I was just thinking maybe this could be a configurable option. I had a client run a pen test on an application using Sonata Admin with the Exporter bundle and they identified this vulnerability - and I had to fix it or else they weren't going to use our application. I didn't like the idea of manipulating the cell data but it was the only option I could find to "plug the hole".

If this isn't useful for the community that's totally fine, for now I've just made my own CsvWriter and XlsWriter classes and use those in place of the ones in this bundle, but that makes me have to closely monitor the changes in those classes when I want to update this bundle - which is no fun at all...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case people actually rely on the values not being escaped, should their be an option to disable the escaping?

I have now made it optional, defaulting to not used (false) so any existing applications using this bundle will not experience any change unless they explicitly add the safe_cells configuration parameter and set it to true for one or both writers.

@greg0ire
Copy link
Contributor

greg0ire commented Jan 6, 2021

Instead of adding that option to each and every future writer that produces files that can be opened with Excel, how about we do this (which you can already do in your project, and allows you not to monitor every change, BTW):

class SafeCellWriterDecorator implements TypedWriterInterface
{
    /** @var TypedWriterInterface */
    private $wrapped;

    public function __construct(TypedWriterInterface $wrapped)
    {
        // here we might want to check if it makes sense to do this with `instanceof CsvWriter`, etc.  
        $this->wrapped = $wrapped;
    }

    public function getDefaultMimeType(): string
    {
        return $this->wrapped->getDefaultMimeType();
    }

    public function getFormat(): string
    {
        return $this->wrapped->getFormat();
    }

    public function open()
    {
        $this->wrapped->open();
    }

    public function close()
    {
        return $this->wrapped->close();
    }

    public function write(array $data)
    {
        // Here, you do your preg_replace()
        return $this->wrapped->write($data);
    }
}

Then we use the configuration nodes you introduce to decide whether we use service decoration or not. It will make it easier for you to write tests, by the way.

Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a lot better !

I only find weird having a csv with ' at the beginning.
The csv could be used for both excel and something else. But since it's an option...

@greg0ire
Copy link
Contributor

greg0ire commented Jan 6, 2021

By the way, are the headers considered safe?

@timwentzell
Copy link
Author

By the way, are the headers considered safe?

Well the headers are normally dependent on the Entity fields and translation labels - so IMO they would be safe.

@greg0ire
Copy link
Contributor

greg0ire commented Jan 6, 2021

Then it's slightly more complex than what I proposed: you would have to take into account showHeaders when implementing SafeCellWriterDecorator::write

@VincentLanglet
Copy link
Member

What is needed to merge this @greg0ire ?

@greg0ire
Copy link
Contributor

greg0ire commented Apr 8, 2021

@VincentLanglet IMO src/Writer/CsvWriter.php should not be touched, and this should be implemented with service decoration instead, as show in the snippet above, only it should also take into account showHeaders to decide whether to act on the first row or not.

@VincentLanglet
Copy link
Member

@timwentzell Are you still interested in this PR ?

if (true === $this->safeCells) {
foreach ($data as $key => $value) {
$data[$key] = preg_replace(
['/^=/', '/^\+/', '/^-/', '/^@/'],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing \r and \t si Symfony's CsvEncoder for example

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@github-actions
Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jun 18, 2022
@github-actions github-actions bot closed this Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent CSV and XLS injection in Writers
5 participants