FiloSottile / age

A simple, modern and secure encryption tool (and Go library) with small explicit keys, no config options, and UNIX-style composability.
https://age-encryption.org
BSD 3-Clause "New" or "Revised" License
16.9k stars 495 forks source link

UX: Redirecting age-keygen STDOUT provides inaccurate error message #149

Closed breadtk closed 3 years ago

breadtk commented 3 years ago

What were you trying to do

Redirect the STDOUT output from age-keygen to a file.

What happened

A warning message that suggested failure has occurred. It notes: "...and trying again" when it actually generated a key and wrote it out successfully.

$ age-keygen > ~/mysecret
Warning: writing to a world-readable file.
Consider setting the umask to 066 and trying again.
Public key: age1l07jr8524hmuer...
$ echo $?
0

Suggestion

Change the warning message to be something more helpful like:

$ age-keygen > ~/mysecret
Warning: writing to a world-readable file. Consider setting the umask to 066 to make it only readable by you.
$
breadtk commented 3 years ago

This issue is similar to #75 in as far as the warning message can be confusing to some.

Tharre commented 3 years ago

Your revised warning message misses the point. Writing to a world accessible file has already happened and as such, the key may be compromised already. The only safe course of action in that case would be setting the umask appropriately and generating a new key.

Also setting the umask to 0400 as you suggest would not lead to the desired result, the file would still be world accessible, I'm guessing you're confusing umask with mode, something that was already mentioned in #75:

$ umask 0400
$ age-keygen > ~/mysecret
$ ls -lha ~/mysecret
--w-rw-rw- 1 tharre tharre 45 Oct 12 03:40 ~/mysecret

I cannot think of a better warning message for redirecting to a file, but perhaps the correct thing to do here is just to tell the user to use age-keygen -o instead, and hope the ones that really need redirection already know what they're doing?

breadtk commented 3 years ago

Writing to a world accessible file has already happened and as such, the key may be compromised already

I see your point, but don’t let perfect be enemy of the good. It’s unlikely for a key to be leaked from the time that the write occurs and when the permission is fixed (assuming the user event wants to do that). If an attacker has a hook on arbitrary write() calls, then the user likely has larger concerns to contend with.

..setting the umask to 0400 as you suggest would not lead to the desired result...I'm guessing you're confusing umask with mode...

You are correct, I was confusing the two. I'll edit the original post accordingly.

I cannot think of a better warning message for redirecting to a file, but perhaps the correct thing to do here is just to tell the user to use age-keygen -o instead

If the purpose of the 2nd error line is to inform the user that the key has already likely leaked (something I disagree with per above), then perhaps a better warning message would be:

Warning: writing key to a world-readable file. Consider deleting the key, setting umask to 066, and then running age-keygen again.

Or:

Warning: writing private key to a world-readable file. Consider fixing the file's permission and run age-keygen again.

Or (my preferred):

Warning: writing your private key to a world-readable file.

Tharre commented 3 years ago

If an attacker has a hook on arbitrary write() calls, then the user likely has larger concerns to contend with.

If the attacker indeed has user-level access already (because it's a multi user system or he exploited a non-root daemon or whatever) then he can trivially hook any write() call on any files that are readable to him via the linux inotify API. There are no special additional capabilities needed for this attack.