keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
21.06k stars 1.46k forks source link

Debian No-Feature KeePassXC Package #10725

Closed CedricSchmeits closed 5 months ago

CedricSchmeits commented 5 months ago

Overview

I'm using the Brave and Firefox browsers under Ubuntu testing using keepassxc version 2.7.7, suddenly the browser integration doesn't work anymore. So I went into the settings menu to enabled it again. But the "Browser Integration" from the listbox selection is gone. Not only that one but there are only two items left:

  1. Algemeen (General)
  2. Beveiliging (Security) See the picture in the attachment. All the other options are missing, so I can't enable the browser integration anymore. Schermafdruk van 2024-05-10 11-40-55

Steps to Reproduce

  1. Open keepassxc
  2. Goto Tools -> Settings
  3. Here the options are missing

Expected Behavior

Working browser options and working integration

Actual Behavior

Browser integration is not working and the options to enable them are not available anymore

Context

KeePassXC - Version 2.7.7 Revision: 68e2dd8

Qt 5.15.10 Debugging mode is disabled.

Operating system: Debian GNU/Linux trixie/sid CPU architecture: x86_64 Kernel: linux 6.7.12-amd64

Enabled extensions:

Cryptographic libraries:

droidmonkey commented 5 months ago

@julian-klode this needs to be reverted asap. This is now our fourth bug report because of the decision to neuter the base KeePassXC package in Debian. Put the base package back where it was and create a keepassxc-minimal.

julian-klode commented 5 months ago

I'm afraid that's not going to happen. It was a mistake to ship with all plugins built by default. This will be painful for a year as users annoyingly do not read the NEWS files they should be reading but there's little that can be done about that.

It is our responsibility to our users to provide them the most secure option possible as the default. All of these features are superfluous and do not really belong in a local password database manager, these developments are all utterly misguided.

Users who need this crap can install the crappy version but obviously this increases the risk of drive-by contributor attacks.

droidmonkey commented 5 months ago

Good luck to you. Really bad decision. We will be sure to let everyone know.

It is our responsibility to our users to provide them the most secure option possible as the default

It's the same code base from the same provider. Do you have a documented, validated security issue with ALL of the features we provide? Otherwise, it's your opinion (and likely yours alone) so 🤷🏻

emdnaia commented 5 months ago

The dev is right every plugin should be selected by the user and can be considered a potential security risk either backdoor potential or vulnerability potential. Nothing is more sacred than a password vault, especially on the most trusted distro there is.

droidmonkey commented 5 months ago

You fundamentally misunderstand our program when you use the word plugin. These are built in features, not plugins. The features can be enabled as desired by the user and they come disabled by default. This change to not compile and ship these features in the base keepassxc package does nothing besides create angry (or confused) users.

TheZ3ro commented 5 months ago

My 2 cent as a former KeePassXC maintainer: I agree with @julian-klode.

Compiling without plugin was added as a feature back in 2016 to reduce attack surface and potential vulnerabilites (see #50, #123, #125). This was especially a heated topic back then because KeeHTTP had vulnerability in the past and also the favicon fetching mechanism via HTTP was (is?) also sucettible to SSRF attacks.

IMHO KeePassXC should provide a warning UI in place of the usual "Browser integration" page informing the user that the current installation don't have such capabilities and they should install the "full" version instead.

Jordo0G commented 5 months ago

Im not to familiar with software development, but does this mean Julian is not compiling the same source code in this repo?

droidmonkey commented 5 months ago

@TheZ3ro I'm all for a keepassxc-minimal package that has no or limited features. To switch the main keepassxc package from full featured to no feature is the problem at hand here. We will bear the brunt of this decision with issues and complaints, not Debian or Julian.

@Jordo0G it is the same code just built with feature flags turned off. So not all the code is present in the final build.

Salamandar commented 5 months ago

as users annoyingly do not read the NEWS files they should be reading

I don't know how to reply but that's difficult to stay calm while users are insulted that way.

You can't expect all users to read NEWS/CHANGELOGs from the hundreds of packages installed on their machine. There's an implicit trust contract maintainers should follow that packages don't change behaviour that way, unless explicit requests are made from the upstream project.

theandrewbailey commented 5 months ago

Is this change expected to be rolled out in default installs on other platforms? Browser integration is a bullet point feature (along with TOTP and SSH agent), and I might move to another fork if they are discontinued or not enabled by default, or this change makes its way to Debian stable.

phoerious commented 5 months ago

As @droidmonkey said, none of these features are plugins. All of them are built-in functionality that belong to the main product. If anything, we will reduce the number of such compile-time flags in the future, so these things cannot be disabled anymore. For example, one that will definitely go is WITH_XC_YUBIKEY. It has served its purpose, but it is becoming a burden. More flags mean more possible configurations in which bugs can occur and it is impossible to test them all. The safest KeePassXC is not the one with all flags disabled, but the one which is tested best by us and by the majority of our users. The only real safe way to reduce the code surface is to actually remove code, not to ifguard sections of it and hope that disabling them doesn't introduce new bugs. We do run a minimal build with all flags disabled on our CI, but that is all we do to test it (that said, if you start enabling flags selectively, you are basically on our own).

drawks commented 5 months ago

I've empathy for all parties in this situation, I think the proper solution would probably be to package both a "-full" and "-minimal" version of the software and utilize Debian package meta-data fields to define a Conflicts relationship between the packages and tag them also both with a Provides for keepassxc and also add a tag Replaces: keepassxc to the -full build so that during a package upgrade an existing user would be provided the version that continues to provide the features of the package which is being upgraded/replaced while new users can choose for themselves which of the versions they'd like to install as well as providing a path for other unrelated packages to depend directly on either the -full or -minimal packages as desired.

It is generally a violation of Debian policy for an upgrade to break existing functionality it is also generally encouraged to provide builds that allow for choice and self determination for system's operators.

  1. https://www.debian.org/doc/debian-policy/ch-relationships.html

addendum to the above: I've not been actively maintaining/packaging any deb stuff in a number of years, the actual interaction of the various package relationship metadata might be slightly different than I've described above, but there should (IIRC) be a combination of metadata that will result in the behavior I've described.

TheZ3ro commented 5 months ago

For example, one that will definitely go is WITH_XC_YUBIKEY.

So in the future every installation of KeePassXC will depend on libyubikey-* (thus increasing the code, the attack surface, the possibility of supply chain attacks, etc)

The safest KeePassXC is not the one with all flags disabled ... The only real safe way to reduce the code surface is to actually remove code

Disabling those compile time flags actually removes code and reduce attack surface so yes, the safest KeePassXC is the one with less "enabled flags".

the one which is tested best by us and by the majority of our users

Distro maintainers run build tests as well, and (contrary to KeePassXC maintainers) have actual telemetry data and crash reports affecting the end users.


Having said this, I expect that the average user does want some (or all) of the so-called "plugins" and the full keepassxc experience out-of-the-box.

I think the best solution would be to provide a keepassxc-minimal package on the distro side (as suggested by @droidmonkey https://github.com/keepassxreboot/keepassxc/issues/10725#issuecomment-2104568291). On keepassxc side, instead of removing WITH_XC_* flags altogether, at least consider adding a MINIMAL flag instead to ease the compilation for such "minimal" package.

I also totally agree with @drawks proposal https://github.com/keepassxreboot/keepassxc/issues/10725#issuecomment-2104845637

nahuhh commented 5 months ago

+1 for keepass-minimal Or maybe we need to find a new maintainer.

droidmonkey commented 5 months ago

We don't depend on libyuibkey anymore since it is basically an abandoned package. The code necessary to interface with yubikey is shipped with keepassxc source. Various good reasons to do this, that code was thoroughly audited by me and is rather minimal.

Germano0 commented 5 months ago

if you upstream developers will agree on suggesting Linux distros package maintainers to provide also a minimalistic KXC version, I will make sure that it will be shipped in Fedora / EPEL

vonHabsi commented 5 months ago

Perhaps you should create your own .deb repository and publicize it?

A large number of projects distribute out of their own repository and don't seem any the worse for it.

Time to join them?

hobbes commented 5 months ago

or just acknowledge that most users use a small subset of all the possibilities of the program, and that the obvious most secure solution is to provide it ? Of course, the most flexible solution would be to make those less used possibilities dynamically loadable, like plugins, so that distribution can package them separately and offer a true customized solution to users. But in the meantime, a default minimal package that meets the requirements of most users, and another one for users who need more possibilities, seems the way to go.

rom1v commented 5 months ago

All of these features are superfluous and do not really belong in a local password database manager

@julian-klode Thank you for the change. I have been using KeePassXC for a long time, and I was not aware that all these features were present, and I strongly prefer them not to be included by default. A package keepassxc-full makes sense to me. My 2 cents.

EDIT: Update: https://github.com/keepassxreboot/keepassxc/issues/10725#issuecomment-2105083957

robsheldon commented 5 months ago

Browser auto-fill is now "crap" that doesn't belong in a password manager? That's absurd.

Browser auto-fill is a required feature of a modern password manager, to prevent users from doing all the things they resort to without it. This doesn't make KeePassXC a more secure password manager, it makes it an encrypted spreadsheet.

Speaking as someone that uses Debian and KeePassXC all day long every day and will be irritated by this decision at some point in the future.

jstorrs commented 5 months ago

The easiest solution is for KeepassXC to have the flags also function as switches that enable notifications to users about features that are missing and inventory them on the about page, etc.

Preferences related to the disabled features could be grayed out or replaced with stub notices that the features were disabled at compile time and not to file bug reports.

droidmonkey commented 5 months ago

and not to file bug reports.

What fantasy world do you live in?

I was not aware that all these features were present

Kind of the point, because they are literally disabled by default. Every feature is opt in for KeePassXC. If you don't enable it, the code never gets executed. We think long and hard before introducing a new feature. We have an extensive code review process. We aren't perfect, but we certainly put a ton of effort into delivering a secure and feature rich (if you desire) password manager.

vonHabsi commented 5 months ago

If the reasons for the new stripped Debian variant called KeepassXC is made known to end users, it will be a statement that the Debian packagers don't trust the full package, a snide comment that the full version may be full of bugs and that Debian's packagers don't consider it trustworthy.

If so then why would they the Debian packagers be offering the keepassxc-full version, or they don't intend to at all?

If the Debian packagers have good reason to believe the keepassxc-full version presents a broader attack surface, then they ought to present what they've seen that makes them feel that way, not promote baseless innuendo.

It is one thing to talk about a broader attack surface to developers or power users who have some grasp of such issues, but speaking of such issues to end users is rather snide, when there doesn't seem to be any evidence of these problems in the wild.

srd424 commented 5 months ago

or just acknowledge that most users use a small subset of all the possibilities of the program, and that the obvious most secure solution is to provide it ?

Unfortunately it won't be the same subset. For example, I use my yubikey but I don't use browser autotype. My elderly relatives, the other way around.

timw4mail commented 5 months ago

I'm afraid that's not going to happen. It was a mistake to ship with all plugins built by default. This will be painful for a year as users annoyingly do not read the NEWS files they should be reading but there's little that can be done about that.

Where would the user learn about those news files? I know with Gentoo they will actually show a message that there is news when syncing with the package repository.

It is our responsibility to our users to provide them the most secure option possible as the default. All of these features are superfluous and do not really belong in a local password database manager, these developments are all utterly misguided.

I don't understand why you would be using a GUI password manager in that case. Security is very important, but must still be balanced with usability.

Users who need this crap can install the crappy version but obviously this increases the risk of drive-by contributor attacks.

As a person who uses KeepassXC because of said 'crap', I'm not excited for the day this gets downstream of Debian into other Distros (Ubuntu, KDE Neon, etc).

khimaros commented 5 months ago

https://github.com/keepassxreboot/keepassxc/issues/10725#issuecomment-2104845637 seems like the right compromise to me. @julian-klode what is your take on this? any problems with this approach?

x86dev commented 5 months ago

I also support the decision leaving out most features which impact security, especially network or interop related features.

The ones who need these should have to explicitly install them. Keep it simple!

yllekz commented 5 months ago

I have many, many concerns about this sudden shift. So as a casual user of keepassxc, who uses the browser plugin, what happens now? When/how/where is this going to be communicated? Will the features just suddenly go away causing breakage to production? This needs to be very precisely thought out and communicated before such a drastic change is made here, including migration instructions.

Do not just push this out and tell people to "read the release notes" because this is a seismic change that warrants an airtight user experience.

Taking an absolute hatchet to the app in favor of an unproven security concern is irresponsible, in my opinion.

varjolintu commented 5 months ago

It is our responsibility to our users to provide them the most secure option possible as the default.

So.. clipboard?

robsheldon commented 5 months ago

I also support the decision leaving out most features which impact security

There's this assumption that these features in some way create some exploitable security issue.

I get the theory behind it: more code and more interoperability and more features &etc. generally leads to more bugs than the alternative, sure.

But there's also a line beyond which that principle stops making sense from a practical standpoint. Like, I could put my laptop into a furnace and melt it into slag. Now it will have no features, and that will make it very secure!

So, does anyone have any specific, demonstrable, practical exploit for these features that justifies disabling them? Has someone found a way to exploit browser auto-fill or HIBP or something in KeePassXC that I'm unaware of?

Dismissing upstream's opinion on this and unilaterally disabling these commonly-used features in this package ought to come with some kind of reasoning more specific than "it's more secure this way", IMO.

Dio9sys commented 5 months ago

Were the changes to the Debian package for keepassxc communicated to the upstream? https://www.debian.org/doc/debian-policy/ch-source.html#changes-to-the-upstream-sources

Also, are there any recent reports, writeups or security audits to show any security risks with the full build of keepass compared to the minimal version? I work in security for a living, so if there's anything I need to be aware of that would warrant turning off the majority of compile flags I would be grateful for any links.

Kwpolska commented 5 months ago

and not to file bug reports.

What fantasy world do you live in?

This might depend on the message. Something like this might deter reports: This copy of KeePassXC was built without support for browser integration. Your package was provided by "Debian". You may need to install another package or report a bug to "Debian". KeePassXC maintainers will not accept bugs related to this feature missing.

A bot that responds to all bugs that mention "Debian" and "browser" (and possibly even closes them) might also help manage the spam.

droidmonkey commented 5 months ago

@Dio9sys no, this was not communicated to us before hand, nor was there a chance to collaborate on an effective solution for both parties. There also seems to be "no going back" per Julian above. So this one way door decision is the new reality I guess. All I can say is, use Flatpak and get away from distro lock in altogether.

hobbes commented 5 months ago

or just acknowledge that most users use a small subset of all the possibilities of the program, and that the obvious most secure solution is to provide it ?

Unfortunately it won't be the same subset. For example, I use my yubikey but I don't use browser autotype. My elderly relatives, the other way around.

yep, that's why the real solution would be to make those features truly dynamic. In the absence of that, the next best thing would be to provide several versions of the package with different « most used » subsets. In the absent of that, provide the minimal most secure version, and another with all features. That doesn't seem so drastic to me: anyone can keep using the full version if they so choose, and most users get the minimal most secure one.

nekohayo commented 5 months ago

@Kwpolska If the experience with xscreensaver is any indication, I would not be surprised if Debian would patch out any such UI messages directed at Debian users, even if upstream were to somehow implement those.

Kwpolska commented 5 months ago

@nekohayo If Debian does so, then all issues mentioning Debian should be auto-closed.

hobbes commented 5 months ago

There's this assumption that these features in some way create some exploitable security issue.

nope, they create more potential for exploitable security issues. That's the very definition of an attack surface: it does not need to be exploited, just exploitable. It is good practice to reduce it, even in the absence of proved exploits.

srd424 commented 5 months ago

yep, that's why the real solution would be to make those features truly dynamic

Adding support for dynamically loadable modules is not renowned for reducing attack surface.

cgranade commented 5 months ago

From a governance and transparency perspective, it's a bit concerning that there's not really any traceability for why such a major decision was made. Was there a specific incident that prompted this change in packaging policy? Was this part of a larger audit? Is there a list of criteria that's used to determine which parts of a project are "features" and which are "crap"? How are those principles applied to (for example) browsers, arguably the largest security surface that a user actively engages with? Why is discussion about this change largely happening on a GitHub issue for the upstream project affected, and not the project that made the change?

OskarW85 commented 5 months ago

Users who need this crap can install the crappy version but obviously this increases the risk of drive-by contributor attacks.

How about not breaking existing userbase and preparing keepassxc-minimal package instead of ego-powertriping?

hobbes commented 5 months ago

yep, that's why the real solution would be to make those features truly dynamic

Adding support for dynamically loadable modules is not renowned for reducing attack surface.

it depends on the feature set. Take a cms: would you want to use a wordpress or drupal installation with each and every single module out there activated ? I don't think it would actually even be possible. But anyway, in this case, the possibility for loading functionality obviously reduces the attack surface.

I don't know where keepassxc stands on that feature scale (obviously lower than a cms :-) ), but there are functionalities that are obviously used by a very small subset of users. Some of them do expose the network. So yes, despite what many seem to think here, they are obvious attack vectors (and no, I don't know of any actual attack, that's not the point).

So instead of launching flames at one another, maybe a better solution would be to acknowledge the problem, start a discussion to determine what a minimal, standard, and full package should contain., and how to migrate existing users to that new scenario. But maybe that's too much to ask...

rom1v commented 5 months ago

Answering my own comment: https://github.com/keepassxreboot/keepassxc/issues/10725#issuecomment-2104915651

Although I think that keepassxc should be minimal by default (with secondary features disabled, causing less dependencies to be linked, to minimize the attack surface), I didn't initially realize that upstream (i.e. @droidmonkey) did not agree with this move.

Given this disagreement, replacing the existing keepassxc package to remove the existing features is probably not the right thing to do. The suggested solution with -full and -minimal looks better to me.

droidmonkey commented 5 months ago

start a discussion to determine what a minimal, standard, and full package should contain

Would have been nice to have about a month ago when this was unilaterally put into action. Alas, here we are. So yah flaming arrows will absolutely be thrown because there was no chance at proper discourse. This thread should be a lesson to downstream folks who think they know what's "best" for the user.

FWIW, all the maintainers of KeePassXC use all the features present in the software. So by that alone, you would think we care a lot about their security and functionality. We only ship code that we care about. Take one look at some of our open PRs and the amount of discourse applied to every new feature PR (remote database support is a standout).

In the lead up to this thread I received three reports of this new package method crippling people's workflow. One report was a user who couldn't open their database anymore because the yubikey feature was removed. Let that sink in for a second. People who lose access to their most important secrets can sometimes do irrational things in the moment of panic.

vonHabsi commented 5 months ago

Users who need this crap can install the crappy version but obviously this increases the risk of drive-by contributor attacks.

The arrogance here says it all. @julian-klode doesn't just want to remove the options, he insists on insulting all the users who have used them and depended on them for years.

If that is the case the proper thing to do is to remove it keepassxc entirely from the debian repos, or publish a new version under another name. It is rather like releasing a browser with Javascript support removed rather than disabled as a system's default browser, on the grounds that Javascript is crap which is not necessary for viewing HTML pages.

It is our responsibility to our users to provide them the most secure option possible as the default. All of these features are superfluous and do not really belong in a local password database manager, these developments are all utterly misguided.

Since when did these features get to be considered superfluous? I've only been using keepassxc for a few months, and I sure as hell wouldn't expect to be copying and pasting passwords from keepassxc into the browser if I had installed it a few months from now. I could easily use a plain text file to store them and copy from it instead.

Is Debian now suckless.org?

If so why not go the whole hog and take out systemd as well, after all you don't need all that crap running under pid 0? Just think of the humongous attack surface all those plugins and options create.

Trust is an essential part of life, and we generally trust others to do what they say they will do and reliably and safely so, and as IT users we put our trust in others knowing that there is a risk involved in that trust.

If that was the case @julian-klode would never even use computers, or fly in a Boeing plane, or even run a checking account because of flaws in the systems we use that are well known to hackers in those fields.

Let people decide for themselves whether the risks are worth taking and don't restrict or frustrate their choices in your bid to make them safe.

a-germain commented 5 months ago

@julian-klode

It is our responsibility to our users to provide them the most secure option possible as the default. All of these features are superfluous and do not really belong in a local password database manager, these developments are all utterly misguided.

Your main responsibility is to ship the package to users. If you're not happy with the features of a program because reasons, then fork it. Oh wait, that's what you essentially did. But you stole the name "keepassxc". Not very nice.

Users who need this crap can install the crappy version but obviously this increases the risk of drive-by contributor attacks.

When I installed the keepassxc package, (like many other users I suppose), I explicitly wanted these features. Now you're going to tell me you don't care about breaking functionality? That goes against everything I'm looking for in a distribution. I want stability. This is anything but.

BTW I wonder how much of Debian's code of conduct you're breaking just now, in this thread? Or does that simply not apply to external contributors?

robsheldon commented 5 months ago

there are functionalities that are obviously used by a very small subset of users.

Did you do some kind of survey to determine how small this subset supposedly is? I didn't receive a copy if you did; I'd be happy to fill it out.

Some of them do expose the network. So yes, despite what many seem to think here, they are obvious attack vectors (and no, I don't know of any actual attack, that's not the point).

That is absolutely the point. You state they are "obvious", but... they're so obvious that you can't describe them?

Whenever the topic of security comes up in software, there are people that wave their hands around terms like "attack surface" and "threat model" and "vector", but struggle when pressed to provide any specifics. People with more expertise in software security likewise don't have as much trouble with this; they have reasons behind their recommendations.

Security doesn't exist in a vacuum. All manner of really bad ideas have been popular over time "for security" (like forced password rotation), and have resulted in overall weakened security.

There are "obvious" security benefits to HIBP integration and browser auto-fill. Users should be automatically notified when one of their accounts has been breached and their password for that account has been found floating in a db dump. Users should rely on their password manager to handle logins for them, so they're less likely to get tricked into a phishing page.

These are actual threats that these features actually address. There needs to be stronger justification for removing them from the default package (against upstream's recommendations) than "because security".

and how to migrate existing users to that new scenario.

Indeed. Let's set up a -minimal package and those users that want to do so can migrate to that package.

aduzsardi commented 5 months ago

reading @julian-klode 's comment , it's clear he is just an egotistical asshole who thinks knows best what users want/need/do or do not do did you actually discussed it with anybody , or your personal prefference is what's important here (retoric question) ?

as many people already noted before me , you should have created a separate package (ie keepasssxc-minimal ) , but no ... you just wanted to show everybody who's boss

hobbes commented 5 months ago

Would have been nice to have about a month ago when this was unilaterally put into action

more like two weeks ago, but hey... right now it's only in debian sid, the development distribution, used for... well, development, and only by people who are supposed to know what they are doing; and a few days ago it migrated to testing, the distribution that noone should be using except people who really know what they are doing.

All these people are supposed to know where to look when something breaks, and most certainly to file bugs against debian instead of the upstream project. That's obviously a packaging issue, not an upstream one, it's even documented in the changelog, the NEWS file (which is shown during upgrades), and the bugtracker. It couldn't be made more obvious and/or documented.

People complain here ? Redirect them to debian's bug tracker without any second thought. They use development tools, they should know how to deal with them.

And in the meantime, please drop the pressure a bit :-)

droidmonkey commented 5 months ago

Wow, @hobbes, you are either oblivious to how things really work or completely disillusioned by Debian's superiority complex. Either way, here's a hint. If the "people who should know what they are doing" don't read the NEWS or know not to file upstream report, then the casual user ABSOLUTELY WILL NOT. The hand waving here is infuriating at best.

a-germain commented 5 months ago

@hobbes

People complain here ? Redirect them to debian's bug tracker without any second thought. They use development tools, they should know how to deal with them.

Indeed, discussion on this should happen on Debian's turf. One bug has already been opened, but is way too soft IMO:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1069743

BTW a heated discussion is also happening on HN:

https://news.ycombinator.com/item?id=40320166