Added time validity checking to Device Attestation Verifier #12212
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
woody-apple
approved these changes
Nov 24, 2021
|
PR #12212: Size comparison from b87d1fa to 0214b5e Increases above 0.2%:
Increases (2 builds for linux)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
emargolis
reviewed
Nov 24, 2021
emargolis
reviewed
Nov 24, 2021
tcarmelveilleux
requested changes
Nov 25, 2021
|
Ping @vijs ? |
|
PR #12212: Size comparison from 7e99454 to a0e0f0b Increases above 0.2%:
Increases (2 builds for linux)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
pullapprove
bot
requested review from
andy31415,
cecille,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
hawk248,
jelderton,
jepenven-silabs,
jmartinez-silabs and
kpschoedel
pullapprove
bot
requested review from
saurabhst,
selissia,
tecimovic,
wbschiller,
yufengwangca and
yunhanw-google
|
PR #12212: Size comparison from 7e99454 to 4ef7efd Full report (7 builds for efr32, p6, telink)
|
tcarmelveilleux
approved these changes
Nov 30, 2021
emargolis
approved these changes
Nov 30, 2021
vijs
force-pushed
the
feature/da_time_validity
branch
from
2d30fd4 to
2ee24f5
Compare
|
PR #12212: Size comparison from a5513c2 to 2ee24f5 Increases above 0.2%:
Increases (2 builds for linux)
Full report (23 builds for efr32, esp32, linux, mbed, p6, telink)
|
vijs
force-pushed
the
feature/da_time_validity
branch
from
2ee24f5 to
dddeb59
Compare
|
PR #12212: Size comparison from cea8769 to dddeb59 Increases above 0.2%:
Increases (2 builds for linux)
Full report (26 builds for efr32, esp32, linux, mbed, p6, qpg, telink)
|
vijs
force-pushed
the
feature/da_time_validity
branch
from
dddeb59 to
cb3698e
Compare
|
PR #12212: Size comparison from 7cba51d to cb3698e Increases above 0.2%:
Increases (2 builds for linux)
Full report (39 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
hawk248
approved these changes
Dec 3, 2021
This was referenced Dec 3, 2021
billwatersiii
pushed a commit
to billwatersiii/connectedhomeip
that referenced
this pull request
Dec 3, 2021
…chip#12212) * Added time validity checking to Device Attestation Verifier * Added new methods to validate a certificate and implemented review comments * Renamed IsCertificateValid to IsCertificateValidAtCurrentTime * Removed "invalid issuing timestamp" test * Disabled cert validation test in certain platforms
Emill
reviewed
Mar 4, 2024
Comment on lines
+1348
to
+1354
| // check if certificate's notBefore timestamp is earlier than or equal to current time. | ||
| result = mbedtls_x509_time_is_past(&mbedCertificate.valid_from); | ||
| VerifyOrExit(result == 1, error = CHIP_ERROR_CERT_EXPIRED); | ||
|
|
||
| // check if certificate's notAfter timestamp is later than current time. | ||
| result = mbedtls_x509_time_is_future(&mbedCertificate.valid_to); | ||
| VerifyOrExit(result == 1, error = CHIP_ERROR_CERT_EXPIRED); |
There was a problem hiding this comment.
The usage of mbedtls_x509_time_is_past and mbedtls_x509_time_is_past are incorrect.
E.g. for mbedtls_x509_time_is_past, the documentation says:
* \note Intended usage is "if( is_past( valid_to ) ) ERROR".
* Hence the return value of 1 if on internal errors.
...
* \return 1 if the given time is in the past or an error occurred,
* 0 otherwise.
Therefore, if an error occurred, we accept the validity.
Correct should be something like this:
// check if certificate's notBefore timestamp is earlier than or equal to current time.
result = mbedtls_x509_time_is_future(&mbedCertificate.CHIP_CRYPTO_PAL_PRIVATE_X509(valid_from));
VerifyOrExit(result == 0, error = CHIP_ERROR_CERT_EXPIRED);
// check if certificate's notAfter timestamp is later than current time.
result = mbedtls_x509_time_is_past(&mbedCertificate.CHIP_CRYPTO_PAL_PRIVATE_X509(valid_to));
VerifyOrExit(result == 0, error = CHIP_ERROR_CERT_EXPIRED);
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Change overview
Testing