- πΊπΈUnited States cmlara
Postponing on π Public Followup for 8.x-1.3 security release. Postponed
- Status changed to Needs work
about 1 year ago 5:39am 23 November 2023 - πΊπΈUnited States cmlara
For 2.x this issue can be simplified, π Remove usage of alreadyAcceptedCode()/storeAccepedCode() in the TOTP,HOTP, Recovery Code plugins. Active will be removing the storage of codes for TFA included plugins and will do a single run cleanup which leaves just the Browser cron job which is already present in this patch.
I'm not sure we would want to backport any cleanup to to 8.x-1.x since that would change the data stored in the backend and the API status of the data is somewhat questionable (nothing says the plugins are or are not API.)
For now this just needs to focus on the TrustedBrowser table.
A few concerns I see are:
- The code is a 30 day limit while the limit is now admin configurable
- I'm a bit concerned that since this is done as a cron and not in a batch its possible to hit a timeout.
- I'm on the fence about the call to the database to get user ID's. While this reasonably should be stable it is also very much off API, however the alternative of calling UserData::get('tfa', NULL, 'tfa_trusted_browser') causes me to worry about excessive memory usage (might be wise to do some math on this)
- πΊπΈUnited States cmlara
π Remove usage of alreadyAcceptedCode()/storeAccepedCode() in the TOTP,HOTP, Recovery Code plugins. Active will not be able to cleanup accepted codes.
We do not store what plugin a code was generated for (this also means we have a risk of FP on hashes too) as such we can't know what records can be purged. Opened π storeAcceptedCode()/alreadyAcceptedCode should check token id. Active .