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

keepassxc: Enable networking by default #103146

Merged
merged 1 commit into from Nov 17, 2020
Merged

Conversation

talyz
Copy link
Contributor

@talyz talyz commented Nov 8, 2020

Motivation for this change

Upstream builds with it enabled, as do other distros. This enables favicon fetching and possibly other network dependent features.

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.

Upstream builds with it enabled, as do other distros.
@asdf8dfafjk
Copy link
Contributor

Have you considered the downsides? Tomorrow if there is a vulnerability in jpeg rendering all my passwords are "gg".

What "other networking features" are there that one can want without knowing about them I wonder... :)

@prusnak
Copy link
Member

prusnak commented Nov 8, 2020

What other networking features are enabled?

Having favicons does not sound like a worthy addition to justify this change which most possibly opens new attack vectors.

@jonafato
Copy link
Contributor

jonafato commented Nov 8, 2020

This flag was originally added with the same default of "off" that upstream's cmake lists uses. At that time, it was required for the browser integration (KeePassHTTP), which has since been replaced by KeePassBrowser. KeePassXC recently introduced a HaveIBeenPwned integration (keepassxreboot/keepassxc#4438) that relies on networking, and this seems like more a compelling feature to switch the default for than favicon downloading. The only other feature that networking enables that I'm aware of is update checking, which is not relevant for nixpkgs.

I don't have strong feelings on the default value of this flag. If it's going to flip, HIBP integration is probably the best reason to do so, as it provides some tangible security benefit to users.

@talyz
Copy link
Contributor Author

talyz commented Nov 8, 2020

After browsing the code I could find two other network features that are enabled or depend on the flag being set: update checking and have i been pwned integration. I've manually enabled this for quite a while and find the favicon fetching very convenient. I'm not sure exactly what possible attack you're imagining, but I would guess other KeePassXC features would be better targets - the browser integration, for instance. In any case, if you're worried you can just opt not to use it - favicon fetching doesn't happen automatically when this flag is set to true. Like I wrote in the original post, upstream builds with this enabled and other distros do too; that should indicate that they don't find this particularly dangerous.

@asdf8dfafjk
Copy link
Contributor

asdf8dfafjk commented Nov 9, 2020

wonder what attack you're imagine

... Everyone else doing it.

To me your reply indicates that you're not first hand familiar with what keepass is actually doing and how and I'm even more negative on this PR sorry.

Do we know if they use http or https for fetching? If it's http- gg. If https, do they fail on a certificate failure? If not , what if someine sends a malicious jpeg (mitm)? Even if yes, what if someone socially engineers and submits a malicios jpeg for inclusion. Are they looking at every last icon? These arguments can be made for the current icon set and other binary resources too but the point is to not make things worse.

Security cannot happen by democracy. I find your argument of everyone is doing so we can too improper. Please argue from first principles.

Not all of us can keep track of every new change (possibly an insecurity vector) and expecting everyone to spend time changing their settings is improper. I wonder if you can spend some extra time on this PR and try to figure out a global setting that disables binary resource fetching ?

Sorry all this might sound harsh but these are all facts. Sensitive programs need to be treated differently from end user programs.

I agree about HIBP and it's arguably useful enough to justify inclusion. That said, we need to consider the average keepass user, and even there the average NixOS user, and there's a decent chance that all passwords are unique and high entropy. (I believe HIBP also informs if any of your accounts has been taken over but I'm hoping that my service provider would inform me too)

Considering all this, and that the default cached builds should always be for the most conservative use case since any additional changes are just one override away, I propose that this PR be kindly rejected.

@talyz
Copy link
Contributor Author

talyz commented Nov 9, 2020

Firstly, I think you're misquoting me / taking what I said out of context. My takeaway from upstream enabling this is that people who are likely more qualified than us have deemed the potential risk imposed by this option to be negligible and I point out that other distros enable it because that means people coming from those will expect the features to be available (as I did); I did not mean to say that "everyone else is doing it, therefore it's a good idea".

Secondly, no I have not studied the code carefully, if that's what you were implying; I trust that upstream provides something secure enough for my use case - if you don't, you probably shouldn't use its software.

Lastly, I feel like you're worrying way too much over a feature that could theoretically be used as an attack vector when there's a much more obvious one already enabled - the browser integration. The way the favicon fetching works, it's also entirely optional, even when enabled at compile time; you have to press a dedicated fetch favicon button for it to actually do it.

@talyz talyz merged commit d5b1464 into NixOS:master Nov 17, 2020
@talyz talyz deleted the keepassxc-networking branch November 17, 2020 22:31
@asdf8dfafjk
Copy link
Contributor

Why was this merged arbitrarily?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/misuse-of-commit-permissions/10071/1

@talyz
Copy link
Contributor Author

talyz commented Nov 18, 2020

Well, I responded to your comments and more than a week went by without any further objections. Providing the expected, vanilla feature set shouldn't be this controversial. Like I've stated twice already, the favicon fetching is still optional even when it's compiled in.

If you really hate this, you can still use an override an disable it. If you want it in the build cache, you could even add a keepassxcNoNetworking attribute. Also, this is only in master, it hasn't been (and probably shouldn't be) backported to any stable release.

@talyz
Copy link
Contributor Author

talyz commented Nov 18, 2020

@jonafato Would you like me to revert this?

hpfr added a commit to hpfr/system that referenced this pull request Apr 23, 2021
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.

None yet

5 participants