Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Year 2038 problem #4177

Closed
zacknewman opened this issue Dec 17, 2023 · 7 comments · Fixed by #4355
Closed

Year 2038 problem #4177

zacknewman opened this issue Dec 17, 2023 · 7 comments · Fixed by #4355
Labels
enhancement New feature or request

Comments

@zacknewman
Copy link
Contributor

This is not a "vulnerability" due to the fact that the issue is 14 years away*; furthermore Vaultwarden may not even be a thing then. Anyway, I am not sure why 32-bit timestamps are used for the TOTP code considering most OSes running on 64-bit systems have been using 64-bit timestamps for a decade or more (even 32-bit Linux has support since 5.6), chrono uses i64s, and totp-lite uses u64s. The change to u64/i64s is easy in code—I've done so on my personal fork—and SQLite has no issues either since INTEGER is a flexible-width data type (up to 64 bits) and DATETIME has type affinity NUMERIC which will store values as INTEGER so long as they are valid 64-bit signed integers. I'm not sure what if anything would be needed to "fix" PostgreSQL and MariaDB/MySQL.

* Technically a contrived setup where servers and clients have their clocks in-sync but set to beyond 2038 are vulnerable.

@BlackDex
Copy link
Collaborator

If I'm correct, only the last used is stored in a i32, all other parts are 64 bit. Since 2038 is far away, i think we have time to fix this and convert this to 64bit.

Thanks for the report

@BlackDex BlackDex added the enhancement New feature or request label Dec 17, 2023
@zacknewman
Copy link
Contributor Author

zacknewman commented Dec 17, 2023

Yes, only the last used is stored/retrieved as an i32; however that is enough to invalidate the entire thing.

i think we have time to fix this and convert this to 64bit.

Lol, I agree that 14 years is enough time.

@LoiLock
Copy link

LoiLock commented Jan 12, 2024

You can have a kid in that time, only for your kid to then come back to this issue and reopen it again.

@BlackDex
Copy link
Collaborator

Why re-open it? The kid can fix it and create a PR ;)

@zacknewman
Copy link
Contributor Author

Why re-open it? The kid can fix it and create a PR ;)

The "kid" would use my fork which has it fixed, but they would use WebAuthn anyway so wouldn't matter.

I do believe that was a joke; but just in case it were not, let me state that my fork has diverged by 10s of thousands of lines if not over 100K. This means sending a git format-patch or PR would take time and not benefit me at all. I realize that's selfish, but I believe making an issue without a PR is better than not making an issue at all. I obviously benefited from the work of this project, so bug and security exploit reports are my contribution. Also note that I have offered PRs before privately and publicly that were rejected, so that also adds to the "waste of time" aspect.

@BlackDex
Copy link
Collaborator

Why re-open it? The kid can fix it and create a PR ;)

The "kid" would use my fork which has it fixed, but they would use WebAuthn anyway so wouldn't matter.

I do believe that was a joke; but just in case it were not, let me state that my fork has diverged by 10s of thousands of lines if not over 100K. This means sending a git format-patch or PR would take time and not benefit me at all. I realize that's selfish, but I believe making an issue without a PR is better than not making an issue at all. I obviously benefited from the work of this project, so bug and security exploit reports are my contribution. Also note that I have offered PRs before privately and publicly that were rejected, so that also adds to the "waste of time" aspect.

It does mean a bit more work for us, but indeed all the items you send we (I) will try to address. From my point of view I'm happy with them, any insight and approvements are welcome!

@gzfrozen
Copy link
Contributor

Hi, I create PR to fix this. Please check:
#4355

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants