keepassxc: Enable networking by default #103146
Conversation
Upstream builds with it enabled, as do other distros.
|
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... :) |
|
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. |
|
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. |
|
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. |
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 |
|
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 |
|
Why was this merged arbitrarily? |
|
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 |
|
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 |
|
@jonafato Would you like me to revert this? |
This has been fixed; see NixOS/nixpkgs#103146
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
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)