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.

Sign up for GitHub

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

Jump to bottom

rand_lib: RAND_poll: Reseed in non-"no-deprecated" builds. #21167

Closed
wants to merge 1 commit into from
Closed

rand_lib: RAND_poll: Reseed in non-"no-deprecated" builds. #21167

wants to merge 1 commit into from

Conversation

mkasick
Copy link
Contributor

@mkasick mkasick commented Jun 9, 2023

I recently discovered that with a default non-"no-deprecated" libcrypto build (i.e., the one that ships with Ubuntu 22.04 LTS), calls to RAND_poll don't reseed the DRBGs the way that calls to RAND_seed or RAND_add do.

I'm aware that the DRBGs will request and add entropy automatically, so manual reseeding isn't necessary. However for applications that have reason (or requirement) to reseed during application-specific events, this brings RAND_poll in-line with the behavior of RAND_seed and RAND_add.

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jun 9, 2023
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jun 9, 2023
@tmshort tmshort added hold: cla required The contributor needs to submit a license agreement branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug labels Jun 11, 2023
@tmshort
Copy link
Contributor

tmshort commented Jun 11, 2023

Not sure if this can be considered (CLA: trivial)

@mkasick
Copy link
Contributor Author

mkasick commented Jun 11, 2023

The commit just reorders two existing lines of code (one of which is preprocessor directive) without further textual modification, and removes a third. I figured that fits pretty squarely under the meaning of a trivial change. Let me know if the consensus is otherwise and I'll sort out the CLA. Thanks.

t8m
t8m requested changes Jun 12, 2023
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

I am OK with CLA: trivial as this is really just some reordering/changing when it is built of existing code.

crypto/rand/rand_lib.c Outdated
}
return ret;
# else
# endif
static const char salt[] = "polling";
Copy link
Member

Choose a reason for hiding this comment

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

You'll have to move this line up before the # ifndef OPENSSL_NO_DEPRECATED_3_0 otherwise you would break ansi build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an -ansi -pedantic build?! Fine. (I thought this might happen.)

@t8m t8m added the tests: exempted The PR is exempt from requirements for testing label Jun 12, 2023
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jun 12, 2023
@paulidale
Copy link
Contributor

I wonder if this is worthwhile?
It's easy to force a reseeding using other calls. I'd really like RAND_poll to disappear...

@mkasick
rand_lib: RAND_poll: Reseed in non-"no-deprecated" builds.
180f6ae 
In a non-"no-deprecated" libcrypto build with a default configuration,
RAND_get_rand_method() == RAND_OpenSSL() and so needs to fall through to
the RAND_seed call (used in "no-deprecated" builds) to perform a reseed.

CLA: trivial
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jun 12, 2023
@mkasick
Copy link
Contributor Author

mkasick commented Jun 12, 2023

I wonder if this is worthwhile? It's easy to force a reseeding using other calls. I'd really like RAND_poll to disappear...
A couple things here:

  • I don't see a defensible argument for why RAND_poll should behave differently in non-deprecated and non-"non-deprecated" builds.
  • The current RAND_poll documentation says it may be used to (re)seed by "polling from various trusted entropy sources," so if one wanted to force a reseed and only read the documentation--not the code--for RAND_poll and RAND_seed/add, they might mistakenly call RAND_poll believing it does something that it (currently) doesn't do, while also believing RAND_seed/add don't do something they actually do. I made this very mistake at first.

As a user, I'd also be OK with a documentation errata update, although I have a preference for just fixing it.

@mkasick mkasick requested a review from t8m June 12, 2023 15:38
@t8m t8m added branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Jun 12, 2023
t8m
t8m approved these changes Jun 12, 2023
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

OK with CLA: trivial.

IMO this is simply a bug that should be fixed on all relevant branches.

Yeah, at some point RAND_poll() should go but we did not deprecate it.

@paulidale paulidale 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 approval: otc review pending This pull request needs review by an OTC member labels Jun 12, 2023
@t8m t8m added the cla: trivial One of the commits is marked as 'CLA: trivial' label Jun 13, 2023
@t8m
Copy link
Member

t8m commented Jun 13, 2023

@paulidale OK with CLA: trivial?

@paulidale
Copy link
Contributor

Yes, okay with trivial.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@paulidale
Copy link
Contributor

Merged, thanks for the contribution.

@paulidale paulidale closed this Jun 13, 2023
openssl-machine pushed a commit that referenced this pull request Jun 13, 2023
@mkasick @paulidale
rand_lib: RAND_poll: Reseed in non-"no-deprecated" builds.
bb13f85 
In a non-"no-deprecated" libcrypto build with a default configuration,
RAND_get_rand_method() == RAND_OpenSSL() and so needs to fall through to
the RAND_seed call (used in "no-deprecated" builds) to perform a reseed.

CLA: trivial

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21167)

(cherry picked from commit cc343d0)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2023
@mkasick @paulidale
rand_lib: RAND_poll: Reseed in non-"no-deprecated" builds.
cc343d0 
In a non-"no-deprecated" libcrypto build with a default configuration,
RAND_get_rand_method() == RAND_OpenSSL() and so needs to fall through to
the RAND_seed call (used in "no-deprecated" builds) to perform a reseed.

CLA: trivial

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21167)
openssl-machine pushed a commit that referenced this pull request Jun 13, 2023
@mkasick @paulidale
rand_lib: RAND_poll: Reseed in non-"no-deprecated" builds.
06ae946 
In a non-"no-deprecated" libcrypto build with a default configuration,
RAND_get_rand_method() == RAND_OpenSSL() and so needs to fall through to
the RAND_seed call (used in "no-deprecated" builds) to perform a reseed.

CLA: trivial

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21167)

(cherry picked from commit cc343d0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 cla: trivial One of the commits is marked as 'CLA: trivial' severity: fips change The pull request changes FIPS provider sources tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mkasick @tmshort @paulidale @t8m @openssl-machine