-
Notifications
You must be signed in to change notification settings - Fork 254
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
Warning: Use Cipheriv for counter mode of aes-256-ctr #296
Comments
how to fix |
@mhamann Any comment since this is a potential vulnerability? |
@bdharrington7 yes...obviously something that should be fixed. Unfortunately the easy fix requires the upstream to update first, and I don't think I've seen any progress there. I would like to revisit the crypto that nconf uses at some point anyway, so maybe the guidance for now is simply to handle encrypted values outside of the package. |
I left a comment on jcrugzz/secure-keys#1. I don't know critical the data is that nconf stores encrypted but an attacker that manages to obtain a ciphertext can decrypt other ciphertexts that were encrypted with the same secret. |
Thanks. Seems like The next major release of nconf would be 1.0, which would be a good opportunity to re-work nconf's encrypted value capability. Any interest in PRing some enhancements in that area (specifically to remove the dependency on secure-keys)? |
I've opened #299. |
bump |
@nichojo89 this was mostly fixed in #322, but still needs a few changes before releasing. There will be a v1.0.0 beta in the future that contains this change. I'm working on the release for that as time allows. |
Any update on this? Node 8 is officially not supported from 2020. I believe the v1 needs to be released ASAP |
Hi there, thanks for the reminder on this. I'm fairly close to having a v1 beta available and I think a full v1 can follow on fairly quickly, given the breaking changes are not extensive. I am hoping to have this available within the next two weeks. Feel free to hold me accountable! :-) |
@mhamann , nconf/lib/nconf/stores/file.js Line 213 in f1ca5f0
I was wandering if you're planning to remove that condition in the version 1. |
@andreasonny83 yeah, i think we should remove deprecated APIs, since we're breaking a few things with this. feel free to submit a PR around it or I'll hopefully get to it here shortly. |
@andreasonny83 offending code has been removed and tests updated. I'll leave the PR (#380) open for a bit before merging if you want to have a look. |
Thanks for sharing @mhamann . The PR looks good to me. |
@mhamann Any ETA for releasing v1? |
v1 beta 1 coming this week! |
@andreasonny83 at long last, v1.0.0-beta.0 is now available on npm. To install, run |
Starting in at least node v8.7.0, this warning is emitted when using the default algorithm. The recommended approach is to use an
iv
and callcrypto.createCipheriv(algorithm, key, iv)
instead ofcrypto.createCipher(algorithm, key)
. I'm guessing this would change the API of secure-keys to require aniv
in addition to asecret
in the options to instantiate it.The text was updated successfully, but these errors were encountered: