search

KelvinTegelaar / AzPwPush

11 stars 10 forks source link

Password creation process may result in overwrite of existing password #6

Closed shellster closed 4 years ago

shellster commented 4 years ago

Admittedly it is unlikely, but conceivably the same random number could be chosen twice in a short period of time. In such an instance, the current code would overwrite the existing password with the new one. This could result in leaking a password to the wrong recipient. Please consider catching this case and generating a new id (also see separate issue on using GUID instead). I will be forking and adding/testing fixes for all the issues I just reported, in the near future. Thanks for this great starting point.

KelvinTegelaar commented 4 years ago

GUID resolved this, thanks for the help!

shellster commented 4 years ago

Reviewed your changes and per this (https://docs.microsoft.com/en-us/windows/win32/api/combaseapi/nf-combaseapi-cocreateguid), I agree that it is for all intents and purposes, solved. I am paranoid, so in my implementation I loop to ensure that the GUID is unique (at least as far as existing password files). GUID should also help prevent a number reuse in a short period of time, which would be relatively difficult to prevent using the prior method. Anyhow, I agree this one is fixed. Thanks for the quick update, especially on the weekend :).