archlinux / archinstall

Arch Linux installer - guided, templates etc.
GNU General Public License v3.0
6.2k stars 539 forks source link

Password shouldn't be written to /tmp #137

Open kpcyrd opened 3 years ago

kpcyrd commented 3 years ago

It seems instead of passing the password through stdin it's currently written to a file and then passed to cryptsetup with --key-file. This isn't as much of a problem if run from the archiso, but could be an issue in some use-cases enabled by things like #124. :)

https://github.com/Torxed/archinstall/blob/bb5caf70b7c3daae863778738775823a51b0b92b/archinstall/lib/luks.py#L52-L69

The password should be passed through stdin instead. You might have to use subprocess directly in case archinstall.lib.general.sys_commnad doesn't support that.

Discovered by @ysf.

Torxed commented 3 years ago

Well spotted @ysf and thank you for reporting @kpcyrd!

sys_command should support it now days, I don't think it did way back when I was using Popen() due to the password input being started in a separate pty, much like SSH does it. But since I use pty.fork() these days that issue is gone. The only concern now is that without a password file, bytes data can not be used as a password (read: random generators or hsm's that output byte streams) and characters like ! can cause issues if not escaped properly on the command-line. Other than those two issues, I don't see any problems in moving to a command-line parameter for the passwords :)

kpcyrd commented 3 years ago

If it's already a file anyway using --key-file is fine, if it's binary that isn't on disk yet --key-file /dev/stdin might work :) stdin can handle arbitrary binary.

progandy commented 3 years ago

man cryptsetup:

   luksFormat <device> [<key file>]
         Initializes a LUKS partition and sets the initial passphrase (for key-slot 0), either via prompting or via <key file>. Note that if the second argument is present, then the passphrase is taken
         from the file given there, without the need to use the --key-file option. Also note that for both forms of reading the passphrase from a file you can give '-' as file name,  which  results  in
         the passphrase being read from stdin and the safety-question being skipped.

You can use a custom pipe for the stdin, no need to send the password through a pty emulation.

https://stackoverflow.com/a/24514542

Torxed commented 3 years ago

Appreciate that information and link, that's a cleaner way of doing I recon! I'll bring this into the re-work of sys_command once I get to it!

Torxed commented 3 years ago

Just for my own sanity, piping the password in to stdin is the way to go. To avoid users reporting issues like this leaking the password if it's on the command line: BbU8NiV

ysf commented 3 years ago

Either this or using a custom logging formatter that masks out the password from the traceback. This is not unusual. Just make sure to build tests for the formatter first and test if it fails correctly before using it.

progandy commented 3 years ago

Either this or using a custom logging formatter that masks out the password from the traceback. This is not unusual. Just make sure to build tests for the formatter first and test if it fails correctly before using it.

Use stdin or someone will file a bug that the password is leaked through procfs and process list tools.

ysf commented 3 years ago

Either this or using a custom logging formatter that masks out the password from the traceback. This is not unusual. Just make sure to build tests for the formatter first and test if it fails correctly before using it.

Use stdin or someone will file a bug that the password is leaked through procfs and process list tools.

Am I missing something? I'd think if someone is allowed to use process list tools on the same machine while I'm using an installer, everything is lost anyway. The usecase tried to be solved is not having issues on github opened that contain user passwords.

Torxed commented 2 years ago

Hi. Just piping to let you guys know I haven't forgotten about this. And I feel archinstall is stable enough that I can start tinker with the encryption process in general.

I've started a PR (#953) to address this and should be dished out later today and ready for release in v2.4.0 :)

Torxed commented 2 years ago

/dev/stdin

Unfortunately this does not work:

Cannot read keyfile from a terminal.

Trying just a normal "type it in" approach since SysCommandWorker() supports that.

strboul commented 2 years ago

Also hand up here :hand:

After the installation, I see the plain text password in the /var/log/archinstall/install.log file:

Marking partition for use with encryption-key: {'type': 'primary', 'encrypted': True, 'format': > True, 'start': '40GB', 'size': '100%', 'mountpoint': '/home', 'filesystem': {'format': 'btrfs'}, > '!password': 'plain.text.password.here'}

Torxed commented 2 years ago

Also hand up here ✋

After the installation, I see the plain text password in the /var/log/archinstall/install.log file:

Marking partition for use with encryption-key: {'type': 'primary', 'encrypted': True, 'format': > True, 'start': '40GB', 'size': '100%', 'mountpoint': '/home', 'filesystem': {'format': 'btrfs'}, > '!password': 'plain.text.password.here'}

That's a regression, I'll fix that in time for v2.4.0. The pw in /tmp is a bit of a larger project.

Torxed commented 2 years ago

I spun off a separate issue @strboul for that issue, in order to track it as a security issue and pin it :) Thank you for reporting this!

svartkanin commented 2 years ago

@Torxed did we resolve this one?