Kunzisoft / KeePassDX

Lightweight vault and password manager for Android, KeePassDX allows editing encrypted data in a single file in KeePass format and fill in the forms in a secure way.
https://www.keepassdx.com/
GNU General Public License v3.0
4.78k stars 276 forks source link

URL matching fails in presence of a path #1940

Open cbiere opened 5 days ago

cbiere commented 5 days ago

Describe the bug

Autofill sugggestions are not matched to existing entries anymore, if the URL in the database includes any path after the hostname. This used to work fine until 4.0.8.

To Reproduce

Steps to reproduce the behavior:

  1. Go to https://github.com/login with any browser
  2. Click on Sign in with KeePassDX
  3. Manually search for github to select the entry

Expected behavior

Step 3 should not be necessary, if you have the login URL in your KeePass database entry.

KeePass Database

KeePassDX:

Android:

Additional context

If you change the URL to https://github.com without any path in your KeePass database or add another URL item to it (URL_1), the problem can be worked around.

This issue results from the changes to fix #1820.

cbiere commented 5 days ago

I've looked at database/src/main/java/com/kunzisoft/keepass/database/search/SearchHelper.kt and this check is really bad:

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

If I have the following URL in my KeePass database: https://www.example.com/login.sh

It will match on https://login.sh/. This isn't only wrong but if the path can be registered as a domain, it can be used for phishing. In a LAN this might even work with just a hostname (e.g. http://login/ or http://signin).

J-Jamet commented 5 days ago

I agree, it's not ideal at all. The problem, as I said, is to use an algorithm that doesn't take time to search. I'd already tried accent searching and it caused ANRs. I'm afraid it'll do the same thing with URL objects. I'll give it a try anyway.

J-Jamet commented 5 days ago

I'll add unit tests to handle all cases. If anyone sees anything missing, don't hesitate to tell me.

J-Jamet commented 5 days ago

Unit tests :

    fun testBuildURL() {
        val expected = "domain.org"

        assertTrue(expected.inTheSameDomainAs("domain.org", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("http://domain.org", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("https://domain.org", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("domain.org/login", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("http://domain.org/login", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("https://domain.org/login", sameSubDomain = false))

        assertTrue(expected.inTheSameDomainAs("https://www.domain.org", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("www.domain.org", sameSubDomain = false))
        assertTrue(expected.inTheSameDomainAs("ww.domain.org", sameSubDomain = false))

        assertTrue(expected.inTheSameDomainAs("domain.org", sameSubDomain = true))
        assertTrue(expected.inTheSameDomainAs("http://domain.org", sameSubDomain = true))
        assertTrue(expected.inTheSameDomainAs("https://domain.org", sameSubDomain = true))
        assertTrue(expected.inTheSameDomainAs("domain.org/login", sameSubDomain = true))
        assertTrue(expected.inTheSameDomainAs("http://domain.org/login", sameSubDomain = true))
        assertTrue(expected.inTheSameDomainAs("https://domain.org/login", sameSubDomain = true))

        assertFalse(expected.inTheSameDomainAs("https://www.domain.org", sameSubDomain = true))
        assertFalse(expected.inTheSameDomainAs("www.domain.org", sameSubDomain = true))
        assertFalse(expected.inTheSameDomainAs("ww.domain.org", sameSubDomain = true))

        assertFalse(expected.inTheSameDomainAs("domain.com", sameSubDomain = false))
        assertFalse(expected.inTheSameDomainAs("omain.org", sameSubDomain = false))
        assertFalse(expected.inTheSameDomainAs("odomain.org", sameSubDomain = false))
        assertFalse(expected.inTheSameDomainAs("tcp://domain.org", sameSubDomain = false))

        assertFalse(expected.inTheSameDomainAs("domain.com", sameSubDomain = true))
        assertFalse(expected.inTheSameDomainAs("omain.org", sameSubDomain = true))
        assertFalse(expected.inTheSameDomainAs("odomain.org", sameSubDomain = true))
        assertFalse(expected.inTheSameDomainAs("tcp://domain.org", sameSubDomain = true))
    }
cbiere commented 5 days ago

I don't know how to write code in Kotlin, but maybe I can provide some pseudocode:


// stringtomatch contains the URL from each entry the database // word is the hostname to search in the database

// extract the hostname from the URL hostname = stringtomatch.replace("^([a-z]+://)([^/]+)(/.*)?$", $2) // compare the extracted hostname, case-insensitive if (hostname.equals(word, true)) return true

// if enabled, check if hostname is a subdomain of word domain = "." + word if (searchsubdomain && hostname.endswith(domain) return true

// no matching URL return false

If a regular expression is too expensive, the hostname can simply be extracted by removing everything up to the first "://" sequence and then everything after the first slash of the remaining sequence.

cbiere commented 4 days ago

As for unit tests I would recommend adding the following checks:

assertFalse(expected.inTheSameDomainAs("https://example.com/domain.org", sameSubDomain = true)) assertFalse(expected.inTheSameDomainAs("https://example.com/www.domain.org", sameSubDomain = false))