Skip to content
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

tls: Use SHA1 for sessionIdContext in FIPS mode #3755

Closed
wants to merge 1 commit into from

Conversation

stefanmb
Copy link
Contributor

By default, a call to tls.createServer() without a sessionIdContext will use a “MD5 hash value generated from command-line” as per documentation.

In FIPS mode MD5 is not allowed, however createServer is often called without specifying an explicit sessionIdContext. A significant number of test cases and applications break. The simple solution is to to use an allowed hash function. I have chosen SHA1 and truncated the output to 128 bits (which is the hardcoded length required by OpenSSL’s SSL_MAX_SID_CTX_LENGTH).

Note that I have opted to maintain the use of MD5 in non-FIPS mode, and updated the documentation accordingly.

@mscdex
Copy link
Contributor

mscdex commented Nov 11, 2015

This should target tls instead of the crypto subsystem.

@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Nov 11, 2015
@stefanmb stefanmb changed the title crypto: Use SHA1 for sessionIdContext in FIPS mode tls: Use SHA1 for sessionIdContext in FIPS mode Nov 11, 2015
@stefanmb
Copy link
Contributor Author

@mscdex I updated the commit message accordingly. Thanks.

@mhdawson mhdawson added the crypto Issues and PRs related to the crypto subsystem. label Nov 11, 2015
if (process.versions.openssl && process.versions.openssl.endsWith("-fips")) {
return crypto.createHash('sha1')
.update(defaultText)
.digest('hex').substring(0,32); /* SSL_MAX_SID_CTX_LENGTH is 128 bits */
Copy link
Member

Choose a reason for hiding this comment

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

Let's use .slice() here, this is what we try to do everywhere else.

@indutny
Copy link
Member

indutny commented Nov 11, 2015

I think that in general it may be a good idea to use sha256 on both fips and non-fips node. What do you think?

@mhdawson
Copy link
Member

The main concern would be interoperability. Would changing to sha256 for non-FIPs mean that there would be problems inter-operating with with earlier Node.js releases ?

@stefanmb
Copy link
Contributor Author

@indutny @mhdawson

I don't particularly like this commit. What's fed into the hash function is the process name and arguments. I maintained MD5 for non-FIPS because I was afraid someone somewhere might be serializing (caching) sessions across process lifetimes.

I've looked a bit more into the OpenSSL caching mechanism, specifically:
https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_set_session_cache_mode.html
https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_sess_set_get_cb.html

The only place where we provide external callbacks for session management is in node_crypto.cc GetSessionCallback and NewSessionCallback. In turn, these store data using CRYPTO_get_ex_data and CRYPTO_set_ex_data which I believe means the data only live as long as the process.

Therefore, I suggest we don't use a hash at all, and instead simply call crypto.randomBytes(16) once in this file and use the resulting value for the lifetime of a given Node process, in both FIPS and non-FIPS mode.

Edit: We still have to clarify why MD5 was mentioned in the docs, is that just internals leaking into the documentation or did it have some meaning to users?

@jasnell
Copy link
Member

jasnell commented Nov 14, 2015

@stefanmb ... just a quick note, I see this PR has a few conflicts now, can you please take a moment to rebase and update. Thank you!

@stefanmb
Copy link
Contributor Author

@jasnell Fixed, thanks!

@stefanmb
Copy link
Contributor Author

@indutny @shigeki
Any thoughts on my comment above regarding the use of crypto.randomBytes instead of a hash?
(1) Is it okay to provide a randomized session ID for the lifespan of each process invocation, instead of deriving the session ID deterministically from the program arguments? In particular, I'm not sure how this affects the cluster module. It seems execArgv is passed to the child workers in cluster.js.
(2) Is it okay to remove the mention of MD5 from the documentation? Quote:

sessionIdContext: A string containing an opaque identifier for session resumption. If requestCert is true, the default is MD5 hash value generated from command-line. Otherwise, the default is not provided.
From documentation for tls.createServer(options[, secureConnectionListener])

If so, I can update this PR. Thank you for your help and advice!

@indutny
Copy link
Member

indutny commented Nov 14, 2015

@stefanmb the idea, originally was to provide the same session context to clustered processes (if I remember it right)

@stefanmb
Copy link
Contributor Author

@indutny Okay, that's what I started to suspect. It's kind of clunky to do it through argv though. Anyway, perhaps the simplest thing is to move to a truncated SHA1 for both FIPS and non-FIPS. OpenSSL requires 128 bits, so I don't think there's a point in using a bigger hash.

@indutny
Copy link
Member

indutny commented Nov 14, 2015

Alright, this makes sense to me.

.update(defaultText)
.digest('hex').slice(0, 32); /* SSL_MAX_SID_CTX_LENGTH is 128 bits */
} else {
return crypto.createHash('md5')
Copy link
Member

Choose a reason for hiding this comment

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

Hm... Let's use sha1 for both cases, and remove condition?

@stefanmb stefanmb force-pushed the fips-cs4-tls-wrap branch 2 times, most recently from 042f645 to ab64b8b Compare November 16, 2015 21:09
@stefanmb
Copy link
Contributor Author

@indutny Okay, changed this to do SHA1 in all cases, the change is now pretty trivial.

@indutny
Copy link
Member

indutny commented Nov 16, 2015

@stefanmb I was thinking about it for a bit now, and I think we should handle it this way. Let's split this PR into two commits:

  • First with both md5 and sha1 (as you originally did). This commit will be backported as semver-patch
  • And the one that removes md5 entirely. This commit is going to be a semver-major

The best way to do it is probably to submit a second PR for the second half. @stefanmb does it sound reasonable?

Thank you very much!

@stefanmb stefanmb changed the title tls: Use SHA1 for sessionIdContext in FIPS mode tls: Use SHA1 for sessionIdContext Nov 16, 2015
@stefanmb stefanmb changed the title tls: Use SHA1 for sessionIdContext tls: Use SHA1 for sessionIdContext in FIPS mode Nov 16, 2015
FIPS 140-2 disallows use of MD5, which is used to derive the
default sessionIdContext for tls.createServer().
MylesBorins pushed a commit that referenced this pull request Nov 30, 2015
FIPS 140-2 disallows use of MD5, which is used to derive the
default sessionIdContext for tls.createServer().

PR-URL: #3755
Reviewed-By: Fedor Indutny <fedor@indutny.com>
rvagg pushed a commit that referenced this pull request Dec 4, 2015
FIPS 140-2 disallows use of MD5, which is used to derive the
default sessionIdContext for tls.createServer().

PR-URL: #3755
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@jasnell jasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
FIPS 140-2 disallows use of MD5, which is used to derive the
default sessionIdContext for tls.createServer().

PR-URL: #3755
Reviewed-By: Fedor Indutny <fedor@indutny.com>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
FIPS 140-2 disallows use of MD5, which is used to derive the
default sessionIdContext for tls.createServer().

PR-URL: #3755
Reviewed-By: Fedor Indutny <fedor@indutny.com>
bbondy pushed a commit to brave/node that referenced this pull request Mar 13, 2016
Notable changes:

* buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) nodejs/node#3767.
  - This makes the behavior match what the docs suggest.
