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

Backport rubyzip fix to 2.0.2 #536

Closed
mdavidn opened this issue Jun 13, 2017 · 19 comments
Closed

Backport rubyzip fix to 2.0.2 #536

mdavidn opened this issue Jun 13, 2017 · 19 comments
Labels
Done in caxlsx This has already been solved in the caxlsx fork.

Comments

@mdavidn
Copy link

mdavidn commented Jun 13, 2017

I'm looking to add Axlsx to one of my projects without downgrading rubyzip to a vulnerable version. It's not clear to me why a critical fix for remote code execution is blocked by new functionality in #501. @randym, please backport the rubyzip constraint and release 2.0.2 to RubyGems.org.

I'm happy to help. There's no tag for 2.0.1 (see #535), so I'm not certain which commit to use as base. Here's my best guess. I had to backport a commit from #304 to make tests pass. I definitely don't understand the full context around rubyzip updates, so let me know if I overlooked something.

I'm happy to open a PR against a 2.x major-version branch if you'd create one.

@damphyr
Copy link

damphyr commented Jun 22, 2017

Oh please do this, there's a ton of other problems with the 1.0.0 version of rubyzip.

@perlun
Copy link

perlun commented Jul 27, 2017

We've been using 7cf7476 for a long time. Unsure of which rubyzip version it depends on though. I looked carefully, this is 2.0.1 + an updated version of rubyzip. But yeah, it would be great to get a new release of the gem out.

For now, this is your best (= least bad) bet:

gem 'axlsx', git: 'https://github.com/randym/axlsx.git',
             ref: '7cf747675097be13df633f1b2a5c45391df52b33' # 2.0.1 + updated version of rubyzip

@perlun
Copy link

perlun commented Jul 27, 2017

Ah, what a pity: I looked more carefully, rubyzip/rubyzip#315 is only included in v1.2.1 of Rubyzip. And the referred-to commit still pins rubyzip to ~> 1.1.1 which means we cannot get the fixed version.

Note to all people reading this maintaining/developing rubygems: Don't peg a dependency like that. It means your consumers cannot ever upgrade to version 1.2, 1.3, 1.4 etc of rubyzip. Instead, use a less pessimistic pinning: ~> 1.1. That means you accept anything in the 1.x series of the dependency to be compatible with your gem.

(If everyone would just read the Semantic Versioning specification and try to follow it, the above would be more-or-less foolproof. Reality is however that sometimes expected-to-be-compatible versions are not so compatible. Still, I believe the approach I suggest here is the "lesser evil", and the Rubygems do explicitly encourage people to stick to Semantic Versioning: http://guides.rubygems.org/patterns/#semantic-versioning)

More details on Rubygems version pinning: http://guides.rubygems.org/patterns/#declaring-dependencies

@ingemar
Copy link

ingemar commented Sep 26, 2017

@randym bump!

@rpbaptist
Copy link

@sandstrom That's pretty great. It serves my purposes as well and have replaced AXLSX with it. Thanks for the pointer!

@perlun
Copy link

perlun commented Sep 26, 2017

@sandstrom We meet again, hope you're doing great with Skovik. 😉 Thanks for the suggestion. Unfortunately, it's not that easy for us since we have a whole ecosystem of our own (not just an app or two). So just removing a dependency and replacing it with another is not always trivial. So for us, the safe (and boring!) approach is to peg towards the known-good commit of this repo.

The rest of you can surely go ahead and use a better gem. Newer versions of our ecosystem do not depend on the axlsx gem as heavily, so for this use case your suggestion might be an idea indeeed.

@md5
Copy link
Contributor

md5 commented Oct 11, 2017

@randym Any chance we could get the target branches that @mdavidn is asking for? It would be great to get his fixes into a 2.0.2 and 2.1.0.pre2 release.

@perlun
Copy link

perlun commented Oct 16, 2017

@md5 Don't hope too much. There hasn't been a single commit merged into this repo for months. 😢 (Happy to be wrong though, but it feels like someone should fork this or work with @randym to be added as a maintainer.)

@ro31337
Copy link

ro31337 commented Jan 4, 2018

You can also update your Gemfile this way:

gem 'axlsx', git: 'https://github.com/NoRedInk/axlsx.git',
             ref: '1a4a6387bf398e2782933ee6607e5589cd15bee3' # 2.1.0-pre-with-new-rubyzip, see why https://github.com/randym/axlsx/issues/536

Thanks to @michaelglass from @NoRedInk

@randym
Copy link
Owner

randym commented Feb 14, 2018

release-3.0.0 branch will be the next alfa release.

@randym randym closed this as completed Feb 14, 2018
@mdavidn
Copy link
Author

mdavidn commented Jul 6, 2018

@randym After 13 months, every stable release of axlsx remains vulnerable to CVE-2017-5946. While you test the next major release, I urge you to backport the fix and release a stable 2.0.2.

I provided such a backport above. You can review and release it thusly:

git remote add mdavidn [email protected]:mdavidn/axlsx.git
git fetch mdavidn release-2.0

## Review and test
git checkout -B release-2.0 mdavidn/release-2.0
# I believe 46a17e4 was the basis for v2.0.1.
git log -p 46a17e4b3fe0531ac6b2e48f818a982a86f538e4..HEAD
rake test

## Release
rake release
git tag v2.0.2
git push -u origin v2.0.2 release-2.0

Alternatively, I will open a PR against a release-2.0 branch if you create one.

git tag v2.0.1 46a17e4b3fe0531ac6b2e48f818a982a86f538e4
git branch -f release-2.0 v2.0.1
git push -u origin v2.0.1 release-2.0

@randym
Copy link
Owner

randym commented Jul 6, 2018 via email

@perlun
Copy link

perlun commented Jul 27, 2018

@randym

release-3.0.0 (pre-released on rubygems https://rubygems.org/gems/axlsx/versions/) is where we want to be.

Sure, thanks for that. We all appreciate it as the long-term fix.

Does this mean there will be no backport of the fix like the one @mdavidn provided? Not all users might be happy to jump on a new major version to get a security bug fix. If time does not permit you to work on axls very much at the current time, maybe it would be a good idea to add more maintainers to the project (to release the burden of maintaining it slightly).

@noniq
Copy link
Collaborator

noniq commented Aug 29, 2018

I’m too totally in favor of getting out a 2.0.2 release containing the RubyZip fix (only). It can be done quickly (I’ll volunteer in preparing the releasing, if necessary), it won’t cause any harm at all, and it will help a lot of folks using this great gem.

@mdavidn
Copy link
Author

mdavidn commented Jan 12, 2019

@randym Nudge on this issue. After 19 months, we're still waiting for a (non-prerelease) release that fixes a remote code execution vulnerability.

I'm happy to help with maintenance if you're short on time.

@michaelglass
Copy link

michaelglass commented Jan 12, 2019

@mdavidn Looks like maintaining version 2 isn't a priority. What advantage do you get from the mainline gem if it's not actively maintained?

I'd suggest you fork and release a new gem if you want, or just use the one we host (see comment above).

@mdavidn
Copy link
Author

mdavidn commented Jan 18, 2019

I have been using a fork, but I would prefer not to.

@semaperepelitsa
Copy link

We couldn't upgrade to 2.1 or 3.0 due to breaking changes, so I created a fork of 2.0.1 with Rubyzip 1.2 support. Maybe somebody finds it useful:

gem "axlsx", "~> 2.0.1", git: "https://github.com/semaperepelitsa/axlsx", branch: "2.0-rubyzip"

Diff: semaperepelitsa/axlsx@v2.0.1...semaperepelitsa:2.0-rubyzip

@straydogstudio
Copy link
Contributor

2.0.2 version released for new Community Axlsx (http://github.com/caxlsx/caxlsx)

@noniq noniq added the Done in caxlsx This has already been solved in the caxlsx fork. label Dec 15, 2019
freesteph added a commit to openregister/registers-frontend that referenced this issue Feb 11, 2020
spreadsheet_architect already pulls-in a newer version of axlsx that
does not have the security vulnerability[1] in it. Ideally we should keep
an eye on the imminent release[2] of spreadsheet_architect that moves
away from axlsx in favour of the community-maintained caxlsx[3].

[1]: randym/axlsx#536
[2]: westonganger/spreadsheet_architect#28 (comment)
[3]: westonganger/spreadsheet_architect@3a6f02d#diff-4ac32a78649ca5bdd8e0ba38b7006a1e
freesteph added a commit to openregister/registers-frontend that referenced this issue Feb 11, 2020
spreadsheet_architect will soon be released[1] with the axlsx
dependency removed, in favour of the community-maintained caxlsx[2]
that does not have the security vulnerability of the former[3].

Ideally we should keep an eye on that release and pin it down in our
Gemfile but until then roll with the master branch as it seems to be
working just fine.

[1]: westonganger/spreadsheet_architect#28 (comment)
[2]: westonganger/spreadsheet_architect@3a6f02d#diff-4ac32a78649ca5bdd8e0ba38b7006a1e
[3]: randym/axlsx#536
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Done in caxlsx This has already been solved in the caxlsx fork.
Projects
None yet
Development

No branches or pull requests