Gracefully handle bad validity dates in public_key:pkix_path_validation #7515

andreasanta · 2023-07-26T17:13:38Z

If a certificate contains an invalid date in the validity attribute, the function public_key:pkix_path_validation raises a function_clause error due to the invocation of calendar:date_to_gregorian_days with an invalid date.

In the attached unit test, the leaf certificate contains the invalid date string "19000100000000Z", and this would entirely fail the validation of the certificate chain.

In this PR, I am handling this edge case by delegating the decision to verify_fun and thus the caller. If verify_fun traps the error and decides to continue, validation is not interrupted, otherwise the function_clause error is raised.

This is an example error reproduced locally with the attached certificate:

{function_clause,
                     [{calendar,date_to_gregorian_days,
                          [1900,1,0],
                          [{file,"calendar.erl"},{line,132}]},
                      {calendar,datetime_to_gregorian_seconds,1,
                          [{file,"calendar.erl"},{line,155}]},
                      {pubkey_cert,validate_time,3,
                          [{file,"pubkey_cert.erl"},{line,147}]},

NOTE: I am aware invalid dates should never be passed to this function, but I am dealing with a hardware implementation that cannot be easily changed, thus I have to permit a more lenient parsing and recovery from error. Also, I do not believe a function_clause error is the right response, since it's too generic and cannot be handled accordingly or interpreted by the caller.

CC @IngelaAndin @potatosalad for visibility

CLAassistant · 2023-07-26T17:13:43Z

All committers have signed the CLA.

github-actions · 2023-07-26T17:14:42Z

CT Test Results

No tests were run for this PR. This is either because the build failed, or the PR is based on a branch without GH actions tests configured.

Results for commit 97d4036

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

lib/public_key/src/pubkey_cert.erl

IngelaAndin · 2023-08-01T08:04:23Z

I am not very fond of these kind of workarounds, but we have been forced to be pragmatic in other cases too, and it seems harmless enough solution. It is vacation time now so I would like to wait a little for final inclusion decision so that we can have a team review. And we would also need to add the new error to the documentation.

andreasanta · 2023-08-06T23:40:44Z

Thanks @IngelaAndin - To be honest I would have also gladly glossed over this edge case, but we are getting plenty of hardware certificates not abiding to the RFCs and we do not have many alternatives.

Please let me know if you would like me to add the documentation commit to the PR as well.

dgud · 2023-08-16T09:24:26Z

lib/public_key/test/public_key_SUITE.erl

+    % First test exception throwing without validation function
+    try
+        public_key:pkix_path_validation(Root, CertificateChain, [])
+    catch
+        % We expect this function to throw if the date is wrong
+        error:function_clause ->
+            ok;
+        _ ->
+            throw("Expected function_clause error on bad date")
+    end,


This is wrong:

Replace try catch with:
{error, {bad_cert, invalid_validity_dates}} = public_key:pkix_path_validation(Root, CertificateChain, []),

This is the current behaviour if no verify fun is supplied. Why would you like to change it?

Since the test will be merged along with your changes there is no reason to test the behavior in the old code.

You shall test the new functionality, i.e. that {error, {bad_cert, invalid_validity_dates}} is returned if no verify_fun is provided.

@andreasanta what about this one?

FYI: This code does not seem to verify what you probably intended (error:function_clause catch part is skipped, testcase does not notice).

@dgud This diff does not alter the old functionality if you don't provide verify_fun. This is why I am leaving the testcase. In order to preserve backwards compatibility, I am still raising an error if no verify_fun is specified.

@u3s Sorry maybe I did not explain my diff correctly. The previous behaviour has not been altered on purpose. You can optionally alter if if you pass a verify_fun that intercepts the validation error. This testcase exists to ensure that the previous behaviour - i.e. throwing an error upon failed validation - still happens if you don't specify a verify_fun.

Please let me know if you'd like a different approach.

You have altered the functionality if no verify_fun is used and you fail to test it.

I do not see a reason to preserve the crash behavior for something that should not really happen. I think this PR is about adding a workaround | graceful handling of that it sometimes does due to mistakes that are out of users control.

@IngelaAndin Refactored as suggested, plus some other improvements, to improve readability.

You have altered the functionality if no verify_fun is used and you fail to test it.

Sorry I do not understand it. I have written two tests, one that lets the code crash as expected, the other prevents the crash by supplying a custom verify_fun. What did I miss?

IngelaAndin · 2023-08-16T10:20:22Z

Yes we want you to add a documentation commit where the invalid_validity_dates are added to the possible bad_cert_reason() type.

IngelaAndin · 2023-08-16T10:27:49Z

Also if you base your patch on maint instead of master we can include it for OTP-26.1 instead of waiting for OTP-27. This I think should be ok as it does not really change any API behavior (only adds an error reason) and it is unlikely to change any existing verify_fun behavior but rather expands the workaround possibilities for the verify_fun, which is one of its uses although the main purpose is to be able to handle application specific extensions. But that is up to you.

IngelaAndin · 2023-08-16T12:40:20Z

@andreasanta and adding the bad_cert_reason() to the documentation is actually adding it to the spec in public_key.erl

lib/public_key/src/pubkey_cert.erl

andreasanta · 2023-08-23T17:28:19Z

@IngelaAndin All done. Addressed comments, rebased on maint and added docs. Please confirm if ok.

u3s · 2023-09-05T09:06:19Z

lib/public_key/src/pubkey_cert.erl

+        NewState ->
+            NewState


I have doubts about this catch everything clause.
Previous code didn't work that way.

Is there a reason for it?

No reason, preventing crash in case of unexpected value. I can remove it if necessary.

I actually think that the last case is the case that the catch verify_fun returns a state. I think the following code could benefit readablility:

validate_time(OtpCert, UserState, VerifyFun) -> TBSCert = OtpCert#'OTPCertificate'.tbsCertificate, {'Validity', NotBeforeStr, NotAfterStr} = TBSCert#'OTPTBSCertificate'.validity, Now = calendar:datetime_to_gregorian_seconds(calendar:universal_time()), case valid_dates(NotBeforeStr, NotAfterStr}) of {true, {NotBefore, NotAfter}} -> case ((NotBefore =< Now) and (Now =< NotAfter)) of true -> UserState; false -> verify_fun(OtpCert, {bad_cert, cert_expired}, UserState, VerifyFun) end; {false, Error} -> verify_fun(OtpCert, {bad_cert, Error}, UserState, VerifyFun) end. valid_dates(NotBeforeStr, NotAfterStr) -> try NotBefore = time_str_2_gregorian_sec(notBefore, NotBeforeStr), NotAfter = time_str_2_gregorian_sec(notAfter, NotAfterStr), {true, {NotBefore, NotAfter}} catch error:function_clause -> {false, invalid_validity_dates} end.

This could be related to comment on tests!

@IngelaAndin ah yes thank you for flagging this! Would you like me to refactor as suggested?

lib/public_key/test/public_key_SUITE.erl

IngelaAndin · 2023-10-24T06:22:14Z

lib/public_key/src/pubkey_cert.erl

+    % Parse and check validity of the certificate dates, and if it fails, invoke `verify_fun` to
+    % hand over control to the caller in order to decide what to do.
+    case parse_and_check_validity_dates(OtpCert) of
+


@adresanta I do not think we need these empty lines between the case clauses. No other cases in the application are written like that. Otherwise I think this is now looking good.

IngelaAndin

We should adhere to coding style of the application. Otherwise I think it looks good :)

andreasanta · 2023-10-31T18:35:24Z

We should adhere to coding style of the application. Otherwise I think it looks good :)

Done away with the spacing within case clauses, thank you.

IngelaAndin · 2023-11-01T11:04:42Z

So last testing round, should be able to merge tomorrow.

IngelaAndin · 2023-11-03T07:27:22Z

@andreasanta some other changes has created a merge conflict, could you please rebase your branch on latest maint and resolve it so that we can use the merge button,

u3s · 2023-11-03T08:00:18Z

lib/public_key/src/public_key.erl

@@ -160,7 +160,7 @@
 -type oid()                  :: tuple().
 -type cert_id()              :: {SerialNr::integer(), issuer_name()} .
 -type issuer_name()          :: {rdnSequence,[[#'AttributeTypeAndValue'{}]]} .
-type bad_cert_reason()      :: cert_expired | invalid_issuer | invalid_signature | name_not_permitted | missing_basic_constraint | invalid_key_usage | duplicate_cert_in_path | {revoked, crl_reason()} | atom().
+-type bad_cert_reason()      :: cert_expired | invalid_issuer | invalid_signature | name_not_permitted | missing_basic_constraint | invalid_key_usage | duplicate_cert_in_path | {revoked, crl_reason()} | invalid_validity_dates | atom().


line above 200 chars. break line please, so there is =< 100 chars per line.

u3s · 2023-11-03T08:59:57Z

lib/public_key/test/public_key_SUITE.erl

+pkix_path_validation_bad_date() ->
+    [{doc, "Ensure bad date formats in `validity` are handled gracefully by verify fun"}].
+pkix_path_validation_bad_date(Config) when is_list(Config) ->
+


please fix trailing whitespaces (marked red on screenshot)

u3s · 2023-11-03T09:03:27Z

lib/public_key/src/pubkey_cert.erl

@@ -140,19 +140,47 @@ prepare_for_next_cert(OtpCert, ValidationState = #path_validation_state{
 %% current time.
 %%--------------------------------------------------------------------	  
 validate_time(OtpCert, UserState, VerifyFun) ->
+


please stick to convention in module and don't add empty line after ->

lib/public_key/test/public_key_SUITE.erl

IngelaAndin · 2023-11-07T09:50:25Z

@andreasanta ping

andreasanta · 2023-11-14T18:13:39Z

Rebased and adapted to style reqs. Thank you.

IngelaAndin · 2023-11-15T06:58:01Z

@andreasanta Thanks, need one more test day (night) for sanity check of the rebase and small adjustments and then we should finally be able to merge.

potatosalad reviewed Jul 26, 2023

View reviewed changes

lib/public_key/src/pubkey_cert.erl Show resolved Hide resolved

lib/public_key/src/pubkey_cert.erl Outdated Show resolved Hide resolved

andreasanta force-pushed the pk_bad_date branch from 516f127 to 091adec Compare July 26, 2023 23:49

jhogberg added the team:PS Assigned to OTP team PS label Jul 31, 2023

IngelaAndin self-assigned this Aug 1, 2023

dgud reviewed Aug 16, 2023

View reviewed changes

u3s reviewed Aug 16, 2023

View reviewed changes

lib/public_key/src/pubkey_cert.erl Outdated Show resolved Hide resolved

lib/public_key/src/pubkey_cert.erl Outdated Show resolved Hide resolved

lib/public_key/src/pubkey_cert.erl Outdated Show resolved Hide resolved

andreasanta changed the base branch from master to maint August 23, 2023 16:45

andreasanta changed the base branch from maint to master August 23, 2023 16:47

andreasanta force-pushed the pk_bad_date branch from 091adec to 6900b58 Compare August 23, 2023 17:25

andreasanta changed the base branch from master to maint August 23, 2023 17:25

andreasanta force-pushed the pk_bad_date branch from 6900b58 to bffa3d4 Compare August 23, 2023 17:27

andreasanta force-pushed the pk_bad_date branch 4 times, most recently from 6af8068 to e9adb6e Compare August 23, 2023 17:57

IngelaAndin added the testing currently being tested, tag is used by OTP internal CI label Aug 25, 2023

IngelaAndin requested review from IngelaAndin, u3s and dgud August 30, 2023 12:29

u3s removed the testing currently being tested, tag is used by OTP internal CI label Sep 5, 2023

u3s reviewed Sep 5, 2023

View reviewed changes

IngelaAndin added the waiting waiting for changes/input from author label Sep 18, 2023

andreasanta commented Oct 22, 2023

View reviewed changes

lib/public_key/test/public_key_SUITE.erl Show resolved Hide resolved

andreasanta force-pushed the pk_bad_date branch 2 times, most recently from ea7b500 to 0e656af Compare October 22, 2023 19:34

IngelaAndin reviewed Oct 24, 2023

View reviewed changes

IngelaAndin added testing currently being tested, tag is used by OTP internal CI and removed waiting waiting for changes/input from author labels Oct 24, 2023

IngelaAndin requested review from u3s and IngelaAndin October 24, 2023 12:29

IngelaAndin added the waiting waiting for changes/input from author label Oct 25, 2023

IngelaAndin requested changes Oct 25, 2023

View reviewed changes

andreasanta force-pushed the pk_bad_date branch from 0e656af to 9b6516f Compare October 31, 2023 18:34

IngelaAndin added testing currently being tested, tag is used by OTP internal CI and removed waiting waiting for changes/input from author testing currently being tested, tag is used by OTP internal CI labels Nov 1, 2023

IngelaAndin approved these changes Nov 1, 2023

View reviewed changes

u3s approved these changes Nov 3, 2023

View reviewed changes

IngelaAndin added waiting waiting for changes/input from author and removed testing currently being tested, tag is used by OTP internal CI labels Nov 7, 2023

andreasanta closed this Nov 14, 2023

andreasanta force-pushed the pk_bad_date branch from 9b6516f to 7dae72f Compare November 14, 2023 18:13

Gracefully handle bad validity dates in public_key:pkix_path_validation

97d4036

andreasanta reopened this Nov 14, 2023

IngelaAndin added the testing currently being tested, tag is used by OTP internal CI label Nov 16, 2023

IngelaAndin merged commit e1b6fcd into erlang:maint Nov 17, 2023
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gracefully handle bad validity dates in public_key:pkix_path_validation #7515

Gracefully handle bad validity dates in public_key:pkix_path_validation #7515

andreasanta commented Jul 26, 2023

CLAassistant commented Jul 26, 2023 •

edited

Loading

github-actions bot commented Jul 26, 2023 •

edited

Loading

IngelaAndin commented Aug 1, 2023

andreasanta commented Aug 6, 2023 •

edited

Loading

dgud Aug 16, 2023

andreasanta Aug 23, 2023

dgud Aug 24, 2023

u3s Sep 5, 2023 •

edited

Loading

andreasanta Sep 17, 2023

dgud Sep 18, 2023

IngelaAndin Sep 18, 2023

andreasanta Oct 18, 2023

andreasanta Oct 18, 2023

IngelaAndin commented Aug 16, 2023

IngelaAndin commented Aug 16, 2023

IngelaAndin commented Aug 16, 2023

andreasanta commented Aug 23, 2023

u3s Sep 5, 2023

andreasanta Sep 17, 2023

IngelaAndin Sep 18, 2023 •

edited

Loading

IngelaAndin Sep 18, 2023

andreasanta Sep 18, 2023

IngelaAndin Oct 24, 2023

IngelaAndin left a comment

andreasanta commented Oct 31, 2023

IngelaAndin commented Nov 1, 2023

IngelaAndin commented Nov 3, 2023

u3s Nov 3, 2023

u3s Nov 3, 2023

u3s Nov 3, 2023

IngelaAndin commented Nov 7, 2023

andreasanta commented Nov 14, 2023

IngelaAndin commented Nov 15, 2023

Gracefully handle bad validity dates in public_key:pkix_path_validation #7515

Gracefully handle bad validity dates in public_key:pkix_path_validation #7515

Conversation

andreasanta commented Jul 26, 2023

CLAassistant commented Jul 26, 2023 • edited Loading

github-actions bot commented Jul 26, 2023 • edited Loading

CT Test Results

Artifacts

IngelaAndin commented Aug 1, 2023

andreasanta commented Aug 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

u3s Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IngelaAndin commented Aug 16, 2023

IngelaAndin commented Aug 16, 2023

IngelaAndin commented Aug 16, 2023

andreasanta commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IngelaAndin Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IngelaAndin left a comment

Choose a reason for hiding this comment

andreasanta commented Oct 31, 2023

IngelaAndin commented Nov 1, 2023

IngelaAndin commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IngelaAndin commented Nov 7, 2023

andreasanta commented Nov 14, 2023

IngelaAndin commented Nov 15, 2023

CLAassistant commented Jul 26, 2023 •

edited

Loading

github-actions bot commented Jul 26, 2023 •

edited

Loading

andreasanta commented Aug 6, 2023 •

edited

Loading

u3s Sep 5, 2023 •

edited

Loading

IngelaAndin Sep 18, 2023 •

edited

Loading