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

Fix & update documentation about RAND_priv_bytes() #6514

Closed
wants to merge 8 commits into from
Closed

Fix & update documentation about RAND_priv_bytes() #6514

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ed10e19
Fix & update documentation about RAND_priv_bytes()
romen Jun 18, 2018
7bd4945
[will-squash] Address feedback
romen Jun 18, 2018
4fdeba6
[will-squash] Update Copyright year in `doc/man3/BN-rand.pod`
romen Jun 18, 2018
509cdce
[fixup] rephrase, restyle, edit RAND.7
romen Jun 19, 2018
e5cde2e
[fixup] make comments about error checking more explicit
romen Jun 19, 2018
d246d50
[fixup] fix links to external manpages
romen Jun 19, 2018
f443e19
[fixup] add missing commas
romen Jun 19, 2018
f26704c
[fixup] address minor grammar nits spotted by @kaduk
romen Jun 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 19 additions & 4 deletions doc/man3/BN_rand.pod
View file
Open in desktop
Expand Up @@ -2,18 +2,24 @@

=head1 NAME

BN_rand, BN_pseudo_rand, BN_rand_range, BN_pseudo_rand_range - generate pseudo-random number
BN_rand, BN_priv_rand, BN_pseudo_rand,
BN_rand_range, BN_priv_rand_range, BN_pseudo_rand_range
- generate pseudo-random number

=head1 SYNOPSIS

#include <openssl/bn.h>

int BN_rand(BIGNUM *rnd, int bits, int top, int bottom);

int BN_priv_rand(BIGNUM *rnd, int bits, int top, int bottom);

int BN_pseudo_rand(BIGNUM *rnd, int bits, int top, int bottom);

int BN_rand_range(BIGNUM *rnd, BIGNUM *range);

int BN_priv_rand_range(BIGNUM *rnd, BIGNUM *range);

int BN_pseudo_rand_range(BIGNUM *rnd, BIGNUM *range);

=head1 DESCRIPTION
Expand All @@ -37,7 +43,14 @@ If B<bits> is 1 then B<top> cannot also be B<BN_RAND_FLG_TOPTWO>.
BN_rand_range() generates a cryptographically strong pseudo-random
number B<rnd> in the range 0 E<lt>= B<rnd> E<lt> B<range>.

The PRNG must be seeded prior to calling BN_rand() or BN_rand_range().
BN_priv_rand() and BN_priv_rand_range() have the same semantics as
BN_rand() and BN_rand_range() respectively. They are intended to be
used for generating values that should remain private, and mirror the
same difference between RAND_bytes() and RAND_priv_bytes().

An error occurs in the underlying B<RAND> functions if the CSPRNG has
not been seeded with enough randomness to ensure an unpredictable byte
sequence.
Copy link
Member Author

Choose a reason for hiding this comment

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

I mirrored the note contained in RAND_bytes.3 for

An error occurs in the underlying B functions if the CSPRNG has not been seeded with enough randomness to ensure an unpredictable byte sequence.

Reading RAND.7, to me it seems that this, and the original note, are in conflict to the following statement:

The default random generator will initialize automatically on first use and will be fully functional without having to be initialized ('seeded') explicitly. It seeds and reseeds itself automatically using trusted random sources provided by the operating system.

  • Should I remove the above note?
  • What about the one in RAND_bytes.3?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, silly me, rereading it they are not in conflict. It's just remarking about the fact that whenever randomness is requested through one of those functions, the developer should always check for errors in case there wasn't enough randomness.

I would rephrase this note and the one in RAND_bytes.3 just to make the message about checking the return value more explicit for this reason!

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to become something bigger? Then you might want to consider one of the following two options:

  • Add "WIP:" to the beginning of the pull request title so we see it is work-in-progress and will make useful comments but not attempt a formal review. (As long as it is WIP, you are free to squash and reorganize as you like)

  • Keep this pull request limited to RAND_priv_bytes() only and create a new WIP pull request (which can even be based on this branch initially) for all the new ideas you came up with.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it's pretty much done now, when I opened it it was limited only to BN_priv_rand() and RAND_priv_bytes(), it was only after the feedback on RAND(7) and RAND_DRBG(7) that I noticed that also RAND(7) needed updating, because it had the same notice note on "long-term secrets" rather than "any private value".

Commit d246d50 too is just an improvement on some feedback I received in this PR, about including a note about errors when the CSPRNG does not have enough entropy: upon reading RAND(7) I initially misinterpreted the intention of the note when compared with the note in RAND(7) about automatic initialization and reseeding: when I (5 minutes later) realized that the note on RAND_bytes(3) was to remind API users to always check for the error return value, I made that more explicit so that other silly users like me don't fall in the same trap!


=head1 RETURN VALUES

Expand All @@ -53,13 +66,15 @@ BN_pseudo_rand_range() has been identical to BN_rand_range().
The "pseudo" functions should not be used and may be deprecated in
a future release.

BN_priv_rand() and BN_priv_rand_range() were added in OpenSSL 1.1.1.

=head1 SEE ALSO

L<ERR_get_error(3)>, L<RAND_add(3)>, L<RAND_bytes(3)>
L<ERR_get_error(3)>, L<RAND_add(3)>, L<RAND_bytes(3)>, L<RAND_priv_bytes(3)>

=head1 COPYRIGHT

Copyright 2000-2017 The OpenSSL Project Authors. All Rights Reserved.
Copyright 2000-2018 The OpenSSL Project Authors. All Rights Reserved.

Licensed under the OpenSSL license (the "License"). You may not use
this file except in compliance with the License. You can obtain a copy
Expand Down
9 changes: 6 additions & 3 deletions doc/man3/RAND_bytes.pod
View file
Open in desktop
Expand Up @@ -24,9 +24,10 @@ into B<buf>. An error occurs if the CSPRNG has not been seeded with
enough randomness to ensure an unpredictable byte sequence.

RAND_priv_bytes() has the same semantics as RAND_bytes(). It is intended to
be used for generating long-term private keys. If using the default
RAND_METHOD, this function uses a separate instance of the PRNG so that
a compromise of the global generator will not affect such key generation.
be used for generating values that should remain private. If using the
default RAND_METHOD, this function uses a separate instance of the PRNG
so that a compromise of the PRNG used for public-facing values will not
affect the secrecy of the values from the private generator.

=head1 RETURN VALUES

Expand All @@ -39,6 +40,8 @@ obtained by L<ERR_get_error(3)>.

RAND_pseudo_bytes() was deprecated in OpenSSL 1.1.0; use RAND_bytes() instead.

RAND_priv_bytes() was added in OpenSSL 1.1.1.

=head1 SEE ALSO

L<RAND_add(3)>,
Expand Down