Keepassxc: 2.5.1 - Browser Confirm Access ignores subdomain specificity

[jm-duke]

I tested this behavior with the KeePassXC-Browser Addon for Firefox and Chrome. Downgrading KeepassXC from 2.5.1 to 2.5.0 fixed the issue, so it seems to be related to the latest version of KeepassXC and not to the browser addon.

Expected Behavior

When browsing to a site, the "KeePassXC-Browser Confirm Access" pop-up requests password access to the credentials stored for this specific site or does not pop-up at all for sites, for which I already granted password access.

Current Behavior

The pop-up requests access to multiple, unrelated sites.

Also, on sites that were already known and that never showed the pop-up after I ticked "Remember this decision", the pop-up requests access to other sites now.

Steps to Reproduce

1. Upgrade KeepassXC to 2.5.1
2. Browse to a site, which requests passwords stored in KeepassXC

Context

Tested on two different Windows desktops, one using Firefox + KeepassXC Browser, the other using Chrome + KeepassXC Browser

Debug Info

KeePassXC - Version 2.5.1
Revision: 0fd8836

Qt 5.13.1
Debugging mode is disabled.

Operating system: Windows 10 (10.0)
CPU architecture: x86_64
Kernel: winnt 10.0.18362

Enabled extensions:

• Auto-Type
• Browser Integration
• SSH Agent
• KeeShare (signed and unsigned sharing)
• YubiKey

Cryptographic libraries:
libgcrypt 1.8.5

[droidmonkey]

We specifically fixed this issue in 2.5.1. What do the URL's look like for the sites that are erroneously sent?

[ThinkChaos]

Since 2.5.1, it KeepassXC sends all the sibling sub-domains: if I browse to a.x.x, KeepassXC asks me if I also want to provide the browser access to credentials for b.x.x, c.x.x, etc.
In my settings, I have checked "Return only best-matching credentials".

[varjolintu]

I'll check the behaviour if there's something we've missed. Thanks for reporting it.

[droidmonkey]

Probably start with a new test case and see where things go sideways

[varjolintu]

@droidmonkey Already did it. I have the fix ready. Sadly this didn't make to 2.5.1.

[varjolintu]

The fix has been submitted. Feel free to test it.

[luchaos]

For what it's worth - the previous version already had an issue with one entry that had a space at the end of the url field. Now it asks for access to seemingly random domains.
Notably when working locally on .test domains - it will ask for other .test domains in the database.
Glad you have it on high priority :) Thank you for your fantastic work!

[varjolintu]

@luchaos Please test the PR if you can! I'd appreciate it if any extra bugs are found.

[jm-duke]

I'd be happy to test as well, but the PR is not available for download yet, is it?

[varjolintu]

@jm-duke You'll have to compile the sources yourself using https://github.com/varjolintu/keepassxc/archive/fix/browser_subdomain_matching.zip or https://github.com/varjolintu/keepassxc/tree/fix/browser_subdomain_matching via git.

[chc-pr]

@jm-duke You'll have to compile the sources yourself using https://github.com/varjolintu/keepassxc/archive/fix/browser_subdomain_matching.zip or https://github.com/varjolintu/keepassxc/tree/fix/browser_subdomain_matching via git.

I know this is a stupid Q, but I have never compiled from source a github windows file. Reading the compiling for windows instructions appears to be using a linux environment to compile (from a quick scan). Have I got that right? I don't really want to mess about with my Windows env as it is really overdue for a HW upgrade and adding yet more stuff into it will probably kill it from lack of storage space.

Sorry to ask such a newbe Q. Thanks

[varjolintu]

@chc-pr That's right. You can install everything needed via MSYS2. It's pretty simple actually. If you use git with it, everything is downloaded to the same MSYS2 root path. It's easy to clean/uninstall.

EDIT: I can also build test packages for Windows, if anyone is interested.

[chc-pr]

Thx. I'll do that ASAP, prob later today or early tomorrow (I feel I should allow a bit of time to do it without being pressured by other things) - but if you can build a Windows test package I can implement that pretty much immediately and report back. Thank you for the offer.

[jm-duke]

+1 I probably won't find the time to build stuff today, but I would definitely be able to test a build

[Morl99]

I am also affected wildly by this, because most of my entries end on db.de (our internal company dns). It leads to receiving 30ish matches on each page, KeepassXC became unusable in 2.5.1 for this.

I will try to build the patched version on my mac and let you know if this works for me. Thanks for the quick fix, I appreciate that!

[droidmonkey]

Apologies for this snafu, I blame myself for not testing this case prior to merging. The code did not look like it changed to cause this, crazy stuff.

[Morl99]

Ok I can confirm, that the above PR fixes this issue for me. I made a couple of tests opening various URLs in the browser. They all behaved correctly.
It would be great if this PR could lead to a new release quickly!

And kudos for the instructions on how to build the software. As an outsider to the project, this made it really easy for me!

[varjolintu]

The matching has been broken a long time, since the KeePassHTTP support. Tried to fix it for 2.5.1 but the subdomain matching was a miss. This time it really should work correctly. Thanks everyone for the patience. It's frustrating when something breaks.

[Morl99]

Looking at the test cases, I get the feeling that there this problem is not actually verified by unit tests yet, is that correct? I am actually not 100% sure if I understand them correctly, I am used reading BDD test cases, so this code is a little hard on my eyes.

[varjolintu]

@Morl99 The test happens here: https://github.com/keepassxreboot/keepassxc/blob/2a5feeb4fdd6da5ed278c7125ad879978891657b/tests/TestBrowser.cpp#L295

First, it's tested against various GitHub URL's, but the test only searches matches for https://github.com/, and there's only one entry URL with a match.

Second, it's tested against a common www subdomain. In that case it returns all entry URL's with that subdomain, plus an entry with https://github.com because it accepts all subdomains.

Third, there's a list of entry URL's to be tested against https://accounts.example.com. As you can see, the matches for that URL are https://accounts.example.com, https://accounts.example.com/path and https://example.com/. All other entries with different subdomains are ignored.

I hope this clarifies the test cases used here.

[droidmonkey]

~~ Shouldn't https://example.com NOT match in the last test case mentioned? This would be because the URL in the entry is less specific then the one provided. ~~

[varjolintu]

@droidmonkey Previously that has been working as a wildcard for all subdomains. Do we want to change that behaviour? This means you cannot use any "general" URL's in the entry anymore, until we have implemented a support for wildcards.

EDIT: When returning only the best matches, only the URL's with subdomain are returned, and not any of those less specific ones.

[droidmonkey]

I'm sorry you are absolutely correct. Please ignore me.

[Morl99]

Thank you @varjolintu, I had found the test class, but was unsure of the test cases. Your explanation made it clear, thanks. I am convinced and think this should be shipped as soon as possible.

[patrick7]

When will this be fixed? Or is there a workaround?
It's difficult / annoying to work with this bug.

[varjolintu]

@patrick7 It's already fixed. Just have to wait for it to be merged. Then a snapshot build can be used while waiting for a new release.

[thePanz]

@varjolintu any ETA for v2.5.2?
Or any way to get a pre-build package?

[chc-pr]

@varjolintu any ETA for v2.5.2?
Or any way to get a pre-build package?

+1

This bug is REALLY REALLY seriously annoying ...

[patrick7]

+1 to the previous 2 comments.

[varjolintu]

You can test a snapshot build from: https://snapshot.keepassxc.org/

[MaxXor]

@thePanz I agree, this bug does heavily impact the usability of the keepassxc browser extension and an update would be very important.

[tohn]

Apparently it's not yet merged in the latest snapshot :(

[varjolintu]

You can also downgrade to 2.5.0 until there's an update.

[thePanz]

@varjolintu Downgrading to 2.5.0 fixed the issue.
I tested the latest AppImage but the bug is still present.

[chc-pr]

Can someone tell me where I can find 2.5.0? I can't see it anywhere and a search of github hasn't actually resulted in my finding it ... but I am sure it must be here somewhere ..

Thx

Ignore this, I found it by accidentally forking the code ... I then saw all the releases ...

[thePanz]

@chc-pr : https://github.com/keepassxreboot/keepassxc/releases/tag/2.5.0
On ArchLinux there is n handy script called downgrade:
$ downgrade keepassxc

[Morl99]

Apparently it's not yet merged in the latest snapshot :(

I don't think that this is true, the fix for this is already merged in the 2.5.2 branch, it is just not released yet. See https://github.com/keepassxreboot/keepassxc/pull/3854

[patrick7]

When will the fixed v2.5.2 be released?
It is getting more and more annoying every day

[droidmonkey]

We are wrapping it up right now.

[patrick7]

Any news?

[varjolintu]

@patrick7 The info I have, the release could be already tomorrow.

[patrick7]

"already".

[droidmonkey]

We are releasing tomorrow.

[mariushilleke]

I am still having the issue in version 2.5.3 now, although both options checked. Also tried it with only "best matching"-option.
Specific URLs look like this:

• https:\//devtools.com/jenkins
• https:\//devtools.de/builda

I get for both URLs each both credential entries

[varjolintu]

@Django987 I'll check it out.