I haven't looked at the crypto yet, but some tips as someone who's designed a few password managers[1][2]:
* Don't allow the user to supply their password via argv. The names and argument vectors of running processes aren't considered privileged information, and the password can be inadvertently leaked or cached by e.g. a process auditing system. Instead, have the user provide their password via standard input.
* Consider a different interface for the master password. An environment variable isn't the worst, but has some of the same leakage problems that argv does. "Best practice" here is to either use the system keychain or an agent-style helper program.
Using stdin also solves the problem of special characters in argv being interpreted by the shell. Even something like a hyphen might be mistaken for a flag.
If they're gonna work around your "no password via argv" security design, it might be a reasonable thing to jst let them. Anyone who goes:
hide add -n account -u user -p password123
And gets an error message saying "Sorry, you can't put the password into the command,it's a security problem in things like bash_history. Please use STDIN"
Then goes "Fuckit!":
echo 'password123' > hide add -n account -u user -p
Maybe they should be allowed to do that?
(I can almost imagine scenarios where someone might want to use a tool like this in some server automation thing where the security tradesoffs really do make using it like that a not awful idea?)
This is a good point, and I should have been more precise in my original comment :-)
If the program is on a TTY, then reading with echo disabled (i.e., `read -s`) is definitely preferable. I usually have stdin configured as a fallback for when the program isn't on a TTY, for programmatic use.
This has the `echo` problem that you mentioned, but it's strictly better than either argv or the environment (both of which persist throughout the program's lifetime and appear in places like procfs).
For the system keychain, I think we're talking about different use cases, no? The system keychain should be used to store the master password, but using it to pre-store any password you intend to later store in another password manager feels a little silly.
I like pass, and I use it. But, I don't actually use GPG/PGP otherwise, and trying to manage your passwords then gets you into managing a PGP key as well, a hassle I don't really want out of a password manager.
Pass was built by Jason Donenfield (zx2c4) who is competent enough to have also built WireGuard and survived the resulting years of uh, review by the Linux kernel team.
The parts of pass I've actually read (which is admittedly not all of it) are designed in a way that feels like the author understands both the large problem (password management) and lots of little implementation details to get there correctly.
One thing I particularly like is how pass gets random passwords. Unlike this tool 'pass' will generate you a strong random password, optionally constrained however you please. The insight is that the CSPRNG is cheap, so you shouldn't put any work into conserving random numbers by doing fancy algorithmic stuff. Instead pass consumes bytes from the CSPRNG (/dev/urandom) and just discards all the ones that aren't allowed in your password. So e.g. if you ask for a numeric password and 'pass' reads the byte 0x8F from the CSPRNG, it doesn't try to massage that into a numeral, it just discards it and fetches another byte until it gets one in the range 0x30 to 0x39. This inefficiency is actually totally fine and makes it easy to verify that the code is correct, which is much more important than slightly reducing the workload of the CSPRNG.
tl;dr I would (and do) trust pass but not this program.
Thanks for the insights, I used to use gopass but never realized that pass itself was made by the same guy who made WireGuard. I also trust Bitwarden however and I've found it a better overall manager due to it being much more convenient.
Making project is a great way to learn stuff, so congrats!
I had a look at your crypto code, and the usual advice would be not to roll out your own crypto. Of course, since we are here to learn, that does not apply, so instead here are a few pointers on things you might want to learn about:
Where is the author rolling their own crypto? They’re simply importing Node’s standard crypto library and using AES, a CSPRNG, and PBKDF2. Whether the usage is secure is another question, but I wouldn’t say this counts as “rolling your own crypto.”
The code has changed substantially since this was first posted, based on feedback in these threads. So it's bad now than it was when some of these comments were written.
That's definitely a good thing in terms of the quality of the code now by the way, I don't see any changes that make it worse and many make it better. But it does mean comments may not refer to the source you looked at.
Yes thank you for your initial comments, made some of your changes and published a new major version now using PBKDF2. Definitely am learning a lot along the way :)
In rolling your own crypto, crypto doesn't mean cryptography but cryptosystem.
The author is not rolling their own cryptography (which is good) but they are clearly using a home made cryptosystem (and not PGP, TLS, or libsodium for example). For example they forgot authenticating their encryption, and the with "password extension" getFilledSecret is more than doubvious. Rule of thumb: if you are using cryptographic primitives directly (such as AES) you are rolling out your own crypto.
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/Encrypt...
- 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.
- 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.
- 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/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 :)
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.
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.
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 :)
(This is my second attempt at "constructive criticism"; my previous attempt failed spectacularly. Sorry this ended up being such long-winded but after rushing to post my previous, off-the-cuff comment, I wanted to avoid doing that again.)
---
> ... I’ve made several changes to the code ... including using PBKDF2 ... actually well before jlgaddis’ comment here was posted ...
Yes, you're right. That's my fault, mostly because of how I tend to browse HN.
I had opened both the GitHub repo and the discussion thread in their own new tabs. I actually had browsed through the repo and briefly looked over some of your code although I will certainly admit that I didn't conduct an extensive, in depth review of it! In fact, I was still reading through it when SWMBO suddenly appeared in front of me, informed me of an urgent mission, and instructed me to begin working on it at once. By that point, I had already "seen enough" so I just closed the tab, tossed the iPad down on the couch, and headed out on my new mission.
When I picked up the iPad again a couple hours later, Safari reloaded the tab that was open (this discussion thread), I began reading the comments, and, after reading your comments upthread, felt compelled to leave one of my own.
---
Anyways...
The point that I had intended to make was that, sometimes, a little knowledge (or, alternatively, "a lot of ignorance") can be a very dangerous thing. I'm gonna digress real quick but I'll come right back to that.
I didn't intend for my comment to be a personal attack on you and I'm sorry that it came across that way. There really was a legitimate point that I was trying to get at; the "bigger picture" that I was attempting to paint, if you will.
I can, however, be (in the words of an old co-worker and more than one student) a bit of a "security nazi" at times. Unfortunately, this was one of them. My knee-jerk reaction to someone "doing something stupid" was a hastily written comment which resulted in a poor choice of words that failed to paint that "bigger picture".
First, it's certainly a good thing that you took the direct advice you were given (i.e., "use a KDF!") and quickly improved upon your new project. Props for that. Presumably, tt's now much more secure as a result (I haven't actually looked at your changes but I'll give you the benefit of the doubt)!
Now, back to that "a little knowledge can be a very dangerous thing" thing.
When you start getting into security-related areas, simple mistakes can have a devastating, outsized, and lasting impact. There's no doubt that a password manager provides huge benefits -- if it's designed and implemented correctly! The only thing that I can think of that's worse than a complete dumpster fire of a password manager is a complete dumpster fire of a password manager that 1) the user assumes is working as expected, 2) the user trusts with their personal/private data, and 3) lulls the user into a false sense of security -- since they might actually be worse off (i.e., "less safe") than they were before!
Ultimately, the "real problem" is that you were developing a tool that claimed to "securely" store secrets without actually knowing how to "securely" store secrets at all! There are several well-tested, industry-standard algorithms designed to do exactly this -- yet, for whatever reason, you instead devised something that sounded okay and seemed like it might kinda sorta work. It quickly became clear from reading your other comments that you had good intentions; it was simply ignorance, not malice.
Unfortunately, when it comes to security-related stuff, it doesn't really matter if it's ignorance or malice -- the end result is the same. It's like handing a loaded AR-15 to someone who has never handled a gun before and telling them to have fun, or letting a little kid set off the fireworks on the Fourth of July: you may get lucky and nothing bad happens; eventually, though, someone's probably going to end up getting hurt and/or missing a few fingers!
---
> * ... I’m going to blast this peion [sic] publically [sic] because it’ll make my Sunday a little bit better ...*
To be clear, I wasn't trying to be condescending or imply that you're a "peion", or stupid, or a shitty developer, or anything like that. If you're creating something that has major security implications, however, you really do need to know what you're doing. If it makes you feel any better, I'm certainly not qualified to develop a password manager capable of securely storing passwords (and I wrote my first program sometime in the late 80's)! That's not to say you won't ever be capable of creating one, of course -- but, if you're going to be developing software that has security implications (especially for other people!) if you get it wrong, please make sure you learn how to do it right.
Completely fair points, thanks for taking the time to clarify your intent and meaning in your previous comments. I also understand and get the points about ignorance vs malice and it not mattering which is present wrt a security tool to be used by others. I admit I’m not a cryptography “expert” in that I couldn’t regurgitate low level details for any industry standard algo, although I do like the think so know what should be used and when (for the most part).
If nothing else I’m smarter and know a little more about what I didn’t know (and still don’t) which I find a ton of value in. Hopefully I’m slowly ridding the world of my contribution to the “ignorance” category you mention above which I admit to have been in wrt developing a secure password manager, but either way I enjoyed learning along the way.
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.
1. Why are all the output lines in the examples in the "Usage" section prefixed with '# '? The output lines in the examples later for individual commands are not prefixed that way.
2. It might be useful to have an option for show to just show the password. Perhaps -P (upper case P). E.g.,
$ hide show -n facebook.com -P
my_password1
I'd expect that most of the time people use -p to retrieve the password they are going to copy/paste it into a browser. With a password-only option they could pipe to something that puts it on the clipboard. A Mac user, for exampled, could use
$ hide show -n facebook.com -P | pbcopy
It would also be useful with commands line tools that take a password on the command line:
$ some_tool --password=$(hide show -n some_tool -P)
or with commands that read the password from a file
I removed the hash’s from the output and definitely love the feedback to support piping the password only to other tools. Will introduce something like this asap.
> You don't need to compare, just feed the resulting hash to AES and see if the output is any good.
You need to arrange to store the salt somewhere to take this (correct) approach to using a PBKDF. The nodeJS bcrypt implementation provides an all-in-one API which was used here, in which you let it handle salting and it "just works" by storing passwords in the style of Unix crypt - but without you understanding why. This probably results in fewer nodeJS apps where the passwords are stored as plaintext or something dumb, but it doesn't afford the understanding you'd need to get encryption keys the same each time a user runs the program.
My guess is that the author realised they didn't understand what was happening here, and so they ripped it out perhaps intending to reintroduce it later once they had other parts working to their satisfaction, and then they never did.
You are correct though that in the absence of a good PBKDF bad guys who get your encrypted password database can "just" brute force the master password relatively inexpensively and this makes the program unsafe for essentially all ordinary users.
If someone pwns your m/c they can "search" and get all password right, what am I missing? May be you should prompt for the master key instead of exporting? Or you are depending on the fact export goes away with the term session?
That’s correct, in most envs exports go away when the session terminates. I guess prompting each command could work, but the UX for that would be awful. I feel like the main concern you’re pointing out “if someone gets hold of your master key all your accounts are compromised” is the same issue any password manager like LastPass has. At least this tool everything is on your machine and you control it instead of a 3rd party. The encrypted flat file is portable and can be sent to other machines or servers as needed as well.
Ive got to say. The title looked interesting. But when I see that .vscode has been checked in, I lose a bit of confidence that this is going to be 100% secure.
I’m interested why you would lose confidence in the tool simply because of that? I check it in because it contains the workspace settings I prefer in my dev env, there’s nothing sensitive in there and its nice to remain portable if I clone the repo on another machine. Losing confidence for something in my code is bad is one thing, but because I did something that doesn’t suit your personal preference seems like a poor way to gauge confidence in a tool.
I have been using keypass for years. I have a flash drive with a key file and keypass software that gets synced to my personal drive everytime I plug in.
I plug it in, enter my master password and right click to copy password to paste it on websites.
Yeah after being an early customer who bought both the mac and iPhone licenses, I felt offended with the new plans/paid upgrades and decided to vote with my wallet.
I prefer the inconvenience of using an old version (which I have a license for) than paying the extorsion price of a paid only update, and with the nonsense of different platforms being sold separately.
I guess they're free to do whatever they want, and I guess sadly nowadays subscriptions do make sense from a SaaS/Developer POV, I just can't fathom it as consumer.
* Don't allow the user to supply their password via argv. The names and argument vectors of running processes aren't considered privileged information, and the password can be inadvertently leaked or cached by e.g. a process auditing system. Instead, have the user provide their password via standard input.
* Consider a different interface for the master password. An environment variable isn't the worst, but has some of the same leakage problems that argv does. "Best practice" here is to either use the system keychain or an agent-style helper program.
[1]: https://github.com/kbsecret/kbsecret
[2]: https://github.com/woodruffw/kbs2