RFC5280 strict time string check by InfoHunter · Pull Request #3566 · openssl/openssl

InfoHunter · 2017-05-26T07:43:51Z

Checklist

documentation is added or updated
tests are added or updated

Thoughts: introduce a new API called "ASN1_TIME_set_string_strict" which is used to check if the time string (both UTC and Generalized) matches what RFC 5280 requires.

To make it compatible with those legacy certificates generated by previous versions of OpenSSL command line tool, all use the original "loose" rule to check the time string except the 'ca' application which is enforced to use the strict version to produce RFC-5280-fulfilled certificates only. Thus, openssl command can still read and use RFC-5280-time-conficting certificates but can't generate any of them.

Docs and tests will be updated as this fix is generally accepted for the concept/idea.

Please ignore the first unrelated 3 outdated commits : )

kroeckx · 2017-05-26T10:05:50Z

I guess I would prefer a function in the X509 name space and not in the generic ASN1. It's for X509 that we want to be strict. But then I'm not sure it's important.

I would also like to see tests to see that we reject invalid strings now, and that it properly ends up as UTC / Generalized.

InfoHunter · 2017-05-26T19:20:27Z

Test cases updated.

InfoHunter · 2017-05-28T18:44:35Z

Hi @kroeckx

I changed ASN1_TIME_set_string_strict to ASN1_TIME_set_string_X509, it's still in the ASN1 name space but the function name is more specific to X509. Since all X509 time related stuff is handled by ASN1 time functions now in OpenSSL, so it might be 'OK' to consider the strict X509 format as a a 'subset' feature offered by the underlaying ASN1. But it still looks good if we add an X509_time_xxx function in x509_vfy.c to wrap the ASN1 layer, so what do you think?

Documentation and test cases are all updated. Please let me know your considerations.

kroeckx · 2017-05-29T17:42:13Z

test/x509_time_test.c

I think you should probably either add something to the TESTDATA structure or use a different one. The current expected is the result of a compare, so you have a cmp_time and when comparing the parsed and the expected value you expect to get something. You don't use the cmp_time, and use expected for something else.

You also don't seem to be using the type you set in the test data, and adjust the type based on the string you give, which doesn't make sense. I think you should check both that parsing is successful and that it returned the correct type. The correct type for all of your strings should be V_ASN1_UTCTIME, because they're all before 2050. You should also create strings that are after 2050.

It might also be useful to do a check like giving "20170217180154Z" and checking that it turned it into "170217180154Z" (and is a V_ASN1_UTCTIME)

Hmm, I know what's going wrong: previously I only considered 'checking' the time string format but what is described in the 4.1.2.5. section of RFC 5280 is not implemented in my patch. So the test cases cover only the 'check' part, but no 're-format' the time string.

I need a little more time to add the missing functionality, thanks man!

InfoHunter · 2017-05-30T20:09:02Z

Hi @kroeckx , I have update the code to have "2050-Generalized-to-UTC" feature implemented, as well as test cases and documentation. Please review them, thanks.

kroeckx · 2017-05-30T20:22:43Z

include/openssl/asn1.h

I've been wondering if we shouldn't just set this flag somehow instead of creating a new API just to set this flag. I don't actually know enough about the whole API to know how to extend it best, so I'm open for other ideas.

IMHO, since we need to utilize the ASN1_TIME to struct tm parsing logic, it might be one of the best ways to reduce duplicated code if we use a flag and treat it in current ASN1_TIME to struct tm parsing functions.

There is no precedent, all other flags are set internally, so I think we're at liberty to define how to set this particular flag whatever way we wish. The way it's implemented here seems sane to me.

kroeckx · 2017-05-30T20:35:30Z

test/x509_time_test.c

use t != NULL

kroeckx · 2017-05-30T20:41:51Z

test/x509_time_test.c

I don't really understand the point of this, why not always give it an ASN1_TIME?

Because we need to test if this function can accept a NULL input for the first parameter and if behaves properly. The doc under doc/man3/ASN1_TIME_set.pod didn't describe this usage for the original ASN1_TIME_set_string(), but it's indeed used that way in apps/ca.c and might be used already by other applications. So I think we need to cover this scenario in case of the future-API-internal-change, we would probably get noticed if input of NULL ASN1_TIME still works or not.

InfoHunter · 2017-05-31T09:10:06Z

Is there a way to re-launch the travis-ci manually?

It seems it failed because the server didn't start:

Client command: echo test | ../util/shlib_wrap.sh ../apps/openssl s_client -engine ossltest -connect localhost:4453 -ct
engine "ossltest" set.
Connection opened
Failed to start up server (localhost,4443): Connection refused
Server command: ../util/shlib_wrap.sh ../apps/openssl s_server -no_comp -rev -engine ossltest -accept 4443 -cert ../apps/server.pem -cert2 ../apps/server.pem -naccept 1 -cipher AES128-SHA:TLS13-AES-128-GCM-SHA256 -status_file ../test/recipes/ocsp-response.der -serverinfo ../test/serverinfo2.pem

Per 9fa299f

I cannot reproduce this locally.

levitte · 2017-05-31T09:18:08Z

Okie, job restarted

InfoHunter · 2017-05-31T09:20:18Z

@levitte , is this normal? I mean a server can't start

mattcaswell · 2017-05-31T09:24:05Z

@levitte , is this normal? I mean a server can't start

Looking at the logs it seems to me that the server did actually start eventually. If you look at the following test the server fails to start there too because the address is already in use. The problem seems to be we gave up trying to connect too quickly. Looking at the code we try 3 times with a 0.1 sec pause between each attempt. Perhaps that is too aggressive.

InfoHunter · 2017-05-31T09:33:08Z

Yep, I noticed those bind errors. I agree with you that an 0.1s internal is too aggressive. Should it be adjust to avoid this? Or maybe the number of tries should be increased.

In a recent PR (openssl#3566) it seems that TLSProxy gave up trying to connect to the server process too quickly. This meant the test failed even though the server *did* eventually start. Currently we try 3 times to connect with a 0.1 second pause between each attempt. That is probably too aggressive.

mattcaswell · 2017-05-31T09:35:37Z

Yep, I noticed those bind errors. I agree with you that an 0.1s internal is too aggressive. Should it be adjust to avoid this? Or maybe the number of tries should be increased.

I upped the retries: #3587

In a recent PR (#3566) it seems that TLSProxy gave up trying to connect to the server process too quickly. This meant the test failed even though the server *did* eventually start. Currently we try 3 times to connect with a 0.1 second pause between each attempt. That is probably too aggressive. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #3587) (cherry picked from commit 142463c)

In a recent PR (#3566) it seems that TLSProxy gave up trying to connect to the server process too quickly. This meant the test failed even though the server *did* eventually start. Currently we try 3 times to connect with a 0.1 second pause between each attempt. That is probably too aggressive. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #3587)

levitte · 2017-05-31T17:16:52Z

doc/man3/ASN1_TIME_set.pod

Please add a history section mentioning this was added for OpenSSL 1.1.1. There are examples on how to do this in other .pod files

kroeckx · 2017-05-31T20:42:31Z

doc/man3/ASN1_TIME_set.pod

I think we don't need to document in detail what it all checks, just that it's restricts it to what RFC5280 allows. So maybe it should say that it only allows YYMMDDHHMMSSZ and YYYYMMDDHHMMSSZ, while the other functions allows others.

Okay, I'll make it briefer.

kroeckx · 2017-05-31T20:48:11Z

So it seems there is a conflict that you will need to resolve.

kroeckx · 2017-06-11T10:50:54Z

@richsalz: Are you still happy with the current version?

richsalz · 2017-06-11T19:56:24Z

I'm still okay; let me know if you want me to merge.

kroeckx · 2017-06-11T20:32:24Z

Feel free to merge

Make funcs to deal with non-null-term'd string in both asn1_generalizedtime_to_tm() and asn1_utctime_to_tm(). Fixes issue #3444. This one is used to enforce strict format (RFC 5280) check and to convert GeneralizedTime to UTCTime. apps/ca has been changed to use the new API. Test cases and documentation are updated/added Signed-off-by: Paul Yang <paulyang.inf@gmail.com> Reviewed-by: Kurt Roeckx <kurt@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3566)

richsalz · 2017-06-11T20:46:35Z

merged and squashed. thanks, paul!

"Note" part is based on PR openssl#3566

based on files changed in PR openssl#3566

Based on discussion in PR openssl#3566. Reduce duplicated code in original asn1_utctime_to_tm and asn1_generalizedtime_to_tm, and introduce a new internal function asn1_time_to_tm. This function also checks if the days in the input time string is valid or not for the corresponding month. Test cases are also added.

"Note" part is based on PR #3566 Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3895)

Based on discussion in PR #3566. Reduce duplicated code in original asn1_utctime_to_tm and asn1_generalizedtime_to_tm, and introduce a new internal function asn1_time_to_tm. This function also checks if the days in the input time string is valid or not for the corresponding month. Test cases are also added. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #3905)

In a recent PR (openssl#3566) it seems that TLSProxy gave up trying to connect to the server process too quickly. This meant the test failed even though the server *did* eventually start. Currently we try 3 times to connect with a 0.1 second pause between each attempt. That is probably too aggressive. Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#3587) (cherry picked from commit 142463c)

InfoHunter force-pushed the rfc5280-time branch from 99a694e to a998229 Compare May 28, 2017 13:25

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label May 28, 2017

InfoHunter force-pushed the rfc5280-time branch from a998229 to b0b71c7 Compare May 28, 2017 18:03

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label May 28, 2017

kroeckx reviewed May 29, 2017

View reviewed changes

InfoHunter force-pushed the rfc5280-time branch from fd70364 to 2760f9f Compare May 30, 2017 18:11

kroeckx reviewed May 30, 2017

View reviewed changes

mattcaswell mentioned this pull request May 31, 2017

Wait longer for the server in TLSProxy to start #3587

Closed

InfoHunter mentioned this pull request May 31, 2017

Fix Coding Style and Strange Logic in apps/passwd #3586

Closed

levitte removed the hold: cla required The contributor needs to submit a license agreement label May 31, 2017

levitte requested changes May 31, 2017

View reviewed changes

kroeckx reviewed May 31, 2017

View reviewed changes

kroeckx approved these changes Jun 11, 2017

View reviewed changes

kroeckx added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jun 11, 2017

richsalz closed this Jun 11, 2017

InfoHunter deleted the rfc5280-time branch June 11, 2017 23:30

InfoHunter mentioned this pull request Jul 9, 2017

Update doc/ca.pod to clarify description for dates #3895

Closed

1 task

InfoHunter mentioned this pull request Jul 11, 2017

Add asn1_time_to_tm function and check days in month #3905

Closed

1 task

InfoHunter added a commit to InfoHunter/openssl that referenced this pull request Jul 15, 2017

Update doc/ca.pod to clarify description for dates

fbdcb61

"Note" part is based on PR openssl#3566

InfoHunter mentioned this pull request Jul 17, 2017

Insert BaishanCloud copyright line #3950

Closed

InfoHunter pushed a commit to InfoHunter/openssl that referenced this pull request Jul 17, 2017

Insert BaishanCloud copyright line

1f14618

based on files changed in PR openssl#3566

levitte pushed a commit that referenced this pull request Jul 23, 2017

Update doc/ca.pod to clarify description for dates

909873b

"Note" part is based on PR #3566 Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3895)

davidben mentioned this pull request Nov 10, 2025

Make ASN1_STRING opaque and X509 more memory-efficient in 4.x #29117

Open

Uh oh!

Conversation

InfoHunter commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

kroeckx commented May 26, 2017

Uh oh!

InfoHunter commented May 26, 2017

Uh oh!

InfoHunter commented May 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

InfoHunter commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

InfoHunter commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented May 31, 2017

Uh oh!

InfoHunter commented May 31, 2017

Uh oh!

mattcaswell commented May 31, 2017

Uh oh!

InfoHunter commented May 31, 2017

Uh oh!

mattcaswell commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroeckx commented May 31, 2017

Uh oh!

kroeckx commented Jun 11, 2017

Uh oh!

richsalz commented Jun 11, 2017

Uh oh!

kroeckx commented Jun 11, 2017 via email

Uh oh!

richsalz commented Jun 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

InfoHunter commented May 26, 2017 •

edited

Loading

InfoHunter commented May 31, 2017 •

edited

Loading