mihaifm / HIBPOfflineCheck

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

HIBP values revert back to previous values when Saving the Keepass data #15

Closed jeff9315 closed 5 years ago

jeff9315 commented 5 years ago

Using Keepass 2.41 and HIBP 1.3.1 ... I just noticed this today and I checked it under 2.40 and got the same behavior ... but I don't know how far back it goes.

When I run HIBP Offline, it updates all the selected entries just fine ... but when I do a Save, the column reverts back to the previous values. The same happens if I select Clear pwned status.

I wanted to make sure that somehow the KeePass data file might be write protected, but other data saves OK.

The easiest way to test this is to clear the status and then Save. It'll revert back. Or you can clear the status, change the values in the options from SECURE and PWNED to something else and then run HIBP offline and then save.

mihaifm commented 5 years ago

weird...let me update my Keepass installation and check this

jeff9315 commented 5 years ago

Another piece of info that might help you debug this ... I changed the column name to "HIBP Offline" since I also use the "HaveIBeenPwned" extension and didn't want to get the 2 confused.

jeff9315 commented 5 years ago

I just noticed that if I edit an entry and save it, the new value of the HIBP gets saved correctly. It's only when I select all the entries and perform the action from the right-click > Selected Entries > Have I Been Pwned and then hit Save that the data reverts back to the previous value. And that's regardless of whether I select 1 entry or all the entries.

mihaifm commented 5 years ago

Hi, I've been trying to replicate this, without success. It works fine for me.

When switching to the new column, did you select the column from the "Provided by plugins" section? If you selected if from "Custom Fields" it might cause some problems

image

jeff9315 commented 5 years ago

I continue to have the problem. However, I also noticed that SOME of my entries retain their HIBP values.

After investing this, it seems that if I just OPEN an entry, it causes the password field to be marked as changed which shows the asterisk indicating the keepass data is dirty and needs to be saved. I then do a SAVE and it retains the HIBP value for that entry (losing the values for any entries in which I have NOT edited the password field). Notice that even if all you do is open a Keepass entry that Keepass is marked as dirty meaning a save is required.

The easiest way to test this is to save your data, exit Keepass completely, remove the plugin (by renaming the extension, and restart Keepass and verify that the plugin is NOT loaded. Then add a new entry to KeePass and make the password (123456 or password or admin or something that you know has been pwned). Then Save it. Then exit KeePass, enable the plugin, select the enty you just added, run HIBP (verify the value in the HIBP column, then SAVE Keepass. If your results are the same as mine, Keepass will now lose the value of the HIBP column for that entry.

I even removed all other plugins to verify it wasn't some screwy interaction.

jeff9315 commented 5 years ago

Has anyone else tried to duplicate my test scenario so that Mihai and I know whether the problem is just me?

jeff9315 commented 5 years ago

@mihaifm ... Have you been able to reproduce my issue using the steps I documented on 1/27?

Thanks ... Jeff

mihaifm commented 5 years ago

@jeff9315 I tried your steps again but it still doesn't replicate. Not sure what may cause this, perhaps you can try again on a fresh database. I wouldn't be surprised if it's filesystem access righs or anything like that.

Regarding the dirty flag, it does indeed appear if you open an entry and press OK, since the plugin re-checks the status and saves it in the database. I'll open another issue for that.

I'm also thinking for a solution to clean up existing data automatically when changing the column name.

jeff9315 commented 5 years ago

OK @mihaifm ... I figured out WHAT is causing the problem but not WHY. I have a trigger that synchronizes with another database. The other database is in a subdirectory under my main Keepass data directory. If I disable that synchronization trigger, HIBPOffline works fine.

The thing is ... even with my trigger Enabled, any other changes I make work fine. They get saved and synchronized. And after I make that other change, then the entry's HIBP gets saved from then on out. It's almost like something got corrupted on EVERY entry at some point in the past and updating the Notes field or a password is enough to fix it and then HIBPOffline works for that entry after that.

jeff9315 commented 5 years ago

@mihaifm ... A little more info ... The Pwned Status reverts back even if I do a manual synchronization. HOWEVER, if I open the entry (which causes HIBPOffline to check the password since I have that option checked), the value is retained even after a synchronization.

So to summarize, synchronization FAILS when doing an offline or online check or clearing the Pwned Status (through the Selected Entries) but succeeds when I open an entry causing HIBPOffline to check the password because of my HIBP Option to check when editing the password field.

So, bottom line seems to me that Pwned Status reverts back after a synchronization when done though the Selected Entries menu but succeeds when done through opening an entry. Does this give you any clues or is this something I should report to Dominick?

mihaifm commented 5 years ago

Thanks for the update, I managed to replicate the issue.

Indeed it's a weird scenario that involves changing the column name and checking the password only via the right click menu, then synchronizing with a copy of the database.

I'll investigate.

mihaifm commented 5 years ago

I made a fix for this. Can you help testing it with the attached plgx?

HIBPOfflineCheck.zip

The problem was that changing the HIBP status did not always mark the entry as modified, which confused the synchronization algorithm which uses Last Modification Time to find out which entry was newer.

jeff9315 commented 5 years ago

YES!!!! That fixed it! Thanks @mihaifm for your perseverance in identifying the problem.