Skip to content

fontconfig_210: remove #92919

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fontconfig_210: remove #92919

wants to merge 1 commit into from
+1 −49

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Jul 11, 2020 •
edited
Loading

fontconfig 2.10.x hasn't had a release in years, is nowhere used inside
nixpkgs and vulnerable to CVE-2016-5384.

Motivation for this change

#88289

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@flokli
fontconfig_210: remove
99f4118 
fontconfig 2.10.x hasn't had a relase in years, is nowhere used inside
nixpkgs and vulnerable to CVE-2016-5384.
@flokli flokli added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Jul 11, 2020
@flokli flokli requested review from vcunat and worldofpeace July 11, 2020 10:51
@flokli
Copy link
Contributor Author

flokli commented Jul 11, 2020 •
edited
Loading

Corresponding 20.03 PR that marks this as insecure: #92921

@flokli flokli mentioned this pull request Jul 11, 2020
Closed
10 tasks
@flokli flokli mentioned this pull request Jul 11, 2020
Closed
1 task
vcunat
vcunat requested changes Jul 11, 2020
View reviewed changes
Copy link
Member

@vcunat vcunat left a comment

Choose a reason for hiding this comment

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

It is still used in nixos/modules/config/fonts/fontconfig.nix. I suspect we don't really need to support systems with fontconfig < 2.11 (CentOS 6 is the only one I can think of), but in that case we should look into cleaning that reference.

@flokli
Copy link
Contributor Author

flokli commented Jul 11, 2020

Okay, I can remove this. We might want to resurrect the code when bumping fontconfig to 2.13 and keep backwards-compat support for 2.12, but then it can easily be resurrected from the history I guess.

@flokli
Copy link
Contributor Author

flokli commented Jul 11, 2020

Urgh, just took a closer look - in fontconfig-conf we use $out/etc/fonts/conf.d for the old version, and $out/etc/fonts/${latestVersion}/conf.d for the more recent version… Shouldn't it be the other way around?

prusnak
prusnak requested changes Jul 11, 2020
View reviewed changes
pkgs/top-level/aliases.nix
@@ -149,6 +149,7 @@ mapAliases ({
fontconfig-ultimate has been removed. The repository has been archived upstream and activity has ceased for several years.
https://github.com/bohoomil/fontconfig-ultimate/issues/171.
'';
fontconfig_210 = throw "fontconfig 2.10.x hasn't had a relase in years, is nowhere used inside nixpkgs and vulnerable to CVE-2016-5384"; # 2020-07-11
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fontconfig_210 = throw "fontconfig 2.10.x hasn't had a relase in years, is nowhere used inside nixpkgs and vulnerable to CVE-2016-5384"; # 2020-07-11
fontconfig_210 = throw "fontconfig 2.10.x hasn't had a release in years, is nowhere used inside nixpkgs and vulnerable to CVE-2016-5384"; # 2020-07-11

@worldofpeace
Copy link
Contributor

Urgh, just took a closer look - in fontconfig-conf we use $out/etc/fonts/conf.d for the old version, and $out/etc/fonts/${latestVersion}/conf.d for the more recent version… Shouldn't it be the other way around?

If there's any issue see here so we can fix them in one go #73795

@flokli flokli mentioned this pull request Jul 11, 2020
Merged
10 tasks
@vcunat
Copy link
Member

vcunat commented Jul 11, 2020 •
edited
Loading

I expect that severity of 2.10 remaining is low (in the current way). Just getting the config files from 2.10 shouldn't be vulnerable, and it seems very unlikely that someone opted in to use 2.10 in some other way. Still, it would be nice to clean this up during that 2.13/2.14 update.

@flokli
Copy link
Contributor Author

flokli commented Jul 11, 2020

Okay, fine to not pursue this any further if we can address this during #73795 soon - it was already suggested in #73795 (comment) anyways.

@flokli flokli closed this Jul 11, 2020
@cole-h
Copy link
Member

cole-h commented Jul 13, 2020

Doesn't look like this made it into #73795. Should we reopen?

@flokli
Copy link
Contributor Author

flokli commented Jul 14, 2020

I reopened #88289.

@flokli
Copy link
Contributor Author

flokli commented Jul 14, 2020

@cole-h would you be up to proposing a PR addressing #88289?

@cole-h
Copy link
Member

cole-h commented Jul 14, 2020

I'm unclear on what direction I should go in, if I were to accept. Do we want to remove 2.10, or just patch it (https://cgit.freedesktop.org/fontconfig/commit/?id=7a4a5bd7897d216f0794ca9dbce0a4a5c9d14940, according to the vulnerability page)?

In the case we want to remove it, how should I handle the aforementioned nixos/modules/config/fonts/fontconfig.nix? I will admit, I'm not well-versed in fontconfig (much less our fontconfig situation), so I don't expect to get it right upon submission.

Once these questions are answered, I can find some time during the week to tackle this. However, I wouldn't be offended if somebody beats me to the punch... ;^)

@vcunat
Copy link
Member

vcunat commented Jul 14, 2020

I expect we want to bump supportVersion to 212 (and thus bring back the 2.12.6 expression), at least from #73795 (comment)

@jtojnar
Copy link
Member

jtojnar commented Jul 16, 2020

2.12 is not necessary since our patched 2.14 should be compatible with 2.11+. We should just drop support version altogether once we are sure there are no regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prusnak prusnak

@vcunat vcunat

@Valodim Valodim

@worldofpeace worldofpeace

Assignees
No one assigned
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@flokli @worldofpeace @vcunat @cole-h @jtojnar @Valodim @prusnak