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

Mitigate under/over-flows in timestamp arithmetic #939

Open
2 tasks
s1fr0 opened this issue Apr 11, 2022 · 1 comment
Open
2 tasks

Mitigate under/over-flows in timestamp arithmetic #939

s1fr0 opened this issue Apr 11, 2022 · 1 comment
Labels
enhancement New feature or request

Comments

@s1fr0
Copy link
Contributor

s1fr0 commented Apr 11, 2022

Problem

Timestamps were recently refactored as Timestamp type, an alias for int64.

As already noted, the choice of using signed integers rather than unsigned integers was made in order to have safer arithmetic operations and eventually reduce bugs. This especially applies when computing elapsed time between two timestamps, where computing a difference is needed.

However, in some circumstances the source of timestamps is not trusted and an attacker may set them to specific values in order to trigger under- or over-flows errors, as discussed in #876 (comment)

This issue aims at finding mitigations to such possible timestamp under- and over-flows.

Acceptance criteria

  • Investigate possible methods to mitigate overflows for (non-trusted) timestamps
  • Implement such solutions
@s1fr0
Copy link
Contributor Author

s1fr0 commented Apr 11, 2022

Since we already introduced the Timestamp type and all timestamps are casted to this type, a quick fix may be to redefine the +,-,*,/ operators for this type so that when reaching the low or high bound of int64, we don't under/over-flow but we return the passed bound. For example:

Timestamp.low - x = Timestamp.low
Timestamp.high + x = Timestamp.high
...

In order to do this, we can cast each operand entering a binary operation to int64, perform the arithmetic safely there by checking each operand's sign, and return the result if no overflow is detected or the corresponding int64 low/high bound if an over/under-overflow is detected.

This approach avoids changing any of the currently implemented logic on timestamps nor seems to require implementing extra checks around the code-base for preventing such arithmetic overflows. Changes are required only in time.nim and eventually in all those places where arithmetic operations for timestamps are done as int64 or other types before they are actually converted to the Timestamp type.

EDIT:

A PoC implementation of this approach can be found in #943. However to successfully overload the +,- operators and avoid ambiguity with the same system operators reserved for int64, I had to declare the Timestamp type to be distinct. This has the unpleasant consequence that all procedures which previously straightforwardly treated timestamps as int64 now require to be overloaded/borrowed.

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
Status: Icebox
Development

Successfully merging a pull request may close this issue.

3 participants