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

Make openVMS seeding less dependent of OpenVMS version [1.1.1] #18730

Closed
wants to merge 2 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Jul 6, 2022

SYS$GETTIM_PREC is a very new function, only available on OpenVMS v8.4.
OpenSSL binaries built on OpenVMS v8.4 become unusable on older OpenVMS
versions, but building for the older CRTL version will make the high
precision time functions unavailable.

Tests have shown that on Alpha and Itanium, the time update granularity
between SYS$GETTIM and SYS$GETTIM_PREC is marginal, so the former plus
a sequence number turns out to be better to guarantee a unique nonce.

Fixes #18727

@levitte levitte added approval: otc review pending This pull request needs review by an OTC member approval: review pending This pull request needs review by a committer branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels Jul 6, 2022
@levitte levitte changed the title Make openVMS seeding less dependent of OpenVMS version Make openVMS seeding less dependent of OpenVMS version [1.1.1] Jul 6, 2022
crypto/rand/rand_vms.c Outdated Show resolved Hide resolved
@t8m
Copy link
Member

t8m commented Jul 7, 2022

I am unsure whether this is a bug fix or a feature.

@t8m t8m added triaged: bug The issue/pr is/fixes a bug triaged: feature The issue/pr requests/adds a feature labels Jul 7, 2022
@levitte
Copy link
Member Author

levitte commented Jul 7, 2022

It's a regression... before introducing the call of sys$gettim_prec(), it was perfectly possible to build OpenSSL on any OpenVMS version and have it run with any OpenVMS from v7.1 and up, as far as I recall. Suddenly, you cannot build on OpenVMS v8.4 and expect it to run on older versions.

In OpenVMS land, this isn't a strange concept, especially when you have clusters with multiple OpenVMS versions in mind.

@mgdaniel, feel free to correct me if I remember things wrong...

@levitte
Copy link
Member Author

levitte commented Jul 7, 2022

The call of sys$gettim_prec() was introduced in the development that became 1.1.1 (i.e. the first 1.1.1 release).

@mgdaniel
Copy link

mgdaniel commented Jul 7, 2022

The call of sys$gettim_prec() was introduced in the development that became 1.1.1 (i.e. the first 1.1.1 release).

Agreed. And I do not imagine it adds significant entropy to the the mix. The difference between $gettime() and $gettim_prec() must (currently) be minimal at best and in the future a few bits of resolution.

@mgdaniel
Copy link

mgdaniel commented Jul 7, 2022

I am unsure whether this is a bug fix or a feature.

I put it in as a bug fix because it prevented execution on less recent versions of VMS. Not a feature.

@mgdaniel
Copy link

mgdaniel commented Jul 7, 2022

It's a regression... before introducing the call of sys$gettim_prec(), it was perfectly possible to build OpenSSL on any OpenVMS version and have it run with any OpenVMS from v7.1 and up, as far as I recall. Suddenly, you cannot build on OpenVMS v8.4 and expect it to run on older versions.

In OpenVMS land, this isn't a strange concept, especially when you have clusters with multiple OpenVMS versions in mind.

@mgdaniel, feel free to correct me if I remember things wrong...

With SYS$GET_ENTROPY becoming available I believe SYS$GETTIM_PREC could easily be eliminated completely in favour of SYS$GETTIM with no effective/significant loss of entropy all-told, thus immediately eliminating the backward compatibility issue.

@levitte
Copy link
Member Author

levitte commented Jul 7, 2022

The call of sys$gettim_prec() was introduced in the development that became 1.1.1 (i.e. the first 1.1.1 release).

Agreed. And I do not imagine it adds significant entropy to the the mix. The difference between $gettime() and $gettim_prec() must (currently) be minimal at best and in the future a few bits of resolution.

So what you're saying is that the call of sys$gettim_prec() could just as well be dropped? 'cause yeah, the resulting time is a count of 100ns units either way, so the "tick" update difference between sys$gettim() and sys$gettim_prec() is probably not dramatic for this sort of use...

@mgdaniel
Copy link

mgdaniel commented Jul 7, 2022

The call of sys$gettim_prec() was introduced in the development that became 1.1.1 (i.e. the first 1.1.1 release).

Agreed. And I do not imagine it adds significant entropy to the the mix. The difference between $gettime() and $gettim_prec() must (currently) be minimal at best and in the future a few bits of resolution.

So what you're saying is that the call of sys$gettim_prec() could just as well be dropped? 'cause yeah, the resulting time is a count of 100ns units either way, so the "tick" update difference between sys$gettim() and sys$gettim_prec() is probably not dramatic for this sort of use...

Agreed. Certainly not significant with the current range of (rather passe) Alphas, and most Itanics (AIUI). The current crop of X86 systems/hypervisors; who knows what the TOD/interrupt clock will be doing. Perhaps we should ask VSI about hypervisor interrupt/timer resolution.

@mgdaniel
Copy link

mgdaniel commented Jul 7, 2022

Perhaps we should ask VSI about hypervisor interrupt/timer resolution.

https://forum.vmssoftware.com/viewtopic.php?f=21&t=8509

@mgdaniel
Copy link

mgdaniel commented Jul 8, 2022

Perhaps we should ask VSI about hypervisor interrupt/timer resolution.

https://forum.vmssoftware.com/viewtopic.php?f=21&t=8509

The upshot of asking this question (ignoring the hypervisor element) is that there is no (current) effective difference between the result of the two calls. This combined with your own SP 800-90A section 4 observation suggests that SYS$GETTIM_PREC may be eliminated while still maintaining compliance.

Suggest you get coding 😁

@mgdaniel
Copy link

And to give your a start, this seems to work:

/*
 *  NIST's SP 800-90A Rev 1, Section 8.6.7
 *  4. A combination of a timestamp and a monotonically increasing sequence number...
 */

static int gettime(unsigned __int64 *t)
{
    static unsigned __int64 seqnum = 0;
    unsigned __int64 t64;
    int  status;

    status = sys$gettim((void *)&t64);
    *t = (t64 & ~0Xff) | (seqnum++ & 0xff);
    return status;
}

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

@hlandau
Copy link
Member

hlandau commented Sep 9, 2022

Approving for reasons given in #18731.

@hlandau hlandau removed the approval: review pending This pull request needs review by a committer label Sep 9, 2022
@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

@levitte
Copy link
Member Author

levitte commented Oct 25, 2022

I'm reworking this change the same way I did #18731, i.e. forego SYS$GETTIM_PREC

SYS$GETTIM_PREC is a very new function, only available on OpenVMS v8.4.
OpenSSL binaries built on OpenVMS v8.4 become unusable on older OpenVM
versions, but building for the older CRTL version will make the high
precision time functions unavailable.

Tests have shown that on Alpha and Itanium, the time update granularity
between SYS$GETTIM and SYS$GETTIM_PREC is marginal, so the former plus
a sequence number turns out to be better to guarantee a unique nonce.

Fixes openssl#18727
@levitte
Copy link
Member Author

levitte commented Oct 25, 2022

Reworked. @mgdaniel, sorry that this took time to get back to. Would you be willing to test this (and by all means, #18731 too!)?

@levitte levitte added the approval: review pending This pull request needs review by a committer label Oct 25, 2022
@levitte levitte requested a review from hlandau October 25, 2022 11:49
@t8m t8m removed the triaged: feature The issue/pr requests/adds a feature label Oct 25, 2022
@levitte levitte removed the approval: otc review pending This pull request needs review by an OTC member label Oct 25, 2022
Copy link
Member

@hlandau hlandau left a comment

Choose a reason for hiding this comment

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

One nit. Otherwise, looks OK.

crypto/rand/rand_vms.c Outdated Show resolved Hide resolved
@mgdaniel
Copy link

mgdaniel commented Oct 25, 2022 via email

@levitte
Copy link
Member Author

levitte commented Oct 26, 2022

https://raw.githubusercontent.com/openssl/openssl/d100d5b52213963bc86dce460226b2b4bef980bf/crypto/rand/rand_vms.c

That's for 1.1.1. I'll get you the same separately in #18731 for 3.0

Sure Richard. Can you provide web links into the final rand_vms.c(s)?

@levitte levitte requested a review from t8m October 26, 2022 09:16
@t8m t8m 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 Oct 26, 2022
@levitte
Copy link
Member Author

levitte commented Oct 27, 2022

Merged

aa542d2 Make openVMS seeding less dependent of OpenVMS version

@levitte levitte closed this Oct 27, 2022
openssl-machine pushed a commit that referenced this pull request Oct 27, 2022
SYS$GETTIM_PREC is a very new function, only available on OpenVMS v8.4.
OpenSSL binaries built on OpenVMS v8.4 become unusable on older OpenVM
versions, but building for the older CRTL version will make the high
precision time functions unavailable.

Tests have shown that on Alpha and Itanium, the time update granularity
between SYS$GETTIM and SYS$GETTIM_PREC is marginal, so the former plus
a sequence number turns out to be better to guarantee a unique nonce.

Fixes #18727

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #18730)
@levitte levitte deleted the fix-18727-111 branch October 27, 2022 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants