make sure the private key is never world-readable #2979
Conversation
vszakats
commented
Jun 4, 2020
•
vszakats
commented
- Set the private key to chmod 0600 on creation.
- Avoid possible race condition by setting the private key backup to 0600 first and then saving the key.
- Reset existing backup key to chmod 0600 before saving the new key.
1. Set the private to chmod 0600 on creation. 2. Avoid possible race condition by setting the private key backup to 0600 _first_ and then saving the key. 3. Reset existing backup key to chmod 0600 before saving the new key.
|
Is there any interest in (or concern with) this? |
|
Just another user, but this seems in line with best practice for storing keys on Unix, AWS even enforces this. |
| @@ -5265,10 +5267,12 @@ _installcert() { | |||
| cp "$_real_key" "$_backup_path/key.bak" | |||
| fi | |||
| if [ -f "$_real_key" ]; then | |||
| chmod 600 "$_real_key" | |||
There was a problem hiding this comment.
don't chmod if the key file is already existing.
There was a problem hiding this comment.
One of the goals of this patch is to avoid leaking the private key via wrongly set file mode. Without this command, the new private key saved in the subsequent cat command may end up in a file that's world readable (if someone set them that way between two acme.sh runs). If somebody is intentionally willing to leak the private key, the "best" solution is to copy it somewhere else, but IMO acme.sh's correct behaviour is to always make sure that a saved private key don't end up in a world-readable file.
There was a problem hiding this comment.
(if someone set them that way between two acme.sh runs). If somebody is intentionally willing to leak the private key, the "best" solution is to copy it somewhere else,
No, we should chmod only if the file doesn't exist. If the user changed the chmod, we should respect it.
There was a problem hiding this comment.
You assume that the user does this intentionally and that the user is an actual person. I'd argue this is easier done by mistake by a human user or can be made by buggy tools (including bugs in acme.sh itself). acme.sh is a tool designed to create a secure and unattended way to update and handle private keys, so I think it's rightly expected that is enforces security around the files it manages. Also note that the user is not expected to tamper with files managed by acme.sh (and similar automated tools.)
There was a problem hiding this comment.
If the key file doesn't exist, we create it and chmod to 600. That's ok.
But if the key file already exists, we should not chmod to the file. We MUST respect the user's choice.
There was a problem hiding this comment.
private key files are recommended to be 600, but remember that it's just a normal file, it can be any mod.
What the mod is MUST meet the user's needs.
What meets the needs is the best choice.
There was a problem hiding this comment.
what acme.sh does is never to make/change choices for the user. Instead, we respect the user's choice MOST.
There was a problem hiding this comment.
You're repeating this notion of respecting the user's choice, but you're ignoring all the issues I've mentioned above that arises from this, in this particular case, including the fact that this isn't a user created file, nor is it a "normal file". Note that e.g. ssh will even fail with an error unless a private key is 0600. acme.sh is lucky that most web servers aren't such picky about this.
To me it looks like acme.sh is purposefully designed to be insecure and for some unknown reason maintainers want to keep it that way.
There was a problem hiding this comment.
but you're ignoring all the issues I've mentioned above
Not true. I didn't ignore the issues. And I said I agree with you that the key file can be set to 600 when we first create the file.
The only argument between us is that: whether to set the key file to 600 every time we renew the cert.
You insist to set to 600 every time, no matter the file is already existing or not.
My point is not to do so if the file is already existing.
And I don't agree with you: acme.sh is secure enough.
acme.sh is lucky that most web servers aren't such picky about this.
you know why is this? why most web servers aren't such picky?
There must be a balance between Secure and Convenience.
100% secure is not always the best choice in all the cases.
There was a problem hiding this comment.
Okay, so I understand you confirmed that acme.sh is purposefully designed to be insecure, and there is no desire to fix this issue for existing installations and existing private keys.
| cat "$CERT_KEY_PATH" >"$_real_key" || return 1 | ||
| else | ||
| cat "$CERT_KEY_PATH" >"$_real_key" || return 1 | ||
| touch "$_real_key" |
There was a problem hiding this comment.
why do we need to "touch" it first ?
There was a problem hiding this comment.
touch is used to create a zero-length file. Without this zero-length file, the subsequent chmod command would fail.
There was a problem hiding this comment.
the "cat" command can create the "$_real_key" file. No need to "touch"
There was a problem hiding this comment.
The issue is that cat by itself cannot create a file with a specific mask, you can, of course, use umask
but touching then modifying the mask before writing to the file is another somewhat common way to handle this that avoids the problems with umask.
The reason this is done is to avoid the race condition where if you write the key to the file and then set the mask, an attacker could read the file after it has had the key written to it, but before the mask has been changed.
There was a problem hiding this comment.
The point is to set the mask before writing the secret content. If you set it after finishing writing the file, there will always be a moment when the content is world-readable. So, e.g. if an attacker knows the file location and hammers that path with reads in a loop, this can be exploited. This patch solves this by first creating the file with the secure mask and only then start outputting the secret content into it.
|
Since this patch was rejected by maintainers, my solution was to disable further |
|
Honestly.. this is the craziest thing I have read on GitHub.. A certificate management tool is unwilling to enforce the protection of the sacred private key file it owns? I just tested master out and it still creates brand new keys with world read access. @vszakats I think you were wrong to close and delete this, you should have at least left it open as there was willingness to at least have 'new' keys being created correctly.. which I totally agree is not enough but it is also a improvement for new users of the tool. @Neilpang based on your questions in the review it's pretty clear that you are not qualified to be reviewing a security oriented project. |
|
@skyscooby Unclosed issues were stacking up on one's profile page (at least back then, not sure if still true), so I used to close things that don't get interest/traction (or outright rejected) to declutter. The problem still seems valid indeed though. Definitely not a happy resolution, because even though you can choose a different client (certbot does support ECC by now), but acme.sh is being used as a dependency by other projects, so it's a security issue one can fall into without being aware. I'd gladly reopen, but the option isn't available anymore (likely because I deleted my fork with the patch). |
|
here is one of my test cert, the key file is 600: |
