Skip to content

Commit cbec07e

Browse files
tniessenmarco-ippolito
tniessen
authored and
marco-ippolito
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 80fb80e commit cbec07e

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
@@ -1171,10 +1171,7 @@ InitializeOncePerProcessInternal(const std::vector<std::string>& args,
11711171
}
11721172
#endif
11731173
if (!crypto::ProcessFipsOptions()) {
1174-
// XXX: ERR_GET_REASON does not return something that is
1175-
// useful as an exit code at all.
1176-
result->exit_code_ =
1177-
static_cast<ExitCode>(ERR_GET_REASON(ERR_peek_error()));
1174+
result->exit_code_ = ExitCode::kGenericUserError;
11781175
result->early_return_ = true;
11791176
result->errors_.emplace_back(
11801177
"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
@@ -22,13 +22,16 @@ const FIPS_ENABLE_ERROR_STRING = 'OpenSSL error when trying to enable FIPS:';
2222
const CNF_FIPS_ON = fixtures.path('openssl_fips_enabled.cnf');
2323
const CNF_FIPS_OFF = fixtures.path('openssl_fips_disabled.cnf');
2424

25+
const kNoFailure = 0;
26+
const kGenericUserError = 1;
27+
2528
let num_children_ok = 0;
2629

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

31-
function testHelper(stream, args, expectedOutput, cmd, env) {
34+
function testHelper(stream, args, expectedStatus, expectedOutput, cmd, env) {
3235
const fullArgs = args.concat(['-e', `console.log(${cmd})`]);
3336
const child = spawnSync(process.execPath, fullArgs, {
3437
cwd: path.dirname(process.execPath),
@@ -55,6 +58,7 @@ function testHelper(stream, args, expectedOutput, cmd, env) {
5558
// Normal path where we expect either FIPS enabled or disabled.
5659
assert.strictEqual(getFipsValue, expectedOutput);
5760
}
61+
assert.strictEqual(child.status, expectedStatus);
5862
childOk(child);
5963
}
6064

@@ -65,6 +69,7 @@ function testHelper(stream, args, expectedOutput, cmd, env) {
6569
testHelper(
6670
testFipsCrypto() ? 'stdout' : 'stderr',
6771
['--enable-fips'],
72+
testFipsCrypto() ? kNoFailure : kGenericUserError,
6873
testFipsCrypto() ? FIPS_ENABLED : FIPS_ENABLE_ERROR_STRING,
6974
'process.versions',
7075
process.env);
@@ -73,6 +78,7 @@ testHelper(
7378
testHelper(
7479
testFipsCrypto() ? 'stdout' : 'stderr',
7580
['--force-fips'],
81+
testFipsCrypto() ? kNoFailure : kGenericUserError,
7682
testFipsCrypto() ? FIPS_ENABLED : FIPS_ENABLE_ERROR_STRING,
7783
'process.versions',
7884
process.env);
@@ -84,6 +90,7 @@ if (!sharedOpenSSL()) {
8490
testHelper(
8591
'stdout',
8692
[],
93+
kNoFailure,
8794
FIPS_DISABLED,
8895
'require("crypto").getFips()',
8996
{ ...process.env, 'OPENSSL_CONF': ' ' });
@@ -93,6 +100,7 @@ if (!sharedOpenSSL()) {
93100
testHelper(
94101
'stderr',
95102
[],
103+
kGenericUserError,
96104
'Calling crypto.setFips() is not supported in workers',
97105
'new worker_threads.Worker(\'require("crypto").setFips(true);\', { eval: true })',
98106
process.env);
@@ -119,6 +127,7 @@ if (!sharedOpenSSL() && !common.hasOpenSSL3) {
119127
testHelper(
120128
'stdout',
121129
[`--openssl-config=${CNF_FIPS_ON}`],
130+
kNoFailure,
122131
testFipsCrypto() ? FIPS_ENABLED : FIPS_DISABLED,
123132
'require("crypto").getFips()',
124133
process.env);
@@ -127,6 +136,7 @@ if (!sharedOpenSSL() && !common.hasOpenSSL3) {
127136
testHelper(
128137
'stdout',
129138
[],
139+
kNoFailure,
130140
testFipsCrypto() ? FIPS_ENABLED : FIPS_DISABLED,
131141
'require("crypto").getFips()',
132142
Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_ON }));
@@ -135,6 +145,7 @@ if (!sharedOpenSSL() && !common.hasOpenSSL3) {
135145
testHelper(
136146
'stdout',
137147
[`--openssl-config=${CNF_FIPS_ON}`],
148+
kNoFailure,
138149
testFipsCrypto() ? FIPS_ENABLED : FIPS_DISABLED,
139150
'require("crypto").getFips()',
140151
Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_OFF }));
@@ -148,6 +159,7 @@ if (!common.hasOpenSSL3) {
148159
testHelper(
149160
'stdout',
150161
[`--openssl-config=${CNF_FIPS_OFF}`],
162+
kNoFailure,
151163
FIPS_DISABLED,
152164
'require("crypto").getFips()',
153165
Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_ON }));
@@ -156,20 +168,23 @@ if (!common.hasOpenSSL3) {
156168
testHelper(
157169
testFipsCrypto() ? 'stdout' : 'stderr',
158170
['--enable-fips', `--openssl-config=${CNF_FIPS_OFF}`],
171+
testFipsCrypto() ? kNoFailure : kGenericUserError,
159172
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
160173
'require("crypto").getFips()',
161174
process.env);
162175
// --force-fips should take precedence over OpenSSL config file
163176
testHelper(
164177
testFipsCrypto() ? 'stdout' : 'stderr',
165178
['--force-fips', `--openssl-config=${CNF_FIPS_OFF}`],
179+
testFipsCrypto() ? kNoFailure : kGenericUserError,
166180
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
167181
'require("crypto").getFips()',
168182
process.env);
169183
// --enable-fips should turn FIPS mode on
170184
testHelper(
171185
testFipsCrypto() ? 'stdout' : 'stderr',
172186
['--enable-fips'],
187+
testFipsCrypto() ? kNoFailure : kGenericUserError,
173188
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
174189
'require("crypto").getFips()',
175190
process.env);
@@ -178,6 +193,7 @@ if (!common.hasOpenSSL3) {
178193
testHelper(
179194
testFipsCrypto() ? 'stdout' : 'stderr',
180195
['--force-fips'],
196+
testFipsCrypto() ? kNoFailure : kGenericUserError,
181197
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
182198
'require("crypto").getFips()',
183199
process.env);
@@ -186,6 +202,7 @@ if (!common.hasOpenSSL3) {
186202
testHelper(
187203
testFipsCrypto() ? 'stdout' : 'stderr',
188204
['--enable-fips'],
205+
testFipsCrypto() ? kNoFailure : kGenericUserError,
189206
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
190207
'require("crypto").getFips()',
191208
Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_OFF }));
@@ -194,6 +211,7 @@ if (!common.hasOpenSSL3) {
194211
testHelper(
195212
testFipsCrypto() ? 'stdout' : 'stderr',
196213
['--force-fips'],
214+
testFipsCrypto() ? kNoFailure : kGenericUserError,
197215
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
198216
'require("crypto").getFips()',
199217
Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_OFF }));
@@ -202,6 +220,7 @@ if (!common.hasOpenSSL3) {
202220
testHelper(
203221
testFipsCrypto() ? 'stdout' : 'stderr',
204222
[],
223+
testFipsCrypto() ? kNoFailure : kGenericUserError,
205224
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
206225
'(require("crypto").setFips(true),' +
207226
'require("crypto").getFips())',
@@ -211,6 +230,7 @@ if (!common.hasOpenSSL3) {
211230
testHelper(
212231
testFipsCrypto() ? 'stdout' : 'stderr',
213232
[],
233+
testFipsCrypto() ? kNoFailure : kGenericUserError,
214234
testFipsCrypto() ? FIPS_DISABLED : FIPS_UNSUPPORTED_ERROR_STRING,
215235
'(require("crypto").setFips(true),' +
216236
'require("crypto").setFips(false),' +
@@ -221,6 +241,7 @@ if (!common.hasOpenSSL3) {
221241
testHelper(
222242
testFipsCrypto() ? 'stdout' : 'stderr',
223243
[`--openssl-config=${CNF_FIPS_OFF}`],
244+
testFipsCrypto() ? kNoFailure : kGenericUserError,
224245
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
225246
'(require("crypto").setFips(true),' +
226247
'require("crypto").getFips())',
@@ -230,6 +251,7 @@ if (!common.hasOpenSSL3) {
230251
testHelper(
231252
'stdout',
232253
[`--openssl-config=${CNF_FIPS_ON}`],
254+
kNoFailure,
233255
FIPS_DISABLED,
234256
'(require("crypto").setFips(false),' +
235257
'require("crypto").getFips())',
@@ -239,6 +261,7 @@ if (!common.hasOpenSSL3) {
239261
testHelper(
240262
testFipsCrypto() ? 'stdout' : 'stderr',
241263
['--enable-fips'],
264+
testFipsCrypto() ? kNoFailure : kGenericUserError,
242265
testFipsCrypto() ? FIPS_DISABLED : FIPS_UNSUPPORTED_ERROR_STRING,
243266
'(require("crypto").setFips(false),' +
244267
'require("crypto").getFips())',
@@ -248,6 +271,7 @@ if (!common.hasOpenSSL3) {
248271
testHelper(
249272
'stderr',
250273
['--force-fips'],
274+
kGenericUserError,
251275
testFipsCrypto() ? FIPS_ERROR_STRING2 : FIPS_UNSUPPORTED_ERROR_STRING,
252276
'require("crypto").setFips(false)',
253277
process.env);
@@ -256,6 +280,7 @@ if (!common.hasOpenSSL3) {
256280
testHelper(
257281
testFipsCrypto() ? 'stdout' : 'stderr',
258282
['--force-fips'],
283+
testFipsCrypto() ? kNoFailure : kGenericUserError,
259284
testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING,
260285
'(require("crypto").setFips(true),' +
261286
'require("crypto").getFips())',
@@ -265,6 +290,7 @@ if (!common.hasOpenSSL3) {
265290
testHelper(
266291
'stderr',
267292
['--force-fips', '--enable-fips'],
293+
kGenericUserError,
268294
testFipsCrypto() ? FIPS_ERROR_STRING2 : FIPS_UNSUPPORTED_ERROR_STRING,
269295
'require("crypto").setFips(false)',
270296
process.env);
@@ -273,6 +299,7 @@ if (!common.hasOpenSSL3) {
273299
testHelper(
274300
'stderr',
275301
['--enable-fips', '--force-fips'],
302+
kGenericUserError,
276303
testFipsCrypto() ? FIPS_ERROR_STRING2 : FIPS_UNSUPPORTED_ERROR_STRING,
277304
'require("crypto").setFips(false)',
278305
process.env);

0 commit comments

Comments
 (0)