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

Add milliseconds platform time function #6891

Merged

Conversation

yuhaoth
Copy link
Contributor

@yuhaoth yuhaoth commented Jan 7, 2023

Description

fix #6623.

This PR provide milliseconds platform time function to resolve ticket age issue.

The time unit of ticket age is milliseconds and the unit in current code is seconds. That causes client age is bigger that server age, it is identified as replay attack. For a final solution, we need milliseconds function.

mbedtls_ms_time is used in #6788 , that has been verified.

Gatekeeper checklist

  • changelog provided
  • backport not required
  • tests provided

@yuhaoth yuhaoth added needs-review Every commit must be reviewed by at least two team members, component-platform Portability layer and build scripts needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels Jan 7, 2023
From RFC8446, the unit of ticket age is million seconds

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
We provide windows and posix implementation for it.
With MBEDTLS_PLATFORM_MS_TIME_ALT, user can provide
their own implementation.

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
@yuhaoth yuhaoth force-pushed the pr/add-milliseconds-platform-function branch from 294829a to 3825749 Compare January 12, 2023 10:02
@yuhaoth yuhaoth force-pushed the pr/add-milliseconds-platform-function branch 2 times, most recently from e609641 to fec8ff2 Compare January 30, 2023 07:24
There's a potential race condition with calling time(NULL) after
GetSystemTime().

See
https://learn.microsoft.com/en-us/archive/msdn-magazine/2004/march/implementing-a-high-resolution-time-provider-for-windows

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
@yuhaoth yuhaoth force-pushed the pr/add-milliseconds-platform-function branch from fec8ff2 to 947fd3d Compare January 30, 2023 07:45
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
@yuhaoth yuhaoth force-pushed the pr/add-milliseconds-platform-function branch from b381988 to eb30684 Compare January 31, 2023 04:59
@yuhaoth
Copy link
Contributor Author

yuhaoth commented Jan 31, 2023

Internal CI pass and OpenCI report docker build fail

@yuhaoth
Copy link
Contributor Author

yuhaoth commented Feb 1, 2023

Now, CI passed

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
@yuhaoth yuhaoth force-pushed the pr/add-milliseconds-platform-function branch from 49ba576 to 041c8c1 Compare February 7, 2023 08:14
yuhaoth added 2 commits March 15, 2023 18:59
We need a non-settable source to avoid security issues.

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
@yuhaoth yuhaoth force-pushed the pr/add-milliseconds-platform-function branch from 884a8b0 to 02d6840 Compare March 15, 2023 11:01
@yuhaoth
Copy link
Contributor Author

yuhaoth commented Mar 15, 2023

@tom-cosgrove-arm , I just re-write the commit history due to the CI fails. Sorry for that.

Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@tom-cosgrove-arm
Copy link
Contributor

Removed needs-work label as the PR has been updated

`GetSystemTimeAsFileTime` returns 100 nano seconds elapsed time,
not 100 micro seconds.

Signed-off-by: Jerry Yu <jerry.h.yu@arm.com>
@yuhaoth
Copy link
Contributor Author

yuhaoth commented Mar 16, 2023

@tom-cosgrove-arm , just fix windows fail. Please re-review

Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@tom-cosgrove-arm
Copy link
Contributor

@daverodgman and @gilles-peskine-arm are either of you two reviewing this, or should I add needs-reviewer?

@daverodgman
Copy link
Contributor

@daverodgman and @gilles-peskine-arm are either of you two reviewing this, or should I add needs-reviewer?

I will TAL

@gilles-peskine-arm
Copy link
Contributor

I'm not reviewing.

@tom-cosgrove-arm tom-cosgrove-arm added the needs-reviewer This PR needs someone to pick it up for review label Mar 20, 2023
@yuhaoth yuhaoth removed the request for review from daverodgman March 22, 2023 02:46
@daverodgman daverodgman added approved Design and code approved - may be waiting for CI or backports needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review needs-ci Needs to pass CI tests labels Mar 29, 2023
@daverodgman daverodgman merged commit b8f5ba8 into Mbed-TLS:development Mar 31, 2023
@yuhaoth yuhaoth deleted the pr/add-milliseconds-platform-function branch December 6, 2023 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-platform Portability layer and build scripts priority-high High priority - will be reviewed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Early data test case will fail randomly cause the anti-play protection from gnutls server
4 participants