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

Whitelist some methods related to java.time.Instant #242

Merged
merged 2 commits into from
May 6, 2020
Merged

Whitelist some methods related to java.time.Instant #242

merged 2 commits into from
May 6, 2020

Conversation

@jglick jglick requested review from Wadeck and daniel-beck May 23, 2019 05:37
jglick
jglick previously requested changes May 23, 2019
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently blacklist System.nanoTime and this seems equally dangerous.

@bitwiseman
Copy link
Contributor

@grooverdan Could you comment on what ways this differs from System.nanoTime?

@grooverdan
Copy link
Contributor Author

Looking at the blacklist for nanoTime it references SECURITY-683 (which I can't find/access) with references to Spectre/Meltdown.

With that in mind only staticMethod java.time.Instant now is really a problem as the other are just accessors fields/maths operator implementations. The java.time.Instant.now method, like java.time.LocalDate.now (approved in #209) uses the system time, unlike System.nanoTime which uses much more precise CPU based implementation of timing. I'd assume its implementation is less precise which is why those where implemented.

Seems mozilla folks are happy if the timing resolution is >= 20us.
https://blog.mozilla.org/security/2018/01/03/mitigations-landing-new-class-timing-attack/
As best I can tell system time is in order of microseconds
https://bugs.openjdk.java.net/browse/JDK-8068730
So its order less than what Mozilla folks are happy with (probably quite conservative in the early days of Spectre) but its out of the nanoseconds relm for which System.nanoTime was blacklisted.

@jglick jglick dismissed their stale review May 31, 2019 13:20

Will leave it to others to comment.

@daniel-beck daniel-beck removed their request for review January 16, 2020 09:52
@grooverdan
Copy link
Contributor Author

staticMethod java.time.Instant now removed

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This much seems harmless, though the PR description may now be misleading.

@dwnusbaum
Copy link
Member

Rebuilding after #293 was merged.

@dwnusbaum dwnusbaum closed this May 5, 2020
@dwnusbaum dwnusbaum reopened this May 5, 2020
@dwnusbaum dwnusbaum changed the title whitelist java.time.Instant Whitelist some methods related to java.time.Instant May 6, 2020
@dwnusbaum dwnusbaum merged commit b351c80 into jenkinsci:master May 6, 2020
@grooverdan grooverdan deleted the instant branch May 6, 2020 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants