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

crypto: Disable crypto.createCipher in FIPS mode #3754

Closed
wants to merge 1 commit into from

Conversation

stefanmb
Copy link
Contributor

FIPS 140-2 disallows MD5, which is used to derive the initialization vector and key for createCipher(). Using a different hashing function (e.g. SHA1) will work, but it will make a FIPS Node build incompatible with data produced by (de)crypt routines from a non-FIPS build, since the use of MD5 is explicitly mentioned by the documentation.

I’ve opted to simply disable this API completely in FIPS mode. Note that the createCipher() API is deprecated and the use of createCipheriv() is recommended in general. Where possible I split up test suites according to use of createCipher and createCipheriv. Some tests cover a large amount of functionality and have no clear delineation of responsibilities. In this case I modified the tests to either avoid calling disallowed API in FIPS mode or to expect an exception.

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Nov 11, 2015
@shigeki
Copy link
Contributor

shigeki commented Nov 12, 2015

Using crypto.pbkdf2() and crypto.createCipheriv() is much bettter instead of using crypto.createCipher(). In FIPS mode, a key deviation should also be cared. I'm +1 to deprecate crypto.createCipher() only in FIPS mode but I wonder how it affects sem-ver change.
And it is better to add some descriptions to the doc.

@jasnell
Copy link
Member

jasnell commented Nov 12, 2015

This is an interesting one since, by default, we do not ship with FIPS mode enabled. Would we consider it a semver-minor or semver-major change if someone decided to disable part of the Intl APIs when ICU is not included? I'm not so sure given that someone has to explicitly create a non-default build to enable those options. Curious what other members of the @nodejs/ctc think...

@indutny
Copy link
Member

indutny commented Nov 12, 2015

@jasnell it can't be a semver-minor, because it does not work right now already :)

@mhdawson
Copy link
Member

I agree it can't break any existing user as the call would have failed in FIPs mode and letting people know about breaking changes is what semver is about. I could have seen it being a semver-major when the original change when in as it did not support the existing API but at this point it is not a change, just a clarification of the current state.

The more interesting question is where its ok to only support a subset of the APIs in different modes, and if it is ok, then whether its semver major or not when you add a new mode that does not support all of the APIs in the same way (for example even without this issue, FIPs disables a number of algs so effectively the API has changed, but ony in that mode). However, I don't think that's tied to this PR so if we want to discuss that we should do it in different issue.

@stefanmb stefanmb force-pushed the fips-cs3-disable-api branch 3 times, most recently from 96cbb66 to 980874a Compare November 13, 2015 20:58
FIPS 140-2 disallows use of MD5, which is used to derive the
initialization vector and key for createCipher(). Modify
all tests to expect exceptions in FIPS mode when disallowed
API is used, or to avoid testing such API in FIPS Mode.
@jasnell
Copy link
Member

jasnell commented Nov 15, 2015

@indutny ... good point. I'm not so sure it's that clear cut since this adds a throw that wasn't there before, albeit only in a mode that is non-default and optional. The change itself LGTM so there's no problem landing, but it's a bit of a tricky one.

Oh... another issue that comes to mind tho: we can't actually run these through the CI since our CI does not build with FIPS mode enabled. Perhaps that's something we need to look at? @nodejs/build

@jasnell
Copy link
Member

jasnell commented Nov 15, 2015

CI Run: https://ci.nodejs.org/job/node-test-pull-request/736/
(not to test the FIPS part, just to test that it doesn't interfere with anything else ;-) ...)

@mhdawson
Copy link
Member

@jasnell adding CI runs that build/test in FIPS mode is a next step I wanted to get in place after getting to the point where the tests would be able to run.

@mhdawson
Copy link
Member

The discussion where seems to be about issues other than whether this should go into master.

I'm lgtm to the specific change itself given @shigeki 's concurrence that it is the right way to go. @shigeki @indutny are we at the point were we can approve this for master ?

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

Yep, LGTM

@mhdawson
Copy link
Member

@shigeki @indutny we ready to land this one ? Just waiting on lgtm's from you two

@shigeki
Copy link
Contributor

shigeki commented Nov 20, 2015

LGTM

@mhdawson
Copy link
Member

@jasnell
Copy link
Member

jasnell commented Nov 20, 2015

CI is red but failures look unrelated. Landing!

jasnell pushed a commit that referenced this pull request Nov 20, 2015
FIPS 140-2 disallows use of MD5, which is used to derive the
initialization vector and key for createCipher(). Modify
all tests to expect exceptions in FIPS mode when disallowed
API is used, or to avoid testing such API in FIPS Mode.

PR-URL: #3754
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Nov 20, 2015

Landed in 56a2b9a

@jasnell jasnell closed this Nov 20, 2015
rvagg pushed a commit that referenced this pull request Dec 5, 2015
FIPS 140-2 disallows use of MD5, which is used to derive the
initialization vector and key for createCipher(). Modify
all tests to expect exceptions in FIPS mode when disallowed
API is used, or to avoid testing such API in FIPS Mode.

PR-URL: #3754
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvagg rvagg mentioned this pull request Dec 17, 2015
MylesBorins pushed a commit that referenced this pull request Dec 29, 2015
FIPS 140-2 disallows use of MD5, which is used to derive the
initialization vector and key for createCipher(). Modify
all tests to expect exceptions in FIPS mode when disallowed
API is used, or to avoid testing such API in FIPS Mode.

PR-URL: #3754
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
FIPS 140-2 disallows use of MD5, which is used to derive the
initialization vector and key for createCipher(). Modify
all tests to expect exceptions in FIPS mode when disallowed
API is used, or to avoid testing such API in FIPS Mode.

PR-URL: #3754
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants