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
jq: Fix CVE-2015-8863 and CVE-2016-4074 #18908
jq: Fix CVE-2015-8863 and CVE-2016-4074 #18908
Conversation
|
@aneeshusa, thanks for your PR! By analyzing the annotation information on this pull request, we identified @7c6f434c, @ryanartecona and @flazz to be potential reviewers |
|
|
||
| stdenv.mkDerivation rec { | ||
| name = "jq-${version}"; | ||
| version = "something"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a valid version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I had pushed an old version by mistake.
| version = "something"; | ||
|
|
||
| # Fix for CVE-2015-8863 is in master, | ||
| # but no release has been made since then (latest release was last year) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you fetch the master? For stability it would be more useful to pick the relevant patches from master until there is a new release. You can use fetchpatch to avoid committing those to nixpkgs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest jq release was last August, and there have been numerous changes in between. I've never been a fan of "backporting culture" and I wouldn't feel comfortable blindly extracting a patch and applying it without studying the source in depth to see more precisely what has changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yet you blindly package a development version without knowing if it is stable and safe to use? I'm also not a big fan of backporting to previous versions but sticking to released versions is good practice. If the project doesn't even do maintenance releases it's not safe to assume that it their master is always stable in my experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case it seems the 1.6 release is delayed and they are recommending to use master:
- Inconsistent behavior of jq with no options when standard output is redirected stedolan/jq#1243 (comment)
- weird behavior during substitutions stedolan/jq#1239 (comment)
I also took a closer look at the patches and they are smaller than I thought, so I will investigate if backporting to 1.5 is feasible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see. In that case it could make sense to use the master. But we should review the changes in master and do some tests before using that version. Do you know why 1.6 is delayed? Is that due to a bug that could be a dealbreaker?
| }; | ||
|
|
||
| # Patch is not yet merged to master | ||
| patches = [ ./CVE-2016-4074.patch ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this patch come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use fetchpatch.
d2b3587
to
d0a0b1c
Compare
jq has not had a release since v1.5 in August 2015, so backport both of these patches (the fix for CVE-2015-8863 is in the current master, while the fix for CVE-2016-4074 is not yet in master).
d0a0b1c
to
c38d3b6
Compare
|
Updated to stay on v1.5, although IMO the development version would also be fine. |
|
Related to: #18856 |
|
I'll merge this for now. If you are confident the git master is indeed okay, please open another PR. Thanks for your work! |
jq has not had a release since v1.5 in August 2015, so backport both of these patches (the fix for CVE-2015-8863 is in the current master, while the fix for CVE-2016-4074 is not yet in master). (cherry picked from commit bfbca9d)
jq has not had a release since v1.5 in August 2015, so backport both of these patches (the fix for CVE-2015-8863 is in the current master, while the fix for CVE-2016-4074 is not yet in master).
|
Have both of these CVEs been fixed? Looking at: seems to indicate that the CVE-2016-4074 still apply to jq version 1.5 (not patched). Is this correct? The page: more clearly shows a patch. However, I'd just like confirmation that both of these CVEs are patched in jq version 1.5. I'm asking the question because I was just told by a co-worker that a BlackDuck scan reported jq 1.5 and the above 2 CVEs. This would force us to stop using jq until a production version is available that fixes the issues. Thanks, |
|
@fatbearinc jq 1.5 is not safe, but this PR pulls in two patches for jq 1.5 for the CVEs, so the jq package in Nixpkgs is patched against both CVEs. |
|
I also just reconfirmed that we are indeed applying the correct patches to fix both CVEs. |
Motivation for this change
Part of #18856.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)jq has not had a release since August 2015, so use the latest version
from git. CVE-2015-8863 has a fix committed to the current master, while
CVE-2016-4074 has not had a fix committed to master, so add a separate
patch for that.
I manually confirmed that the CVEs are fixed in the resulting binary.