Skip to content

Use the crypto random functions instead of rand().#1039

Merged
iphydf merged 1 commit into
TokTok:masterfrom
iphydf:avoid-rand
Aug 11, 2018
Merged

Use the crypto random functions instead of rand().#1039
iphydf merged 1 commit into
TokTok:masterfrom
iphydf:avoid-rand

Conversation

@iphydf

@iphydf iphydf commented Aug 1, 2018

Copy link
Copy Markdown
Member

Presumably the uses of rand() were fine because they were not used in
security-sensitive places, but having to think about whether a crappy RNG
is acceptable in each situation requires effort that could better be
spent elsewhere.

Also, this means that once we have a custom deterministic RNG for
testing, that RNG is used everywhere, so all the code is deterministic.

It also allowed us to delete a system-specific function that wasn't used
anywhere except in a call to srand().


This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Aug 1, 2018

@sudden6 sudden6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewed 8 of 8 files at r1.
Reviewable status: 0 of 1 LGTMs obtained (waiting on @zugz)

@robinlinden robinlinden left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewed 8 of 8 files at r1.
Reviewable status: 0 of 1 LGTMs obtained (waiting on @zugz)

@zugz zugz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained (waiting on @zugz)


toxcore/onion_client.c, line 270 at r2 (raw file):

    }

    uint32_t num_nodes = (onion_c->path_nodes_index < MAX_PATH_NODES) ? onion_c->path_nodes_index : MAX_PATH_NODES;

Might as well make it const?


toxcore/onion_client.c, line 1624 at r2 (raw file):

                    for (j = 0; j < n; ++j) {
                        uint32_t num = random_u32() % num_nodes;

const?


toxcore/onion_client.c, line 1744 at r2 (raw file):

            if (num_nodes != 0) {
                for (i = 0; i < (MAX_ONION_CLIENTS_ANNOUNCE / 2); ++i) {
                    uint32_t num = random_u32() % num_nodes;

const?

@iphydf iphydf force-pushed the avoid-rand branch 2 times, most recently from b3d129e to 3e5d449 Compare August 9, 2018 22:52

@iphydf iphydf left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained (waiting on @zugz)


toxcore/onion_client.c, line 270 at r2 (raw file):

Previously, zugz (zugz) wrote…

Might as well make it const?

Done.


toxcore/onion_client.c, line 1624 at r2 (raw file):

Previously, zugz (zugz) wrote…

const?

Done.


toxcore/onion_client.c, line 1744 at r2 (raw file):

Previously, zugz (zugz) wrote…

const?

Done.

Presumably the uses of `rand()` were fine because they were not used in
security-sensitive places, but having to think about whether a crappy RNG
is acceptable in each situation requires effort that could better be
spent elsewhere.

Also, this means that once we have a custom deterministic RNG for
testing, that RNG is used everywhere, so all the code is deterministic.

It also allowed us to delete a system-specific function that wasn't used
anywhere except in a call to `srand()`.
@iphydf iphydf unassigned zugz Aug 11, 2018
@iphydf iphydf merged commit 463cbcb into TokTok:master Aug 11, 2018
@iphydf iphydf deleted the avoid-rand branch August 11, 2018 13:10
@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.6 Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants