Skip to content

Commit d99d657

Browse files
tniessentargos
tniessen
authored and
targos
committed
src: fix FIPS init error handling
If `--enable-fips` or `--force-fips` fails to be applied during `ProcessFipsOptions()`, the node process might exit with `ExitCode::kNoFailure` because `ERR_GET_REASON(ERR_peek_error())` can return `0` since `ncrypto::testFipsEnabled()` does not populate the OpenSSL error queue. You can likely test this locally by running node --enable-fips && echo $? with the current `node` binary from the main branch if compiled without support for FIPS. As confirmed by the `XXX` comment here, which was added three years ago in commit b697160, even if the error queue was populated properly, the OpenSSL reason codes are not really related to the Node.js `ExitCode` enumeration. PR-URL: #58379 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
1 parent dc21054 commit d99d657

File tree

2 files changed

+29
-5
lines changed

2 files changed

+29
-5
lines changed

‎src/node.cc

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1175,10 +1175,7 @@ InitializeOncePerProcessInternal(const std::vector<std::string>& args,
11751175
}
11761176
#endif
11771177
if (!crypto::ProcessFipsOptions()) {
1178-
// XXX: ERR_GET_REASON does not return something that is
1179-
// useful as an exit code at all.
1180-
result->exit_code_ =
1181-
static_cast<ExitCode>(ERR_GET_REASON(ERR_peek_error()));
1178+
result->exit_code_ = ExitCode::kGenericUserError;
11821179
result->early_return_ = true;
11831180
result->errors_.emplace_back(
11841181
"OpenSSL error when trying to enable FIPS:\n" +

‎test/parallel/test-crypto-fips.js

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,16 @@ const FIPS_ENABLE_ERROR_STRING = 'OpenSSL error when trying to enable FIPS:';
2323
const CNF_FIPS_ON = fixtures.path('openssl_fips_enabled.cnf');
2424
const CNF_FIPS_OFF = fixtures.path('openssl_fips_disabled.cnf');
2525

26+
const kNoFailure = 0;
27+
const kGenericUserError = 1;
28+
2629
let num_children_ok = 0;
2730

2831
function sharedOpenSSL() {
2932
return process.config.variables.node_shared_openssl;
3033
}
3134

32-
function testHelper(stream, args, expectedOutput, cmd, env) {
35+
function testHelper(stream, args, expectedStatus, expectedOutput, cmd, env) {
3336
const fullArgs = args.concat(['-e', `console.log(${cmd})`]);
3437
const child = spawnSync(process.execPath, fullArgs, {
3538
cwd: path.dirname(process.execPath),
@@ -56,6 +59,7 @@ function testHelper(stream, args, expectedOutput, cmd, env) {
5659
// Normal path where we expect either FIPS enabled or disabled.
5760
assert.strictEqual(getFipsValue, expectedOutput);
5861
}
62+
assert.strictEqual(child.status, expectedStatus);
5963
childOk(child);
6064
}
6165

@@ -66,6 +70,7 @@ function testHelper(stream, args, expectedOutput, cmd, env) {
6670
testHelper(
6771
testFipsCrypto() ? 'stdout' : 'stderr',
6872
['--enable-fips'],
73+
testFipsCrypto() ? kNoFailure : kGenericUserError,
6974
testFipsCrypto() ? FIPS_ENABLED : FIPS_ENABLE_ERROR_STRING,
7075
'process.versions',
7176
process.env);
@@ -74,6 +79,7 @@ testHelper(
7479
testHelper(
7580
testFipsCrypto() ? 'stdout' : 'stderr',
7681
['--force-fips'],
82+
testFipsCrypto() ? kNoFailure : kGenericUserError,
7783
testFipsCrypto() ? FIPS_ENABLED : FIPS_ENABLE_ERROR_STRING,
7884
'process.versions',
7985
process.env);
@@ -85,6 +91,7 @@ if (!sharedOpenSSL()) {
8591
testHelper(
8692
'stdout',
8793
[],
94+
kNoFailure,
8895
FIPS_DISABLED,
8996
'require("crypto").getFips()',
9097
{ ...process.env, 'OPENSSL_CONF': ' ' });
@@ -94,6 +101,7 @@ if (!sharedOpenSSL()) {
94101
testHelper(
95102
'stderr',
96103
[],
104+
kGenericUserError,
97105
'Calling crypto.setFips() is not supported in workers',
98106
'new worker_threads.Worker(\'require("crypto").setFips(true);\', { eval: true })',
99107
process.env);
@@ -120,6 +128,7 @@ if (!sharedOpenSSL() && !hasOpenSSL3) {
120128
testHelper(
121129
'stdout',
122130
[`--openssl-config=${CNF_FIPS_ON}`],
131+
kNoFailure,
123132
testFipsCrypto() ? FIPS_ENABLED : FIPS_DISABLED,
124133
'require("crypto").getFips()',
125134
process.env);
@@ -128,6 +137,7 @@ if (!sharedOpenSSL() && !hasOpenSSL3) {
128137
testHelper(
129138
'stdout',
130139
[],
140+
kNoFailure,
131141
testFipsCrypto() ? FIPS_ENABLED : FIPS_DISABLED,
132142
'require("crypto").getFips()',
133143
Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_ON }));
@@ -136,6 +146,7 @@ if (!sharedOpenSSL() && !hasOpenSSL3) {
136146
testHelper(
137147
'stdout',
138148
[`--openssl-config=${CNF_FIPS_ON}`],
149+
kNoFailure,
139150
testFipsCrypto() ? FIPS_ENABLED : FIPS_DISABLED,
140151
'require("crypto").getFips()',
141152
Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_OFF }));
@@ -149,6 +160,7 @@ if (!hasOpenSSL3) {
149160
testHelper(
150161
'stdout',
151162
[`--openssl-config=${CNF_FIPS_OFF}`],
163+
kNoFailure,
152164
FIPS_DISABLED,
153165
'require("crypto").getFips()',
154166
Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_ON }));
@@ -157,20 +169,23 @@ if (!hasOpenSSL3) {
157169
testHelper(
158170
testFipsCrypto() ? 'stdout' : 'stderr',
159171
['--enable-fips', `--openssl-config=${CNF_FIPS_OFF}`],
172+
testFipsCrypto() ? kNoFailure : kGenericUserError,
160173
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
161174
'require("crypto").getFips()',
162175
process.env);
163176
// --force-fips should take precedence over OpenSSL config file
164177
testHelper(
165178
testFipsCrypto() ? 'stdout' : 'stderr',
166179
['--force-fips', `--openssl-config=${CNF_FIPS_OFF}`],
180+
testFipsCrypto() ? kNoFailure : kGenericUserError,
167181
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
168182
'require("crypto").getFips()',
169183
process.env);
170184
// --enable-fips should turn FIPS mode on
171185
testHelper(
172186
testFipsCrypto() ? 'stdout' : 'stderr',
173187
['--enable-fips'],
188+
testFipsCrypto() ? kNoFailure : kGenericUserError,
174189
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
175190
'require("crypto").getFips()',
176191
process.env);
@@ -179,6 +194,7 @@ if (!hasOpenSSL3) {
179194
testHelper(
180195
testFipsCrypto() ? 'stdout' : 'stderr',
181196
['--force-fips'],
197+
testFipsCrypto() ? kNoFailure : kGenericUserError,
182198
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
183199
'require("crypto").getFips()',
184200
process.env);
@@ -187,6 +203,7 @@ if (!hasOpenSSL3) {
187203
testHelper(
188204
testFipsCrypto() ? 'stdout' : 'stderr',
189205
['--enable-fips'],
206+
testFipsCrypto() ? kNoFailure : kGenericUserError,
190207
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
191208
'require("crypto").getFips()',
192209
Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_OFF }));
@@ -195,6 +212,7 @@ if (!hasOpenSSL3) {
195212
testHelper(
196213
testFipsCrypto() ? 'stdout' : 'stderr',
197214
['--force-fips'],
215+
testFipsCrypto() ? kNoFailure : kGenericUserError,
198216
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
199217
'require("crypto").getFips()',
200218
Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_OFF }));
@@ -203,6 +221,7 @@ if (!hasOpenSSL3) {
203221
testHelper(
204222
testFipsCrypto() ? 'stdout' : 'stderr',
205223
[],
224+
testFipsCrypto() ? kNoFailure : kGenericUserError,
206225
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
207226
'(require("crypto").setFips(true),' +
208227
'require("crypto").getFips())',
@@ -212,6 +231,7 @@ if (!hasOpenSSL3) {
212231
testHelper(
213232
testFipsCrypto() ? 'stdout' : 'stderr',
214233
[],
234+
testFipsCrypto() ? kNoFailure : kGenericUserError,
215235
testFipsCrypto() ? FIPS_DISABLED : FIPS_UNSUPPORTED_ERROR_STRING,
216236
'(require("crypto").setFips(true),' +
217237
'require("crypto").setFips(false),' +
@@ -222,6 +242,7 @@ if (!hasOpenSSL3) {
222242
testHelper(
223243
testFipsCrypto() ? 'stdout' : 'stderr',
224244
[`--openssl-config=${CNF_FIPS_OFF}`],
245+
testFipsCrypto() ? kNoFailure : kGenericUserError,
225246
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
226247
'(require("crypto").setFips(true),' +
227248
'require("crypto").getFips())',
@@ -231,6 +252,7 @@ if (!hasOpenSSL3) {
231252
testHelper(
232253
'stdout',
233254
[`--openssl-config=${CNF_FIPS_ON}`],
255+
kNoFailure,
234256
FIPS_DISABLED,
235257
'(require("crypto").setFips(false),' +
236258
'require("crypto").getFips())',
@@ -240,6 +262,7 @@ if (!hasOpenSSL3) {
240262
testHelper(
241263
testFipsCrypto() ? 'stdout' : 'stderr',
242264
['--enable-fips'],
265+
testFipsCrypto() ? kNoFailure : kGenericUserError,
243266
testFipsCrypto() ? FIPS_DISABLED : FIPS_UNSUPPORTED_ERROR_STRING,
244267
'(require("crypto").setFips(false),' +
245268
'require("crypto").getFips())',
@@ -249,6 +272,7 @@ if (!hasOpenSSL3) {
249272
testHelper(
250273
'stderr',
251274
['--force-fips'],
275+
kGenericUserError,
252276
testFipsCrypto() ? FIPS_ERROR_STRING2 : FIPS_UNSUPPORTED_ERROR_STRING,
253277
'require("crypto").setFips(false)',
254278
process.env);
@@ -257,6 +281,7 @@ if (!hasOpenSSL3) {
257281
testHelper(
258282
testFipsCrypto() ? 'stdout' : 'stderr',
259283
['--force-fips'],
284+
testFipsCrypto() ? kNoFailure : kGenericUserError,
260285
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
261286
'(require("crypto").setFips(true),' +
262287
'require("crypto").getFips())',
@@ -266,6 +291,7 @@ if (!hasOpenSSL3) {
266291
testHelper(
267292
'stderr',
268293
['--force-fips', '--enable-fips'],
294+
kGenericUserError,
269295
testFipsCrypto() ? FIPS_ERROR_STRING2 : FIPS_UNSUPPORTED_ERROR_STRING,
270296
'require("crypto").setFips(false)',
271297
process.env);
@@ -274,6 +300,7 @@ if (!hasOpenSSL3) {
274300
testHelper(
275301
'stderr',
276302
['--enable-fips', '--force-fips'],
303+
kGenericUserError,
277304
testFipsCrypto() ? FIPS_ERROR_STRING2 : FIPS_UNSUPPORTED_ERROR_STRING,
278305
'require("crypto").setFips(false)',
279306
process.env);

0 commit comments

Comments
 (0)