Coverity times #19004
Coverity times #19004
Conversation
Fixes a bug in the cookie code which would have caused problems for ten minutes before and after the lower 32 bits of time_t rolled over.
Avoid a problem when the lower 32 bits of time_t roll over by delaying the cast to integer until after the time delta has been computed.
Avoid a problem when the lower 32 bits of time_t roll over by delaying the cast to integer until after the time delta has been computed.
paulidale
added
branch: master
There was a problem hiding this comment.
It's interesting how little overlap there is with #17701, #17786, or #17778.
This does introduce an incompatible change in the cookie format, so cookies received across a software upgrade would not be accepted. This is probably uninteresting, given how cookies are used (whereas for session tickets it would be a bigger deal!), but I wanted to confirm that you'd thought about it before hitting 'approve'.
|
This isn't attempting to fix all of the y2038 problems but it should help with some. There will be more required 😢 I did dig into the cookies a bit and I think it's okay except, as you noted, across software upgrades. Is this better or worse than rejecting cookies issued during the window of failure? I think it's better, but it's not clear cut. A move to OSSL_TIME would likely break cookies again, assuming that that is stored in the cookie instead of the time_t value. |
There was a problem hiding this comment.
Looking at the cookie usage a bit more, you do need to update MAX_COOKIE_SIZE and the associated comment.
Bumping COOKIE_STATE_FORMAT_VERSION would alleviate basically all of my (nascent) concerns. I think it would mean dropping connection attempts across the software upgrade, but the alternative is a lot harder to analyze and ensure the safety of.
|
Okay, made those changes. Thanks for the suggestion. |
kaduk
removed
the
approval: review pending
mattcaswell
added
approval: done
openssl-machine
removed
the
approval: done
|
This pull request is ready to merge |
openssl-machine
added
the
approval: ready to merge
Coverity 1508506:
Fixes a bug in the cookie code which would have caused problems for
ten minutes before and after the lower 32 bits of time_t rolled over.
Coverity 1508534 & 1508540:
Avoid problems when the lower 32 bits of time_t roll over by delaying
the cast to integer until after the time delta has been computed.
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #19004)
|
Merged to master. |
Coverity 1508506:
Fixes a bug in the cookie code which would have caused problems for
ten minutes before and after the lower 32 bits of time_t rolled over.
Coverity 1508534 & 1508540:
Avoid problems when the lower 32 bits of time_t roll over by delaying
the cast to integer until after the time delta has been computed.
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#19004)
Various TLS time_t use fixes for problems that will happen when the low 32 bits of time_t rolls over.
I've flagged this for all supported branches but it might be better to refactor master to use OSSL_TIME instead of persisting with time_t. That's too much change for 3.0 and 1.1.1.