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

Security issue regarding derived key: Ref: Issue #6 & #13 #15

Closed
jas- opened this issue Sep 18, 2018 · 7 comments
Closed

Security issue regarding derived key: Ref: Issue #6 & #13 #15

jas- opened this issue Sep 18, 2018 · 7 comments

Comments

@jas-
Copy link

jas- commented Sep 18, 2018

Originally posted by @jas- in #13 (comment)

@jas-
Copy link
Author

jas- commented Dec 19, 2018

Not seeing any traction here.

Using a static initial key as the source of future keying material combined with a non-random salt weakens the resulting cipher text used to store sensitive data in the browser.

The salt being generated should be replaced with the window.crypto.getRandomValues() function. This will ensure the generated salt to derive your master key is truly random.

While the usability decision to hard code a passphrase for the initial keying material is common, if you are truly trying to protect data in the browser prompting the user for a passphrase or even using a combination of the browsers fingerprint with a user supplied passphrase will ensure any browser plugin and/or XSS style attack retrieving the stored data would be hard pressed to convert it back to plain text.

@softvar
Copy link
Owner

softvar commented Dec 19, 2018

Hey @jas- , could you help in resolving the issue? A pull-request might help here.
If you can fix this, it would increase the security of the data being stored.
Thanks!

@jas- jas- closed this as completed Dec 21, 2018
@jas- jas- reopened this Dec 27, 2018
@konsultaner
Copy link
Contributor

@jas- Isn't it just a problem if you don't pass your own password? See here.

@jas-
Copy link
Author

jas- commented Jan 11, 2019

Yes, but why a static key?

@konsultaner
Copy link
Contributor

@jas- when I started #23 I thought the meta data is encrypted with the password and method passed into the constructor. But this is not the case. I know why @softvar did it like this. He wanted to make it possible to use different algorithms and passwords for parallel instances. I wanted that too, but thought that would be impossible if the meta data is encoded with the first given password and encryption method. By splitting it up into namespaces, the meta data may be encrypted with the passed password and encryption method. This way it would be possible to fix #22 and this issue.

@jas- jas- closed this as completed Jan 11, 2019
@jas-
Copy link
Author

jas- commented Jan 12, 2019

@konsultaner Implementing a key-ring inside of the existing DOM to allow for encrypted objects with different keys doesn't alleviate the original issue.

References: Classification of cryptographic keys, RFC 4107 - Guidelines for cryptographic key management, NIST SP 800-57 - Recommendations for key management.

Unless the user of this package is educated about the key derivation limitation when using the default static value and why they should a) use their own or b) prompt the user, then this package leaves the end user susceptible to simple decryption with little effort.

Not really sure why you mentioned this issue with your patch.

@konsultaner
Copy link
Contributor

@jas- my patch wasn't a fix yet. My PR was a preparation to make it fixable and keep the initial intention of @softvar to have multiple instances running at the same time. I was wondering why you closed this issue anyway. Maybe you should reopen until my next PR.

konsultaner added a commit to konsultaner/secure-ls that referenced this issue Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants