-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
deps: set CARES_RANDOM_FILE
for c-ares
#48156
Conversation
Upstream c-ares renamed `RANDOM_FILE` to `CARES_RANDOM_FILE` some time ago in c-ares 1.17.2.
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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
# ifdef CARES_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.
It doesn't reset |
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.
|
Next best thing: c-ares/c-ares#526 |
Landed in 70da075 |
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 <midawson@redhat.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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 <midawson@redhat.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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 <midawson@redhat.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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 <midawson@redhat.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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 <midawson@redhat.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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 <midawson@redhat.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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 <midawson@redhat.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Upstream c-ares renamed
RANDOM_FILE
toCARES_RANDOM_FILE
some time ago in c-ares 1.17.2.Refs: c-ares/c-ares#397