4 ms·

This seems dangerous to use? Just a few issues looking at the code: - No master password is required - if you don't set one a default value of "hide" is used

This seems dangerous to use? Just a few issues looking at the code: - No master password is required - if you don't set one a default value of "hide" is used - The description says it uses AES256 (it is the default) but the code lets you specify your own algorithm, and supports ECB mode and even Triple DES - When an algorithm requires a (for instance) 32 byte key, and your master password is larger than 32 bytes, it will just take the first 32 bytes and ignore the rest. If your master password is less than 32 bytes, it will just keep concatenating your password with itself until it gets 32 bytes: https://github.com/whatl3y/hide/blob/master/src/libs/Encryption.ts#L131-L135 https://github.com/whatl3y/hide/blob/master/src/libs/Encrypt...

whatl3y 6y ago
- for you first point, this is not true[0]. can you provide a reference? - for your second point, if you want to fork this repo and use another algo, go for it. if you install the package from npm and use it or clone and install from source without changing it, it uses the default AES256 - this could be argued as a valid point, and maybe forcing the user to use 32 bytes as their password is the right way to do it, but no 3rd party password manager forces this limitation so i don’t believe I should? Maybe just forcing a password difficulty is the right way to go? I’m certainly open to ideas, this is just a small project i enjoyed building and feel like the minimum security requirements to meet my needs are there. [0]: https://github.com/whatl3y/hide/blob/1c3aaca634f504c3c274bac6a184b8783cd72df8/src/index.ts#L18 https://github.com/whatl3y/hide/blob/1c3aaca634f504c3c274bac...
- staticassertion 6y ago
  For the third point, why not just hash the password with SHA256 first? Sorry, I didn't look at the code, just going off of the described issue. If you have an arbitrarily sized input and you want a fixed 32 byte output, that seems by far the simplest way.
  - whatl3y 6y ago
    This is what I should do :) I simply append the password to itself to get 32 bytes, but SHA256 is almost certainly the better/right way. Will add a PR this weekend.
    - staticassertion 6y ago
      Cool! Sounds like a good idea. A key stretcher is probably a good idea too if you haven't done so. Given that this is local only, you can go pretty nuts on the key stretching. For example, on a server I'd say >100K rounds of PBKDF2. For local, you could do 1B.
    - jlgaddis 6y ago
      A single round of SHA256 used as a master password is definitely NOT what you "should" do. --- (I apologize in advance. I don't typically make comments like this on HN but I feel it really is warranted here.) Key Derivative Functions (KDFs) have already been mentioned here in the comments. By not using one -- and, perhaps more importantly, by 1) not even understanding why you should use one and 2) thinking that one round of SHA256 is what you "should" do -- well, to be blunt, your project just screams out "hacked together piece of shit made by someone who has absolutely zero business whatsoever making a password manager". --- "Show HN"'s are to get feedback. As it stands, your project should not be used -- by pretty much anyone! It doesn't have to stay that way, though. Please take some of the good advice you've gotten in the comments here and improve your project so that it can safely be used.
      - whatl3y 6y ago
        To anyone else who reads this I’ve made several changes to the code based on feedback presented in previous comments so far including using PBKDF2 for key derivation (actually well before jlgaddis’ comment here was posted which is unfortunate, sort of screams “I’m lazy and before I confirm I’m going to blast this peion publically because it’ll make my Sunday a little bit better than it already was”). Nonetheless I appreciate the last bullet as I definitely post to Show HN to learn and get better as a developer, and certainly expect nothing less than to receive a few “fuck your shitty project”s along the way :)
- arkadiyt 6y ago
  - config.cryptography.password is always set to a default value if not provided, it will never hit your error case [0]. This is trivially verifiable and I suggest you try it. - no, you accept the algorithm as an environment variable - someone can install your package as is and still set it themselves - as kohtatsu mentioned you should be using a key derivation function. [0]: https://github.com/whatl3y/hide/blob/1c3aaca634f504c3c274bac6a184b8783cd72df8/src/config.ts#L18 https://github.com/whatl3y/hide/blob/1c3aaca634f504c3c274bac...
  - whatl3y 6y ago
    - Wow apologies you’re exactly right, I just converted this from JS to typescript and instead of making this variable null | undefined | string, i made it a string and added a default as a shortcut to get it released. Apologies for questioning this will fix it asap, if nothing else I get some peer review when publishing a Show HN :) https://github.com/whatl3y/hide/blob/5ee54aad1b1c3da4d1865689286b28382c11ae1d/src/config.js https://github.com/whatl3y/hide/blob/5ee54aad1b1c3da4d186568... - Got it, yeah I guess my defense would be it still requires the user to make a conscious decision so it’s not as bad as it being ambiguous or unclear, although removing the ability to control this via env var might be the right move. - I’ll look into and add something to do this soon :)

whatl3y 6y ago

- for you first point, this is not true[0]. can you provide a reference? - for your second point, if you want to fork this repo and use another algo, go for it. if you install the package from npm and use it or clone and install from source without changing it, it uses the default AES256 - this could be argued as a valid point, and maybe forcing the user to use 32 bytes as their password is the right way to do it, but no 3rd party password manager forces this limitation so i don’t believe I should? Maybe just forcing a password difficulty is the right way to go? I’m certainly open to ideas, this is just a small project i enjoyed building and feel like the minimum security requirements to meet my needs are there. [0]: https://github.com/whatl3y/hide/blob/1c3aaca634f504c3c274bac6a184b8783cd72df8/src/index.ts#L18 https://github.com/whatl3y/hide/blob/1c3aaca634f504c3c274bac...

staticassertion 6y ago
For the third point, why not just hash the password with SHA256 first? Sorry, I didn't look at the code, just going off of the described issue. If you have an arbitrarily sized input and you want a fixed 32 byte output, that seems by far the simplest way.
- whatl3y 6y ago
  This is what I should do :) I simply append the password to itself to get 32 bytes, but SHA256 is almost certainly the better/right way. Will add a PR this weekend.
  - staticassertion 6y ago
    Cool! Sounds like a good idea. A key stretcher is probably a good idea too if you haven't done so. Given that this is local only, you can go pretty nuts on the key stretching. For example, on a server I'd say >100K rounds of PBKDF2. For local, you could do 1B.
  - jlgaddis 6y ago
    A single round of SHA256 used as a master password is definitely NOT what you "should" do. --- (I apologize in advance. I don't typically make comments like this on HN but I feel it really is warranted here.) Key Derivative Functions (KDFs) have already been mentioned here in the comments. By not using one -- and, perhaps more importantly, by 1) not even understanding why you should use one and 2) thinking that one round of SHA256 is what you "should" do -- well, to be blunt, your project just screams out "hacked together piece of shit made by someone who has absolutely zero business whatsoever making a password manager". --- "Show HN"'s are to get feedback. As it stands, your project should not be used -- by pretty much anyone! It doesn't have to stay that way, though. Please take some of the good advice you've gotten in the comments here and improve your project so that it can safely be used.
    - whatl3y 6y ago
      To anyone else who reads this I’ve made several changes to the code based on feedback presented in previous comments so far including using PBKDF2 for key derivation (actually well before jlgaddis’ comment here was posted which is unfortunate, sort of screams “I’m lazy and before I confirm I’m going to blast this peion publically because it’ll make my Sunday a little bit better than it already was”). Nonetheless I appreciate the last bullet as I definitely post to Show HN to learn and get better as a developer, and certainly expect nothing less than to receive a few “fuck your shitty project”s along the way :)
arkadiyt 6y ago
- config.cryptography.password is always set to a default value if not provided, it will never hit your error case [0]. This is trivially verifiable and I suggest you try it. - no, you accept the algorithm as an environment variable - someone can install your package as is and still set it themselves - as kohtatsu mentioned you should be using a key derivation function. [0]: https://github.com/whatl3y/hide/blob/1c3aaca634f504c3c274bac6a184b8783cd72df8/src/config.ts#L18 https://github.com/whatl3y/hide/blob/1c3aaca634f504c3c274bac...
- whatl3y 6y ago
  - Wow apologies you’re exactly right, I just converted this from JS to typescript and instead of making this variable null | undefined | string, i made it a string and added a default as a shortcut to get it released. Apologies for questioning this will fix it asap, if nothing else I get some peer review when publishing a Show HN :) https://github.com/whatl3y/hide/blob/5ee54aad1b1c3da4d1865689286b28382c11ae1d/src/config.js https://github.com/whatl3y/hide/blob/5ee54aad1b1c3da4d186568... - Got it, yeah I guess my defense would be it still requires the user to make a conscious decision so it’s not as bad as it being ambiguous or unclear, although removing the ability to control this via env var might be the right move. - I’ll look into and add something to do this soon :)