* child_process: `child.send()` now properly returns a boolean like
the docs suggest (Rich Trott) nodejs/node#3577.
* doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) nodejs/node#3662.
* http_parser: update http-parser to 2.6.0 from 2.5.0 (James M
Snell) nodejs/node#3569.
  - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`,
`REBIND`, `UNBIND`.
  - Also added ACL and IPv6 Zone ID support.
* npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner)
nodejs/node#3685.
  - See the release notes for
    https://github.com/npm/npm/releases/tag/v3.3.7,
    https://github.com/npm/npm/releases/tag/v3.3.8,
    https://github.com/npm/npm/releases/tag/v3.3.9,
    https://github.com/npm/npm/releases/tag/v3.3.10,
    https://github.com/npm/npm/releases/tag/v3.3.11, and
    https://github.com/npm/npm/releases/tag/v3.3.12 for more details.
* repl: The REPL no longer crashes if the persistent history file
cannot be opened (Evan Lucas) nodejs/node#3630.
* tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) nodejs/node#3755.
* v8: Added some more useful post-mortem data (Fedor Indutny) nodejs/node#3779.

PR-URL: nodejs/node#3736
bbondy pushed a commit to brave/node that referenced this pull request Mar 14, 2016
Notable changes:

* buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) nodejs/node#3767.
  - This makes the behavior match what the docs suggest.
* child_process: `child.send()` now properly returns a boolean like
the docs suggest (Rich Trott) nodejs/node#3577.
* doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) nodejs/node#3662.
* http_parser: update http-parser to 2.6.0 from 2.5.0 (James M
Snell) nodejs/node#3569.
  - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`,
`REBIND`, `UNBIND`.
  - Also added ACL and IPv6 Zone ID support.
* npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner)
nodejs/node#3685.
  - See the release notes for
    https://github.com/npm/npm/releases/tag/v3.3.7,
    https://github.com/npm/npm/releases/tag/v3.3.8,
    https://github.com/npm/npm/releases/tag/v3.3.9,
    https://github.com/npm/npm/releases/tag/v3.3.10,
    https://github.com/npm/npm/releases/tag/v3.3.11, and
    https://github.com/npm/npm/releases/tag/v3.3.12 for more details.
* repl: The REPL no longer crashes if the persistent history file
cannot be opened (Evan Lucas) nodejs/node#3630.
* tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) nodejs/node#3755.
* v8: Added some more useful post-mortem data (Fedor Indutny) nodejs/node#3779.

PR-URL: nodejs/node#3736
bbondy pushed a commit to brave/node that referenced this pull request Mar 14, 2016
Notable changes:

* buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) nodejs/node#3767.
  - This makes the behavior match what the docs suggest.
* child_process: `child.send()` now properly returns a boolean like
the docs suggest (Rich Trott) nodejs/node#3577.
* doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) nodejs/node#3662.
* http_parser: update http-parser to 2.6.0 from 2.5.0 (James M
Snell) nodejs/node#3569.
  - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`,
`REBIND`, `UNBIND`.
  - Also added ACL and IPv6 Zone ID support.
* npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner)
nodejs/node#3685.
  - See the release notes for
    https://github.com/npm/npm/releases/tag/v3.3.7,
    https://github.com/npm/npm/releases/tag/v3.3.8,
    https://github.com/npm/npm/releases/tag/v3.3.9,
    https://github.com/npm/npm/releases/tag/v3.3.10,
    https://github.com/npm/npm/releases/tag/v3.3.11, and
    https://github.com/npm/npm/releases/tag/v3.3.12 for more details.
* repl: The REPL no longer crashes if the persistent history file
cannot be opened (Evan Lucas) nodejs/node#3630.
* tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) nodejs/node#3755.
* v8: Added some more useful post-mortem data (Fedor Indutny) nodejs/node#3779.

PR-URL: nodejs/node#3736
bbondy pushed a commit to brave/node that referenced this pull request Mar 15, 2016
Notable changes:

* buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) nodejs/node#3767.
  - This makes the behavior match what the docs suggest.
* child_process: `child.send()` now properly returns a boolean like
the docs suggest (Rich Trott) nodejs/node#3577.
* doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) nodejs/node#3662.
* http_parser: update http-parser to 2.6.0 from 2.5.0 (James M
Snell) nodejs/node#3569.
  - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`,
`REBIND`, `UNBIND`.
  - Also added ACL and IPv6 Zone ID support.
* npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner)
nodejs/node#3685.
  - See the release notes for
    https://github.com/npm/npm/releases/tag/v3.3.7,
    https://github.com/npm/npm/releases/tag/v3.3.8,
    https://github.com/npm/npm/releases/tag/v3.3.9,
    https://github.com/npm/npm/releases/tag/v3.3.10,
    https://github.com/npm/npm/releases/tag/v3.3.11, and
    https://github.com/npm/npm/releases/tag/v3.3.12 for more details.
* repl: The REPL no longer crashes if the persistent history file
cannot be opened (Evan Lucas) nodejs/node#3630.
* tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) nodejs/node#3755.
* v8: Added some more useful post-mortem data (Fedor Indutny) nodejs/node#3779.

PR-URL: nodejs/node#3736
@sneak
Copy link

sneak commented Apr 7, 2016

This broke my app:

krs_1 | 
krs_1 | > key-recovery-service@0.0.4 test /var/app
krs_1 | > node ./node_modules/mocha/bin/_mocha
krs_1 | 
krs_1 | _tls_wrap.js:22
krs_1 |   if (process.config.variables.openssl_fips) {
krs_1 |                               ^
krs_1 | 
krs_1 | TypeError: Cannot read property 'openssl_fips' of undefined
krs_1 |   at getDefaultSessionIdContext (_tls_wrap.js:22:31)
krs_1 |   at _tls_wrap.js:17:33
krs_1 |   at NativeModule.compile (node.js:954:5)
krs_1 |   at NativeModule.require (node.js:902:18)
krs_1 |   at tls.js:221:21
krs_1 |   at NativeModule.compile (node.js:954:5)
krs_1 |   at NativeModule.require (node.js:902:18)
krs_1 |   at https.js:3:13
krs_1 |   at NativeModule.compile (node.js:954:5)
krs_1 |   at Function.NativeModule.require (node.js:902:18)
krs_1 |   at Function.Module._load (module.js:298:25)
krs_1 |   at Module.require (module.js:366:17)
krs_1 |   at require (module.js:385:17)
krs_1 |   at Object.<anonymous> (/var/app/node_modules/supertest/node_modules/superagent/node_modules/form-data/lib/form_data.js:5:13)
krs_1 |   at Module._compile (module.js:435:26)
krs_1 |   at Object.Module._extensions..js (module.js:442:10)
krs_1 |   at Module.load (/var/app/node_modules/coffee-script/lib/coffee-script/register.js:45:36)
krs_1 |   at Function.Module._load (module.js:313:12)
krs_1 |   at Module.require (module.js:366:17)
krs_1 |   at require (module.js:385:17)
krs_1 |   at Object.<anonymous> (/var/app/node_modules/supertest/node_modules/superagent/lib/node/index.js:8:16)
krs_1 |   at Module._compile (module.js:435:26)
krs_1 |   at Object.Module._extensions..js (module.js:442:10)
krs_1 |   at Module.load (/var/app/node_modules/coffee-script/lib/coffee-script/register.js:45:36)
krs_1 |   at Function.Module._load (module.js:313:12)
krs_1 |   at Module.require (module.js:366:17)
krs_1 |   at require (module.js:385:17)
krs_1 |   at Object.<anonymous> (/var/app/node_modules/supertest/lib/test.js:5:15)
krs_1 |   at Module._compile (module.js:435:26)
krs_1 |   at Object.Module._extensions..js (module.js:442:10)
krs_1 |   at Module.load (/var/app/node_modules/coffee-script/lib/coffee-script/register.js:45:36)
krs_1 |   at Function.Module._load (module.js:313:12)
krs_1 |   at Module.require (module.js:366:17)
krs_1 |   at require (module.js:385:17)
krs_1 |   at Object.<anonymous> (/var/app/node_modules/supertest/index.js:7:12)
krs_1 |   at Module._compile (module.js:435:26)
krs_1 |   at Object.Module._extensions..js (module.js:442:10)
krs_1 |   at Module.load (/var/app/node_modules/coffee-script/lib/coffee-script/register.js:45:36)
krs_1 |   at Function.Module._load (module.js:313:12)
krs_1 |   at Module.require (module.js:366:17)
krs_1 |   at require (module.js:385:17)
krs_1 |   at Object.<anonymous> (/var/app/node_modules/supertest-as-promised/index.js:3:17)
krs_1 |   at Module._compile (module.js:435:26)
krs_1 |   at Object.Module._extensions..js (module.js:442:10)
krs_1 |   at Module.load (/var/app/node_modules/coffee-script/lib/coffee-script/register.js:45:36)
krs_1 |   at Function.Module._load (module.js:313:12)
krs_1 |   at Module.require (module.js:366:17)
krs_1 |   at require (module.js:385:17)
krs_1 |   at Object.<anonymous> (/var/app/test/app.coffee:3:11)
krs_1 |   at Object.<anonymous> (/var/app/test/app.coffee:2:1)
krs_1 |   at Module._compile (module.js:435:26)
krs_1 |   at Object.loadFile (/var/app/node_modules/coffee-script/lib/coffee-script/register.js:16:19)
krs_1 |   at Module.load (/var/app/node_modules/coffee-script/lib/coffee-script/register.js:45:36)
krs_1 |   at Function.Module._load (module.js:313:12)
krs_1 |   at Module.require (module.js:366:17)
krs_1 |   at require (module.js:385:17)
krs_1 |   at /var/app/node_modules/mocha/lib/mocha.js:219:27
krs_1 |   at Array.forEach (native)
krs_1 |   at Mocha.loadFiles (/var/app/node_modules/mocha/lib/mocha.js:216:14)
krs_1 |   at Mocha.run (/var/app/node_modules/mocha/lib/mocha.js:468:10)
krs_1 |   at Object.<anonymous> (/var/app/node_modules/mocha/bin/_mocha:403:18)
krs_1 |   at Module._compile (module.js:435:26)
krs_1 |   at Object.Module._extensions..js (module.js:442:10)
krs_1 |   at Module.load (module.js:356:32)
krs_1 |   at Function.Module._load (module.js:313:12)
krs_1 |   at Function.Module.runMain (module.js:467:10)
krs_1 |   at startup (node.js:136:18)
krs_1 |   at node.js:963:3
krs_1 | 
krs_1 | npm ERR! Test failed.  See above for more details.

It works on 4.2.3 but fails on 4.2.4. It still fails on 4.4.2 (current) and latest 5.x.

Isn't the point of LTS to not add features and not break compatibility?

@MylesBorins
Copy link
Contributor

@sneak are you 100% there isn't some monkey patching of the process object happening in your app?

The error you are getting is suggesting that process.config.variables does not exist at all.

here is a screen shot of me sanity checking this in v4

screen shot 2016-04-07 at 4 05 58 pm

Here is v5
screen shot 2016-04-07 at 4 08 06 pm

@sneak
Copy link

sneak commented Apr 7, 2016

I'm not; I have not read every line of the testing framework involved, I'm just letting you know that a test suite that passed on 4.2.3 now no longer runs on 4.2.4.

I thought the point of LTS was to not break stuff and cost people time; why not keep this FIPS stuff to 5.x? I don't know what ill-advised stuff this app or its deps was doing, but I do now know that it doesn't work anymore and that this was the change that broke it.

@sneak
Copy link

sneak commented Apr 7, 2016

PS: When posting text snippets, you don't need to push bitmaps, simply wrapping them in triple backticks is sufficient.

@MylesBorins
Copy link
Contributor

/cc @nodejs/crypto @nodejs/lts

fwiw we extensively smoke test LTS, we do occasionally hit edges like this though. Thank you for reporting this, we'll dig in and see what we can find.

@sneak
Copy link

sneak commented Apr 7, 2016

FWIW:

  "devDependencies": {
    "chai": "3.5.0",
    "istanbul": "0.4.3",
    "mocha": "2.4.5",
    "supertest": "1.2.0",
    "supertest-as-promised": "3.1.0",
    "sqlite3": "*"
  },
  "dependencies": {
    "argparse": "1.0.2",
    "bitcoinjs-lib": "1.5.6",
    "body-parser": "1.13.2",
    "coffee-script": "1.10.0",
    "express": "4.13.1",
    "js-beautify": "1.5.10",
    "lodash": "^4.0.0",
    "moment": "2.10.6",
    "morgan": "1.6.1",
    "node-jsrender": "1.0.6",
    "nodemailer": "1.4.0",
    "nodemailer-smtp-transport": "1.0.3",
    "pg": "4.4.2",
    "pg-hstore": "2.3.2",
    "prompt-sync": "1.0.0",
    "q": "1.4.1",
    "sequelize": "3.17.0",
    "sequelize-cli": "1.9.1",
    "validator": "4.0.2",
    "winston": "2.2.0"
  }

I bumped the versions of the stuff mentioned in the stack trace to latest, but still no love.

@jasnell
Copy link
Member

jasnell commented Apr 7, 2016

Yeah, the goal of LTS is to keep things as stable as absolutely possible but regressions do creep in from time to time. To help keep things easier to maintain we'll go ahead and pull certain changes back that appear innocuous but every once in a while we'll hit an edge case that needs to be looked at. For this, a quick existence check on process.config.variables and process.config (just to be safe) should be sufficient. Interested in opening a PR? ;-) Once it's reviewed we can get it landed in master, v5 and v4 so it goes out with the next release runs.

@sneak sneak mentioned this pull request Apr 8, 2016
@sneak
Copy link

sneak commented Apr 8, 2016

There you go, a PR against 4.x. I haven't tried getting 5.x to work yet, I can do that next.

@sneak
Copy link

sneak commented Apr 8, 2016

It looks like the change is exactly the same in 5.x, but master has a different file without any FIPS stuff in it. (I haven't tested the breaking app against master, but can if you need me to.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants