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

Matching against domain goes too far #1820

Closed
cbiere opened this issue May 9, 2024 · 11 comments
Closed

Matching against domain goes too far #1820

cbiere opened this issue May 9, 2024 · 11 comments

Comments

@cbiere
Copy link

cbiere commented May 9, 2024

When the setting "Subdomain search" is disabled, entries are suggested that match only the last part of the domain name.

To Reproduce

Steps to reproduce the behavior:

  1. Add an entry to the database with the URL https://box.com/
  2. Add an entry to the database with the URL https://dropbox.com/
  3. Browse to https://account.box.com/login
  4. Enter a email address as username
  5. Use autofill with KeePassDX and see that both dropbox and box are suggested, even though dropbox.com should clearly not match box.com and this is even on account.box.com.

Expected behavior

An entry with https://box.com might match any subdomain like example.box.com but not a domain that just has box.com at the end. While enabling the "Subdomain search" setting prevents this from happening, there should be no match because these domains are completely unrelated and it only benefits phishing.

KeePass Database

Irrelevant.

KeePassDX:

  • Version: 4.0.6
  • Build: Free
  • Language: en

Android:

  • Device: sweet
  • Version: 14
@cbiere cbiere added the bug label May 9, 2024
@SuperITMan
Copy link

Same issue here, the autofill provides wrong entries for the following examples :

  • 192.168.0.1:
    • 192.168.0.10 entries
    • 192.168.0.11 entries
    • 192.168.0.1 entries
  • mydomain.com:
    • sub1.mydomain.com entries
    • sub2.mydomain.com entries
    • mydomain.com entries

@J-Jamet J-Jamet added this to 4.1.0 May 11, 2024
@cbiere
Copy link
Author

cbiere commented May 11, 2024

Same issue here, the autofill provides wrong entries for the following examples :

192.168.0.1:
192.168.0.10 entries
192.168.0.11 entries
192.168.0.1 entries

Yeah, this looks wrong and even more odd. I wonder if there is a specific logic for matching IP addresses to cause these results.

  • mydomain.com:

    • sub1.mydomain.com entries
    • sub2.mydomain.com entries
    • mydomain.com entries

This is okay if you don't have the option "Subdomain search" enabled. The idea behind this is presumably that many domains use the same credentials on different subdomains like www. or login., for example. The problem is that it also matches sub1mydomain.com which is not a sub-domain of mydomain at all.

@J-Jamet J-Jamet moved this to Todo in 4.1.0 May 13, 2024
@J-Jamet J-Jamet moved this from Todo to In Progress in 4.1.0 May 15, 2024
@J-Jamet J-Jamet removed this from 4.1.0 Nov 2, 2024
@J-Jamet J-Jamet added this to 4.2.0 Nov 2, 2024
@J-Jamet J-Jamet moved this to Todo in 4.2.0 Nov 2, 2024
@J-Jamet
Copy link
Member

J-Jamet commented Nov 8, 2024

A new algorithm will be implemented in version 4.1.0.
Same as #1105

@J-Jamet J-Jamet removed this from 4.2.0 Nov 8, 2024
@J-Jamet J-Jamet added this to 4.1.0 Nov 8, 2024
@J-Jamet J-Jamet moved this to Done in 4.1.0 Nov 8, 2024
J-Jamet added a commit that referenced this issue Nov 11, 2024
@J-Jamet J-Jamet closed this as completed Nov 18, 2024
@cbiere
Copy link
Author

cbiere commented Nov 19, 2024

This issue is not fixed. If anything it is now worse than before. Ever since I found this issue I had enabled "subdomain search" in the app settings. After updating to 4.1.0 I have to select each entry manually. This even applies to the login on github.com, so I wonder how this was tested, if it was tested at all.

@J-Jamet
Copy link
Member

J-Jamet commented Nov 19, 2024

You're right, I closed it too quickly. I thought the #1105 resolution would solve this problem too, but it's not exactly the same because there's no subdomain here.

@J-Jamet
Copy link
Member

J-Jamet commented Nov 19, 2024

I've just figured out what's wrong, I've tested it but I don't know why, the SearchHelper line contains \\ that have been added line

stringToCheck.endsWith("\\/$word", !searchParameters.caseSensitive)

Maybe an error of inattention before making the commit but when I remove them it works fine.

@J-Jamet J-Jamet added this to 4.2.0 Nov 19, 2024
@J-Jamet J-Jamet moved this to Done in 4.2.0 Nov 19, 2024
@J-Jamet J-Jamet closed this as completed by moving to Done in 4.2.0 Nov 19, 2024
@J-Jamet J-Jamet reopened this Nov 19, 2024
@J-Jamet J-Jamet removed this from 4.2.0 Nov 24, 2024
@J-Jamet J-Jamet added this to 4.1.1 Nov 24, 2024
@J-Jamet J-Jamet moved this to Done in 4.1.1 Nov 24, 2024
@J-Jamet J-Jamet closed this as completed by moving to Done in 4.1.1 Nov 24, 2024
@cbiere
Copy link
Author

cbiere commented Nov 25, 2024

I just installed 4.1.1 and the login on github.com doesn't suggest the existing entry at all regardless of the setting "subdomain search". The matching of the URLs to the login domain still seems to be broken.

@J-Jamet
Copy link
Member

J-Jamet commented Nov 25, 2024

I've just done the test again and there's no problem. There is no sub-domain to the github login https://github.com/login , I have two entries with URL https://github.com and github.com and both are recognized.

@J-Jamet
Copy link
Member

J-Jamet commented Nov 25, 2024

I also made sure that it worked properly with https://dropbox.com/ and https://account.box.com/login

@cbiere
Copy link
Author

cbiere commented Nov 25, 2024

I created a test database to verify this. The github entry is picked up, if I remove the trailing slash:
Screenshot_20241125-080248

As soon as the trailing slash is present or the full URL to the login form – as I prefer, the entry is not picked up anymore:
Screenshot_20241125-082722

This used to work just fine.

@J-Jamet
Copy link
Member

J-Jamet commented Nov 25, 2024

Indeed, but that's another problem. The resolution of the current issue respects the constraints you put in the first description.

Can you open another dedicated issue for this? Currently the recognition system only does basic string checks without real URL formatting, I have to transform the string in this case and I'm afraid it will slow down the search for many entries. I'll think about it and try to find a solution to also recognize IPs and port numbers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Status: Done
Development

No branches or pull requests

3 participants