Freerdp: Supplying passwords via command line argument (e.g. /p:<password>) is broken by design

[martinvonwittich]

The currently recommended method to pass passwords to FreeRDP seem to be several command line arguments that take the respective password as an argument, as documented in the usage text:

martin@dogmeat ~/src/FreeRDP % ./client/X11/xfreerdp | grep -i pass
    /pth:        Pass the hash (restricted admin mode)
    /p:               Password
    /gp:              Gateway password
    /reconnect-cookie:   Pass base64 reconnect cookie to the connection
    /assistance:      Remote assistance password

martin@dogmeat ~/src/FreeRDP % ./client/X11/xfreerdp                                                                             
[...]
Examples:
    xfreerdp connection.rdp /p:Pwd123! /f
    xfreerdp /u:CONTOSO\JohnDoe /p:Pwd123! /v:rdp.contoso.com
    xfreerdp /u:JohnDoe /p:Pwd123! /w:1366 /h:768 /v:192.168.1.100:4489
    xfreerdp /u:JohnDoe /p:Pwd123! /vmconnect:C824F53E-95D2-46C6-9A18-23A5BB403532 /v:192.168.1.100

As most people probably know, this is very unsafe because the password is visible in the process table, e.g. in ps aux. At some point a workaround was added for this problem that replaces the password in the process name with asterisks: #829

Unfortunately, this workaround suffers from two issues:

1. An attacker can still learn the password length from the process table, which might speed up a brute force attack.
2. It is a race condition vulnerability: an attacker might still be able to read the password from the process table if he's fast enough, because there is a small time window between the process creation and the point where the FreeRDP command line parser overwrites the password. If the attacker manages to read the command line from the process table in this time window, he's able to read the password.

I've written a small Perl script that can demonstrate the issue (required the libfile-slurp-perl package to be installed on a Debian system):

#!/usr/bin/perl
use warnings;
use strict;
use File::Slurp;
use Scalar::Util qw(looks_like_number);
my %proc;

sub read_proc($)
{
  my $print = shift;
  my %old_proc = %proc;
  %proc = ();
  opendir my $dh, '/proc' || die "can't opendir";
  while (readdir $dh)
  {
    next if $_ eq "." || $_ eq "..";
    next if !looks_like_number($_);
    $proc{$_} = 1;
    if (!$old_proc{$_} && $print)
    {
      for (my $i = 0; $i < 100; $i++)
      {
        my $contents = read_file "/proc/$_/cmdline", err_mode => "carp";
        next if !defined $contents;
        $contents =~ s/\0/ /g;
        if ($contents =~ /\bxfreerdp/ && $contents !~ /\/p:\*$/)
        {
          print "$contents\n";
          last;
        }
      }
    }
  }
}

read_proc 0;

while(1)
{
  read_proc 1;
}

The script will print FreeRDP passwords after a FreeRDP process has been spawned:

martin@dogmeat ~ % ./test.pl
xfreerdp /cert-ignore +clipboard /u:martin /bpp:8 /size:1680x1050 +compression -wallpaper -menu-anims -themes /v:10.2.56.101 /p:myverysecretpassword 

martin@dogmeat ~ % xfreerdp /cert-ignore +clipboard /u:martin /bpp:8 /size:1680x1050 +compression -wallpaper -menu-anims -themes /v:10.2.56.101 /p:myverysecretpassword
loading channel cliprdr
unable to connect to 10.2.56.101:3389
Error: protocol security negotiation or connection failure

There is the /from-stdin option, but it interferes with other parts of FreeRDP which might want to read from STDIN (for example the "Do you trust the above certificate?" prompt, though this can be worked around with /cert-ignore) and it only supports /p passwords and not the other switches like /pth, /gp and /assistance (at least as far as I can tell from the source, I haven't actually needed any of these switches yet).

There is a patch from @rkeene in #2904 that adds support for reading passwords from an environment variable, but this apparently hasn't been added yet. I'd suggest the following changes:

• add support for reliably and securely passing passwords to FreeRDP, with environment variables and files. Environment variables should be unset by FreeRDP as soon as the password has been read so they can't be accidentally dumped or passed to subprocessed, and files should probably cause a warning message to be printed if they are world-readable.
• deprecate (but don't remove) the old /p etc. options and cause them to print warnings
• update the documentation so that it demonstrates how to pass passwords securely

[ilammy]

How environment variables are any different from sanitized command line arguments from the security standpoint? They are also (usually) readable from /proc/pid/environ by any process with the same EUID, they are also susceptible to the same race condition if they are unset at some later point, and the user also needs to consciously avoid spilling them into their shell history. The files with plaintext passwords are also not that much secure as they are obviously readable by any process launched by the same user. Anything else requires support for a particular protocol for retrieving the password directly into the process memory.

I believe the /p option and the like are desiged for case when the user fully understands the security implications of passing the password via command line. This is inherently less secure than requiring a human user to physically type the password on the terminal or on the login screen of the remote session. The current approach gives a reasonable degree of protection from a casual explorer.

I think it's better to require something like Network Level Authentication if passwordless remote authentication is really necessary.

[akallabeth]

@martinvonwittich The password handling from stdin is currently implemented for passwords (user and gateway) only. A pull extending that functionality to the other authentication tokens would have my vote for a merge.
To your suggestion with environment variables:
As @ilammy already pointed out from a security standpoint they can be considered the same as passing them via command line, aka VERY insecure as they are part of public process variables.

[martinvonwittich]

@ilammy, @akallabeth: /proc/<pid>/environ is only readable by the process owner and by root, and noone else (at least on Linux, not sure about other Unixes)), while /proc/<pid>/cmdline (and therefor also the cmdline from ps aux) is world-readable:

martin@martin ~ % sleep 1m &
[1] 22111
martin@martin ~ % ll /proc/22111/environ 
-r-------- 1 martin martin 0 Apr  4 22:23 /proc/22111/environ
martin@martin ~ % ll /proc/22111/cmdline 
-r--r--r-- 1 martin martin 0 Apr  4 22:23 /proc/22111/cmdline

I'd argue that this makes passing secrets via environment variable much safer than passing secrets via command line. I'm not worried about attacks originating from my account (or root) - it's probably impossible to defend against that in any case, because if the attacker has already gained access to my account, he could just wrap xfreerdp in a shell script to collect my secrets (or use strace/gdb/...). I am concerned though when other users on the same system can so easily read my secrets, because that absolutely shouldn't be possible.

and the user also needs to consciously avoid spilling them into their shell history.

Yes. but the history also isn't readable by other users, so this isn't really a problem. There are also scenarios where you could use variables to pass secrets where the history is entirely irrelevant, for example when some other program wants to invoke FreeRDP and pass a secret securely.

[akallabeth]

@martinvonwittich There are other aspects of environment variables ;) they are persistent until you reboot/close the shell. As already mentioned, the functionality to provide secrets via stdin (and not by command line) is currently the best option as it is not readable from anything outside the process (except root of course, if you scan memory)

[martinvonwittich]

There are other aspects of environment variables ;) they are persistent until you reboot/close the shell.

Well, that is easy to solve:

martin@dogmeat:~$ password=swordfish sh -c 'echo $password'
swordfish
martin@dogmeat:~$ echo $password

FreeRDP could also unset secret variables as soon as it has read the secrets which would prevent accidental propagation to child processes. But I think my original point still stands - environment variables are still safer than command line arguments.

As already mentioned, the functionality to provide secrets via stdin (and not by command line) is currently the best option as it is not readable from anything outside the process (except root of course, if you scan memory)

Yeah, sorry, I had inadvertently read over your comments regarding stdin. You are of course right in that stdin is secure too, but as soon as you want to pass multiple different secrets via stdin, you'd have to come up with some kind of syntax to differentiate between the separate secrets, or am I overlooking something obvious here? And when you create a custom syntax, you also have to think about stuff like quoting... and before you know it, you're essentially reinventing the wheel. These problems have all been solved a long time ago, and the solution is called "environment variables" :)

[akallabeth]

@martinvonwittich Well, for stdin it is quite simple. Each secret is separated by a newline. The sequence is fixed (which should be documented somewhere, but still)

[martinvonwittich]

@akallabeth but what if a secret contains a newline? Then you'd have to quote the newline. (I don't know if there's ever a legitimate reason for a secret to contain a newline, but I think it's generally good design to anticipate such corner cases before they can cause bugs or security vulnerabilities.)

And what if the caller only wants to provide a certain secret and still wants to be prompted for other secrets? E.g. he wants to pass the gateway credentials via stdin, but still expects to be prompted for the user credentials, or the other way around? (Full disclosure: I've never used RDP gateways, so I'm not entirely sure if my example actually makes any sense.) To properly differentiate between secrets in cases like these, simply separating them with newlines probably won't work.

[akallabeth]

@martinvonwittich No secret contains a newline (how do you want to enter that in any GUI?) The other case you mention is no issue at all, then the wrapper script that provides the data via stdin just asks the user for that password...

[ilammy]

how do you want to enter [a newline] in any GUI?

Indirectly, via u+0010 or Alt+010, or whatever your system supports for inputting Unicode via multiple key presses.

[ilammy]

Well, if one thinks of the possibility of learning the information about the password by _other_ users in the system then @martinvonwittich's point holds well: environment variables _are_ more secure way to pass the password to the FreeRDP process in comparison with command line arguments.

As for child processes, I do not think that FreeRDP spawns any. There are CreateProcess functions in WinPR, but they don't seem to be used anywhere except for their tests. However, I believe we still should unset the variable containing the password just in case. This brings one caveat with unsetenv(): it is not thread-safe and should be called before any threads are spawned. Also it will not flush the password from /proc/pid/environ, but there is no way around that.

[ilammy]

Though, I'd still argue for files being a better way than environment variables for passing secrets. They are certainly not going to be implicitly passed to the child processes, they are certainly not visible in process information, and they are certainly can be made non-readable by other users.

[driehuis]

The most secure way to deal with passwords is to read them from a designated file descriptor. Gpg to this end has the --passphrase-fd nnn command line option. Here's a shell script example of passing a password into gpg:

exec 7<passwd
gpg -c -passphrase-fd 7

For me, --from-stdin is sort of workable, but if FreeRDP could be modified to be able to specify the fd to read from, that would clean up my code. A backward compatible syntax could be

--from-stdin:[force]:[fd]

where fd is a numeric file descriptor id. I'd be happy with --from-stdin:[fd]:[force] as well, or even a new option --credentials-fd n.

[akallabeth]

Maybe a good idea to allow such a parameter.
Could be extended to allow all command line parameters to be read that way.

[astrand]

I agree that this is an important issue which needs to be fixed before 2.0. Here are my thoughts:

• I don't like the idea of environment variables, due to the problems mentioned above. Transferring the credentials over a FD is preferred.

• Mechanism: "stdin" is a FD (0), so why is there a need to use some other file FD? gpg has a "passphrase-fd" option, but it also has a lot of other features using FDs, such as "command-fd". But FreeRDP does not have that.

• Protocol; what should be transferred. The /from-fd implementation where all options can be transferred is certainly interesting, but IMHO quite complex and somewhat "odd". I don't think I've seen any other Linux programs using this approach. Another drawback, if I've understand it correctly, is that it is not interactive or event driven: The "Do you trust the above certificate" prompt text will not be transferred, so the program supplying the FD cannot display the certificate to the user. You need to know in advance if you want to use /cert-ignore or /cert-tofu etc.

My suggestion is instead that FreeRDP should support the "askpass" approach. It is commonly used in Linux and supported by for example:

• git
• ssh
• sudo
• wget (since 2016-09-03)

Since the prompt is given as arguments, it is possible for the askpass program to determine if it should present a GUI prompt to the user, automatically supply an existing known password etc, but looking at the prompt text. This allows for maximum flexibility, without having to invent a "pipe protocol" with explicit prompt tagging etc. There are multiple implementations. For example, "ksshaskpass" integrates with KWallet in order to remember passwords.

[akallabeth]

@astrand the from-fd approach allows piping, so a echo foo |xfreerdp /from-fd will work. Askpass would be a great extension to that though.

[astrand]

Unfinished work:
https://gist.github.com/astrand/4eada81baee88d1f352c85a910a153cd

[vermgit]

I don't know if it helps in all situations (and I hope it's on-topic :) ), but I also didn't like the password in the command line. However, I learned that I can use /sec:tls instead of the /p-switch, which at least helps when connecting to Windows machines. What it does is it shows the typical Windows login form where you enter the password as you'd be when sitting in front of the machine. You can still preset the username with /u.

Maybe a different approach to the matter...