Backport security fix from #4119 to v1

[acrobat]

This is:

• a bugfix (security fix)
• a new feature
• refactoring
• additional unit tests

Checklist:

• Changes are covered by unit tests
  • Changes are covered by existing unit tests
  • New unit tests have been added
• Code style is respected
• Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
• CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
• Documentation is updated as necessary

Why this change is needed?

This PR backports the security fix of #4119 to v1 because currently the use of Roave/SecurityAdvisories blocks installs of v1 without this fix. If this is merged the patched versions of the cve should be updated

[patrickbrouwers]

Would be great if it could be backported, so Laravel Excel can keep running on v1 until I'm able to finish the v4 release

[nuernbergerA]

This would be great as yajra datatables is also affected

[oleibman]

@PowerKiKi @MarkBaker There is clearly a lot of support for this PR, but I'm afraid I don't understand what it's trying to do. I don't know what the "release" branch is or how it is used. For that matter, I don't even know what "v1" is. Can you evaluate and merge if appropriate? And explain for me regardless?

[acrobat]

@PowerKiKi @MarkBaker There is clearly a lot of support for this PR, but I'm afraid I don't understand what it's trying to do. I don't know what the "release" branch is or how it is used. For that matter, I don't even know what "v1" is. Can you evaluate and merge if appropriate? And explain for me regardless?

The "v1" mentioned here is the version 1 of this library, my guess was that the release branch was still that version of the code. If I need to target a different branch, please let me know.

[oleibman]

@acrobat We are currently on Release 2.2.2 (which includes the security fix). Why are you not using it rather than V1? (Genuine curiosity, not snarkiness, I assure you.) Release 2 arrived over 7 months ago. The release branch was last updated in February 2023 and is 874 commits behind the master branch. It doesn't even have the changes for release 1.29.0 which came out in June 2023.

[Flightfreak]

Other packages that rely on the library have not yet adopted the breaking changes since the 2.X release. 1.29 was the last one before and having this security fix as 1.29.1 on top would allow many projects to resolve this security issue.

[SanderMuller]

@acrobat We are currently on Release 2.2.2 (which includes the security fix). Why are you not using it rather than V1? (Genuine curiosity, not snarkiness, I assure you.) Release 2 arrived over 7 months ago. The release branch was last updated in February 2023 and is 874 commits behind the master branch. It doesn't even have the changes for release 1.29.0 which came out in June 2023.

Packages have to release a major release to go from v1 to v2, which is far from ideal for a security fix. This will not only take longer to prepare on the packages end, but will also slow down adoption which means vulnerable projects for longer.

[Flightfreak]

@oleibman if you’d like, i have a branch ready with all commits until tag 1.29.0 + the backport of the security fix. Ideally this is merged as a “1” branch en receives a 1.29.1 tag

[oleibman]

@Flightfreak I like that approach. But, as I indicated earlier, I'm in a little over my head on what the PR would be doing. I understand it a little better now thanks to everyone's explanations, but I would still like to hear what my colleagues have to say.

[ovidenov]

Hello,

Drupal's module vbo_export https://www.drupal.org/project/vbo_export , relies in it's composer.json for a version of this library ^1.6 ,
In our CI pipeline this library is marked as insecure on each deploy.
I think it will make sense to update the v1. branch/tag as soon as possible.

Regards,
Oleg

[rjbijl]

@acrobat We are currently on Release 2.2.2 (which includes the security fix). Why are you not using it rather than V1? (Genuine curiosity, not snarkiness, I assure you.) Release 2 arrived over 7 months ago. The release branch was last updated in February 2023 and is 874 commits behind the master branch. It doesn't even have the changes for release 1.29.0 which came out in June 2023.

@oleibman also, the 2.2.2 won't run on PHP 7.4, which some applications (for numerous reasons) still use. So a backport of the fix to 1.x would be highly appreciated.

[jvadesso]

As we are currently transitioning an application to PHP 8(.3), but are not there yet that we can make that jump, I would appreciate the backport as well. 👍

[alexislefebvre]

If this version is released as 1.30.0, it will still be marked as vulnerable since the vulnerabilities on https://github.com/PHPOffice/PhpSpreadsheet/security show this:

Affected versions <2.1.0
Affected versions <2.2.1

Will maintainers be able to mark <1.29.0 as vulnerable but not 1.30.0?

[oleibman]

I have created a request which covers both fixes. I have not yet pushed it because I am still not sure of all the ramifications. @PowerKiKi or @MarkBaker or, indeed, anyone else here, please help. Here's what I've done so far.

• Checked out 1.29.0 making a "headless" version.
• Made and tested the changes.
• Created a branch.
• Committed.

Which is where I stand now. This is much different than my usual change because the code came directly from the PhpSpreadsheet repository, not my forked version (checkout did not find the appropriate version in my own repository). So, my first question is: If I push, it will just create a new branch and not do anything else, right?

Assuming the answer to my first question is yes, can I create a pull request from the new branch? Will it target 1.29.0 rather than master?

Again assuming the expected answers, I can now merge the PR and tag a 1.29.1 release. Does Composer/Packagist continue to think that the current release is 2.2.2, or has it changed to 1.29.1 and I have to do something to reset it? If so, what must I do?

And then the big question. What do I do to the CVE to indicate that it is fixed in both 1.29.1 and 2.2.2+, but not for other releases? I am having trouble finding any documentation on that subject.

[Flightfreak]

@oleibman correct you can create the branch, and if not yet tagged, it won't do any harm.

To my limited understanding, PHPOffice ideally maintains a release branch for each "major" version. Tags are tied to commits not branches. As you can't insert the security fix into the past of the current "master" branch without rewriting the whole branch history, you need a separate branch on which you can add that additional commit that can then be tagged. Packagist/composer will work out the details themselves, they don't consider the branches. They will understand the 2.2.2 is the highest one.

I think, from what I can see, the vulnerability report can be edited and additional "affected products" and "version conditions" can be defined. I suspect that if you set it like this, it will be fine:
Affected versions:
>=2.0.0, <2.2.1, <1.29.0

Patched versions:
>=2.2.2, >=1.29.1, <=2.0.0

[oleibman]

@Flightfreak Thank you for the information. I will attempt the push in a few hours, and will then try to establish a schedule for the rest.

[oleibman]

There is now a branch security-patch in PhpSpreadsheet with the 2 fixes. (For the record, the earlier fix was the result of routine maintenance before we ever saw the security advisory that it fixed.) After some hiccoughs, it has successfully run the unit test suite for Php7.4-8.3. I think no PR is required, and would probably be disastrous if attempted. I will attempt to tag it tomorrow night (I am in California) in order to give people an opportunity to comment before I do so. If the tag succeeds, I will try to update the CVE's. We usually tag things from the command line, however I will probably try it from the Github interface because it has a button (something like "set as latest release") which I want to make sure to uncheck (despite earlier reassurances, I am nervous), and I'm not sure what the equivalent would be from the command line.

If this all succeeds, I think this PR is no longer needed. I am changing it to draft status now for that reason.

[nuernbergerA]

For all that cant wait till the release you can already use the security patch by adding this to your composer.json

{
    "require": {
        "phpoffice/phpspreadsheet": "dev-security-patch as 1.29.1"
    }
}

[oleibman]

For updating the Security Advisory, I submitted a question to Github Support, which was answered immediately by Copilot. It suggests a slight amendment to what @Flightfreak suggested above:

The suggested version ranges are almost correct, but there's a small adjustment needed.
The patched versions should not include <=2.0.0 because it's not a patched version range. Here's the corrected version:

Affected versions: >=2.0.0, <2.2.1, <1.29.1

Patched versions: >=2.2.2, >=1.29.1

This means that all versions from 2.0.0 up to, but not including 2.2.1, and all versions below 1.29.1 are affected.
The versions that have the fix are 2.2.2 and above, and 1.29.1 and above.

As for your question about moving to release 3, you wouldn't need to update the advisory again.
The >=2.2.2 covers all future versions above 2.2.2, including release 3.

So that is what I will try later, unless somebody thinks it is wrong.

[oleibman]

And ... the information at https://docs.github.com/en/code-security/security-advisories/guidance-on-reporting-and-writing-information-about-vulnerabilities/best-practices-for-writing-repository-security-advisories#version-syntax seems to contradict that. (It was a link in the reply.)

You cannot specify multiple affected version ranges in the same field, such as > 2.0, < 2.3, > 3.0, < 3.2
To specify more than one range, you must create a new Affected products section for each range,
by clicking the + Add another affected product button.

So that is what I will try (usual caveats).

[oleibman]

Release 1.29.1 is tagged, and is available in Packagist/Composer. Security Advisory is updated in PhpSpreadsheet to indicate that 1.29.1 is okay, but that hasn't propagated anywhere. I have opened an issue with Github Support to see if there's anything else I need to do.

[jaulz]

@oleibman thanks a lot for your work!

[patrickbrouwers]

@oleibman Thanks for releasing! Much appreciated!

[rjbijl]

@oleibman Thanks for the effort. And you learned a whole lot in the process ;)

[Flightfreak]

Thank you @oleibman and all involved 👍🏻

[oleibman]

My ticket with Github Support has disappeared. I have opened a new one.

[macbookandrew]

@oleibman thank you!!

[oleibman]

1.29.1 is no longer showing as vulnerable at Packagist. I still have no idea what needed to be done to accomplish that. If I get an answer from Github Support, I will post the relevant bits here. In the meantime, thank you all for your comments - this has been an education. My apologies for the inconvenience.

[SanderMuller]

@oleibman thank you for your hard work!

For now it seems that roave/security-advisories is still conflicting with 1.29.1, but maybe it's caching since GHSA-wgmf-q9vr-vww6 and GHSA-ghg6-32f9-2jp7 seem to mark it as a patched version and Roave pulls from either GitHub Advisories or https://github.com/FriendsOfPHP/security-advisories/tree/master/phpoffice/phpspreadsheet

[oleibman]

Here is what Github Support had to say:

The version ranges had two issues. The first is there needs to be a space between the operators (<= and >=) and the version. The second is that vulnerable version ranges are not allowed to overlap, e.g. < 2.1.0 still says that 1.29.0 is affected. To resolve the conflict, I changed < 2.1.0 to >= 2.0.0, < 2.1.0. You find our guidance on specifying vulnerable version ranges here.

So, even though it accepted the originally published advisory, which also did not have a space between operator and version, it needed the space for the update. At any rate, here's a picture of what one of the updated advisories now says:

[alexislefebvre]

• GHSA-ghg6-32f9-2jp7 shows “Patched versions 2.2.1” but
• GHSA-wgmf-q9vr-vww6 shows “Patched versions >=2.1.0”.

May it cause issues when 2.2.2 or 2.3.0 will be released? It won't match the range of the first advisory. Should it be >= 2.2.1 instead?

---

Update: actually, the range is shown only if we access to the reports from this URL https://github.com/PHPOffice/PhpSpreadsheet/security:

• GHSA-ghg6-32f9-2jp7 has no range for “Patched versions 2.2.1”
• GHSA-wgmf-q9vr-vww6 has ranges for 2 releases

[oleibman]

@alexislefebvre I think we're okay. The Html fix in the Github Advisory Database (which I believe is the one that needs to be correct) just specifies 2.1.0 as a patched version, yet 2.2.1 and 2.2.2 aren't shown as vulnerable at packagist. Likewise, the other one specifies 2.2.1 as the patched version, and 2.2.2 isn't shown as vulnerable to it. So I don't think we'll have a problem with 2.3.0 or 3.0.0. The difference between the local version of the advisory and the one at Github is that the latter was handled by someone who know what they were doing, and the former was handled by me. I don't think any humans will be misled by the local versions, and, since I really don't want to touch anything else, I will leave them as is.

[ChinhTQ-Vnext]

I am sorry but please apply to version use php 7.3. Thank you

[Flightfreak]

@ChinhTQ-Vnext are you aware that PHP 7.3 self is no longer receiving security support since the end of the year 2021?
https://www.php.net/supported-versions.php

[ChinhTQ-Vnext]

@Flightfreak
I am sorry, I'm really not clear about this. But this means that php 7.4 will not receive security support since the end of the year 2022?. However, PHPOffice/PhpSpreadsheet version 1.29.1 apply to fix security error and used with php ^7.4.

[marineusde]

@Flightfreak I am sorry, I'm really not clear about this. But this means that php 7.4 will not receive security support since the end of the year 2022?. However, PHPOffice/PhpSpreadsheet version 1.29.1 apply to fix security error and used with php ^7.4.

Why do you need a security fix for a package if your php version is extrem unsecure?

[ChinhTQ-Vnext]

@marineusde
We are required to keep the php version the same and find a fix security . This is a pre-built project and we have the right to upgrade php.

[SanderMuller]

@marineusde We are required to keep the php version the same and find a fix security . This is a pre-built project and we have the right to upgrade php.

The only right people have related to open source is to fork the project and fix it yourself.
So I would recommend forking and bumping the PHP version yourself and use the fork in your project.

[Flightfreak]

@ChinhTQ-Vnext, a difficult situation indeed. From what I can see the last tag supporting PHP 7.3 is 1.25.2. I guess you could back-port this particular fix into a 1.25.3 tag on your own fork and submit a PR here. It might get accepted.

[oleibman]

@ChinhTQ-Vnext 1.29.1 was applied not to allow continued support for Php7.4, it was applied to allow continued support for PhpSpreadsheet Release 1, which can be used with supported versions of Php. People may not yet have had time to evaluate whether PhpSpreadsheet Release 2 meets their needs. We hope that they will do so soon, but there is no need for us to force that issue. Php7.4 users were incidental beneficiaries of the change. Without wishing to discuss it further (I will not respond to any replies on this particular aspect), they, and you, should upgrade Php as soon as possible.

[peterjaap]

You can just use the patch with vaimo/composer-patches to patch v1 in your local install;

{
  "phpoffice/phpspreadsheet": {
    "source": "https://patch-diff.githubusercontent.com/raw/PHPOffice/PhpSpreadsheet/pull/4119.patch",
    "level": 1
  }
}

$ composer patch:apply
Processing patches configuration
  - Applying patches for phpoffice/phpspreadsheet (1)
    ~ my/project: https://patch-diff.githubusercontent.com/raw/PHPOffice/PhpSpreadsheet/pull/4119.patch [NEW]
      source

Writing patch info to install file
Patched the PhpStorm config

[PowerKiKi]

I am late to the party, but I second everything that @oleibman said. This backporting is an exceptional case. PhpSpreadsheet does not maintains a release branch for each "major" version, precisely because we don't have the resource to handle multiple versions at once. If you want the latest security patches, you have to either upgrade PhpSpreadsheet or fork it and backport the patch yourself.

So I'd like to thank @oleibman for going the extra mile with this.