Skip to content

deps: set CARES_RANDOM_FILE for c-ares#48156

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
Merged
nodejs-github-bot merged 1 commit into from
May 26, 2023
Merged

deps: set CARES_RANDOM_FILE for c-ares #48156

nodejs-github-bot merged 1 commit into from
May 26, 2023

Conversation

@richardlau
Copy link
Member

@richardlaurichardlau commented May 24, 2023

Upstream c-ares renamed RANDOM_FILE to CARES_RANDOM_FILE some time ago in c-ares 1.17.2.

Refs: c-ares/c-ares#397

@richardlau
deps: set CARES_RANDOM_FILE for c-ares
0df81fa
Upstream c-ares renamed `RANDOM_FILE` to `CARES_RANDOM_FILE` some time ago in c-ares 1.17.2.
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 24, 2023

Review requested:

  • @nodejs/net

@nodejs-github-botnodejs-github-bot added cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. labels May 24, 2023
@richardlaurichardlau added the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 24, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/51948/

mhdawson
mhdawson approved these changes May 24, 2023
View reviewed changes
Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 24, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/51951/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 24, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/51954/

@richardlaurichardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 24, 2023
bnoordhuis
bnoordhuis approved these changes May 24, 2023
View reviewed changes
Copy link
Member

@bnoordhuisbnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but the c-ares logic from before yesterday's upgrade looks broken to me:

node/deps/cares/src/lib/ares_init.c

Lines 2189 to 2202 in 0e79635

# ifdefCARES_RANDOM_FILE
FILE*f=fopen(CARES_RANDOM_FILE, "rb");
if(f){
setvbuf(f, NULL, _IONBF, 0);
counter=aresx_uztosi(fread(key, 1, key_data_len, f));
fclose(f);
}
# endif
#endif/* WIN32 */
if (!randomized){
for (;counter<key_data_len;counter++)
key[counter]=(unsigned char)(rand() % 256); /* LCOV_EXCL_LINE */
}

It reads from /dev/urandom but then overwrites the result with rand()...

Should probably be filed as a security bug against the current release lines because it results in somewhat predictable DNS sequence ids.

The latest c-ares still falls back to that code path when /dev/urandom isn't accessible.

@richardlau
Copy link
MemberAuthor

richardlau commented May 24, 2023

LGTM but the c-ares logic from before yesterday's upgrade looks broken to me:

node/deps/cares/src/lib/ares_init.c

Lines 2189 to 2202 in 0e79635

# ifdefCARES_RANDOM_FILE
FILE*f=fopen(CARES_RANDOM_FILE, "rb");
if(f){
setvbuf(f, NULL, _IONBF, 0);
counter=aresx_uztosi(fread(key, 1, key_data_len, f));
fclose(f);
}
# endif
#endif/* WIN32 */
if (!randomized){
for (;counter<key_data_len;counter++)
key[counter]=(unsigned char)(rand() % 256); /* LCOV_EXCL_LINE */
}

It reads from /dev/urandom but then overwrites the result with rand()...

It doesn't reset counter so should not be overwriting the bytes read from /dev/urandom.

@bnoordhuis
Copy link
Member

bnoordhuis commented May 24, 2023

Right, but it assumes the read always succeeds with the requested number of bytes. At least musl libc sometimes return short or zero reads (e.g. when interrupted by a signal) and other libcs probably do too.

#define HAVE_ARC4RANDOM_BUF 1 on Linux could help because it's provided by glibc and handles edge cases like ☝️ but it's missing from musl...

@github-actionsgithub-actionsbot mentioned this pull request May 25, 2023
Open
21 tasks
@bnoordhuis
Copy link
Member

bnoordhuis commented May 25, 2023

#define HAVE_ARC4RANDOM_BUF 1 on Linux could help because it's provided by glibc and handles edge cases like ☝️ but it's missing from musl...

Next best thing: c-ares/c-ares#526

@github-actionsgithub-actionsbot mentioned this pull request May 26, 2023
Open
21 tasks
@richardlaurichardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label May 26, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 26, 2023
@nodejs-github-botnodejs-github-bot merged commit 70da075 into nodejs:mainMay 26, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 26, 2023

Landed in 70da075

This was referenced May 27, 2023
Open
Open
Open
Open
targos pushed a commit that referenced this pull request May 30, 2023
@richardlau@targos
deps: set CARES_RANDOM_FILE for c-ares
3047cae
Upstream c-ares renamed `RANDOM_FILE` to `CARES_RANDOM_FILE` some time ago in c-ares 1.17.2. PR-URL: #48156 Refs: c-ares/c-ares#397 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
@github-actionsgithub-actionsbot mentioned this pull request May 31, 2023
Open
22 tasks
@targostargos mentioned this pull request Jun 4, 2023
Merged
denihs pushed a commit to meteor/node-v14-esm that referenced this pull request Jun 22, 2023
@richardlau@denihs
deps: set CARES_RANDOM_FILE for c-ares
7d4da40
Upstream c-ares renamed `RANDOM_FILE` to `CARES_RANDOM_FILE` some time ago in c-ares 1.17.2. PR-URL: nodejs#48156 Refs: c-ares/c-ares#397 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
@mxdvlmxdvl mentioned this pull request Jun 23, 2023
Merged
erikolofsson pushed a commit to Malterlib/node that referenced this pull request Jun 26, 2023
@richardlau@erikolofsson
deps: set CARES_RANDOM_FILE for c-ares
704b543
Upstream c-ares renamed `RANDOM_FILE` to `CARES_RANDOM_FILE` some time ago in c-ares 1.17.2. PR-URL: nodejs#48156 Refs: c-ares/c-ares#397 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
erikolofsson pushed a commit to Malterlib/node that referenced this pull request Jun 26, 2023
@richardlau@erikolofsson
deps: set CARES_RANDOM_FILE for c-ares
78159c8
Upstream c-ares renamed `RANDOM_FILE` to `CARES_RANDOM_FILE` some time ago in c-ares 1.17.2. PR-URL: nodejs#48156 Refs: c-ares/c-ares#397 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
denihs pushed a commit to meteor/node-v14-esm that referenced this pull request Jul 3, 2023
@richardlau@denihs
deps: set CARES_RANDOM_FILE for c-ares
8baf61c
Upstream c-ares renamed `RANDOM_FILE` to `CARES_RANDOM_FILE` some time ago in c-ares 1.17.2. PR-URL: nodejs#48156 Refs: c-ares/c-ares#397 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@richardlau@Ceres6
deps: set CARES_RANDOM_FILE for c-ares
46a72e0
Upstream c-ares renamed `RANDOM_FILE` to `CARES_RANDOM_FILE` some time ago in c-ares 1.17.2. PR-URL: nodejs#48156 Refs: c-ares/c-ares#397 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@richardlau@Ceres6
deps: set CARES_RANDOM_FILE for c-ares
936dcc4
Upstream c-ares renamed `RANDOM_FILE` to `CARES_RANDOM_FILE` some time ago in c-ares 1.17.2. PR-URL: nodejs#48156 Refs: c-ares/c-ares#397 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
@richardlaurichardlau deleted the cares branch August 24, 2023 12:21
aduh95 pushed a commit to aduh95/node that referenced this pull request Feb 18, 2025
@richardlau@RafaelGSS
deps: set CARES_RANDOM_FILE for c-ares
70f9449
Upstream c-ares renamed `RANDOM_FILE` to `CARES_RANDOM_FILE` some time ago in c-ares 1.17.2. PR-URL: nodejs#48156 Refs: c-ares/c-ares#397 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
m0th3rch1p pushed a commit to m0th3rch1p/node that referenced this pull request Jul 20, 2025
@richardlau@RafaelGSS
deps: set CARES_RANDOM_FILE for c-ares
5df04e8
Upstream c-ares renamed `RANDOM_FILE` to `CARES_RANDOM_FILE` some time ago in c-ares 1.17.2. PR-URL: nodejs#48156 Refs: c-ares/c-ares#397 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes

@mhdawsonmhdawsonmhdawson approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.caresIssues and PRs related to the c-ares dependency or the cares_wrap binding.dependenciesPull requests that update a dependency file.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@richardlau@nodejs-github-bot@bnoordhuis@mhdawson