mihaifm / HIBPOfflineCheck

Keepass plugin that performs offline and online checks against HaveIBeenPwned passwords
Other
317 stars 17 forks source link

Online check mode has no effect. Always results in `Secure`, even for known breached entries. #48

Closed Grocel closed 1 year ago

Grocel commented 1 year ago

The online check mode doesn't seem to work as intended. It always results in Secure, even for known breached entries. The offline mode is not affected and works fine. This happens for global checks and also for single entry checks. Upon check a process bar appears and it runs. It takes about 1 to 5 seconds per entry to process.

What I got: Online checked entries always results in Secure, even known bad ones.

What I expect: Online checked entries always resulting in an accurate status, e.g. Pwned for breached entries.

How to reproduce: Create an entry with a known bad password e.g. testtest or 123456789 and run the check on it. Compare it with the result of the offline mode if necessary.

mihaifm commented 1 year ago

That's weird. Are you behind a proxy by any chance?

Can you try querying the API directly and see what you get?

curl https://api.pwnedpasswords.com/range/4f0a1
Grocel commented 1 year ago

No, I do not use a proxy. I would also recommend to not return Secure on network/connection failure. Maybe leave the field unchanged or clear it in this case, if you can't output the error.

Output: curl_output.txt

mihaifm commented 1 year ago

Actually the Secure status is set after the connection has been established, so if the hash is not found in the API response, status remains Secure. You can chechk the code here:

https://github.com/mihaifm/HIBPOfflineCheck/blob/a3c5b54939a682133f2bf0f7d83d9394c2ee8721/HIBPOfflineCheck/HIBPOfflineColumnProv.cs#L111

Your output also looks good, so it might not be a connection issue.

Maybe it’s some kind of newline issue. Are you on Linux/Mac?

Grocel commented 1 year ago

I'm on Windows 10. Version 10.0.19045.2364

Is Environment.NewLine the right choice for the API text?

How does it work that you actually don't need an API for the request? Isn't there an API key requirement at HIBP nowadays? It costs at least $3.50 a month. Is it some kind of reduced/outdated end point?

Other suggestions, because I saw the code:

mihaifm commented 1 year ago

There seems to be bug in there, indeed. I made a mistake during the previous fix

 if (line.Length < pwdSha.Length)

Needs to be

if (truncatedSha.Length + line.Length < pwdSha.Length)

Oddly enough the issue only replicates for passwords with low breach count, say t3stt3st. If the breach count has a lot of digits, that condition above does not trigger. That's why I couldn't replicate it for testtest or 123456789.

Could you test the fix with the attached dll ?

HIBPOfflineCheck.zip

The newlines in the response should not be a problem, even if we're on Linux. Regarding the API, it's certainly not outdated, only the end point for checking emails is paid and needs a key, the one we're using is for checking the breach count and is free.

Grocel commented 1 year ago

Because of my security policies I have to follow I can't test the file for you, I'm sorry.

Interesting that you couldn't reproduce the issue. I was able to reproduce the issue with testtest and 123456789. Are you sure you actually tested the online mode at your end?

mihaifm commented 1 year ago

Ok, no worries. Yep, tested Online mode.

We need to keep investigating the problem you’re having..but I’m out of ideas…let me know if you can think of anything else.

Grocel commented 1 year ago

I would like to recommend to commit your changes (if it actually fixed for you) and make an official release. I would report back if it is fixed for me as well.

mihaifm commented 1 year ago

Done. New release published.

Grocel commented 1 year ago

It works for me too now. Thank you. 👍

mihaifm commented 1 year ago

Cool. Thanks for getting involved in solving this!