[MIR] freerdp

Looks good to me. Would be nice to see someone subscribe to Ubuntu bugs though.

Throwing to security team for a final check.

NACK. This is unacceptable for anything that uses encryption:

libfreerdp/crypto_openssl.c:

RD_BOOL
crypto_cert_verify(CryptoCert server_cert, CryptoCert cacert)
{
        /* FIXME: do the actual verification */
        return True;
}

I didn't look any further than this; it implies a grievous lack of attention to security.

I am working with upstream on this, will hopefully have a patch soon

Have had an attempt at a patch, but I'm not sure exactly what needs to be changed to make it safe. Here is the conversation I had with upstream:

On 27/01/11 23:42, Mads Kiilerich wrote:
> > On 01/27/2011 04:51 AM, Robert Ancell wrote:
>> >> Hi FreeRDPers!
>> >>
>> >> I'm currently trying to get Remmina/FreeRDP as the default on
>> >> the Ubuntu 11.04 CD, but our pesky security team wants the
>> >> certificate checking to work:
>> >>
>> >> RD_BOOL crypto_cert_verify(CryptoCert server_cert, CryptoCert
>> >> cacert) { /* FIXME: do the actual verification */ return True; }
> >
> > I assume this is from crypto_openssl.c and that you don't care
> > about other crypto backends. Ok.
> >
> > This function is only used to verify the individual links in the
> > x509 certificate chain is correct. That alone is far from enough.
> > Note however that this part works with the gnutls backend.
> >
> > Finally (so far) there is the tls option. libfreerdp/tls.c (which
> > so far only works with openssl) is far more complete but still not
> > completely finished.
> >
>> >> So the question is: - - Any chance of this working by the end of
>> >> February? - - Any plans for this? - - If you guys haven't got
>> >> plans, I'll work on a patch. I'm not an expert at certificate,
>> >> do I just need to pass the information to the GUI and let the
>> >> user ACK/NACK it?
> >
> > AFAIK there are no specific plans and no chance unless somebody do
> > something.
> >
> > I think FreeRDP is quite stable and reliable on local trusted
> > networks, but I wouldn't recommend using it on untrusted networks
> > or when connecting to untrusted servers. FreeRDP security in these
> > (and other) areas is definitely not worse than rdesktop (which I
> > assume is the only alternative).
> >
> > It would be great if you could work on improvements in this area.
> >
> > A brief description of some aspects of a good solution could be: *
> > options for warning/accepting/failing on "Proprietary Certificate"
> > * more common handling of certificates for tls and non-tls *
> > support more crypto backends for tls (and nla) (but focusing on
> > openssl first is fine) * checking that the server certificate
> > matches the request hostname * functionality for checking that the
> > x509 chain can be validated with the systems CA certificates
> > (probably only useful in very few setups) * functionality for using
> > other CA certificates (so you can add your local AD CA and
> > automatically trust all servers on the domain regarding rdp without
> > adding it to the global configuration) * ssh-like "known host"
> > functionality, asking "unknown host X shows certificate Y - trust
> > it and store it to next time?", adding it to some "known_hosts"
> > file and using it next time and failing/prompting if it doesn't
> > match next time
> >
> > It will require changes to both libfreerdp and xfreerdp and will
> > thus also require a so version bump.
> >
> > Not a trivial task ... It might make sense to focus on "known
> > host" and ignore the PKI mess. That might bring you most of the way
> > to what you want.
> >
> > /Mads
Hi Mads,

Thanks for the information. Yes, we would be switching from rdesktop
...

Generally the TLS verification should be automatic, as detailed in upstream's reply (i.e. performing proper CN validation via the known CA certs, check for NULL bytes, etc). In the case of a mismatch, then, yes, it should go to the UI.

I still don't think anything that claims to be TLS enabled should go into main if it does not securely handle TLS. We can't control where people connect to, so we can't claim TLS should only be used for "trusted networks".

OK, so I think we can disable TLS - then it uses OpenSSL right? Can you see how this compares to rdesktop (which we will continue to provide if we don't switch to freerdp).

TLS is a secure protocol. Both OpenSSL and GnuTLS implement it. (Disabling
TLS means disabling support for encrypted rdp communication.)

Moving back to New status. I'll work on this more for Oneiric.

Status changed to 'Confirmed' because the bug affects multiple users.

Targetting to precise alpha-2, as it was requested to switch to remmina in https://blueprints.launchpad.net/ubuntu/+spec/desktop-p-default-apps. What is still left here? Can we disable TLS entirely for now? Did rdesktop have TLS support without proper verification?

So, freerdp 0.8.2-2build1 is still not fixed when compiled to use openssl, which is how it is compiled in Ubuntu:
crypto_cert_verify(CryptoCert server_cert, CryptoCert cacert)
{
        /* FIXME: do the actual verification */
        return True;
}

crypto_nss.c is no better. crypto_polarssl.c is slightly better in that it at least returns False. crypto_gnutls.c is slightly better with:
crypto_cert_verify(CryptoCert server_cert, CryptoCert cacert)
{
        /* FIXME: check more here ... */
        unsigned int verify; /* What is this? */
        return gnutls_x509_crt_check_issuer(server_cert->cert,cacert->cert) &&
                        gnutls_x509_crt_verify(server_cert->cert, &cacert->cert, 1, 0, &verify) == GNUTLS_E_SUCCESS;
}

But this is not good enough (see http://www.ubuntu.com/usn/usn-1283-1/ as an example why). http://www.gnu.org/s/gnutls/manual/html_node/Simple-client-example-with-X_002e509-certificate-support.html gives an example on how to properly verify a certificate (though admittedly, it doesn't do the whole cert chain so more effort would be needed there-- but lack of full chain checking shouldn't block this MIR because it should fail safe in that regard).

So to move this along:
 * NAK when compiled against openssl (as it is now)
 * NAK when compiled against nss
 * NAK when compiled against gnutls, but this is more easily fixable with the above info and if the userspace bits are in place
 * ACK if disable SSL support

The gnutls bit alone could easily get a CVE *right now* and the bits for openssl and nss are egregious in production code *if* the code is claiming secure connections (which in looking in remmina, it appears to be). These SSL issues are very important to fix because if the program claims to have secure connections, people will trust that. We've had too many security updates on SSL issues over the years because of improperly implemented certificate verification. As such, we can solve this easily by not claiming SSL support at all, which I think is the easiest way forward. If work is going to be put into fixing the gnutls bits please follow the above example.

As for rdesktop, afaict it doesn't seem to claim support SSL in its manpage or its online documentation and there is nothing in vinagre that exposes this, so there is no expectation of security.

Guys, please, stop assessing 0.8.2! We're working hard on getting FreeRDP 1.0 ready for inclusion in Precise, with all the changes you have required. The 0.8.x development branch is deprecated and no longer maintained. We are now in FreeRDP 1.0 beta5, and we should be able to make the stable release this month. We had proper certificate checking for a while now, but it won't magically appear in 0.8.x, you need to be taking a look at 1.x.

Now once you see that 1.0 does fit your security requirements, we can work on making sure we have the stable release ready for your deadlines, instead of wasting time deciding if it can be included or not. In 1.0 the method you need to take a look at is tls_verify_certificate in libfreerdp-core/tls.c: https://github.com/FreeRDP/FreeRDP/blob/master/libfreerdp-core/tls.c

When connecting, the user is prompted for accepting/denying a certificate if it cannot be validated by OpenSSL. It works just like SSH, once you accept the certificate it gets added to ~/.freerdp/known_hosts.

For the record, freerdp 1.0 is packaged in http://anonscm.debian.org/gitweb/?p=collab-maint/freerdp.git, just waiting for the Debian maintainer to upload it.

I just uploaded freerdp 1.0, thanks to Jeremy Bicha for packaging it in Debian's git. So this is no longer blocked, 1.0 should address the SSL verification issues that came up earlier.

Can you please MIR review this again? Thanks!

Moving to beta-1 milestone, as alpha-2 is today. But I'd appreciate a review well before beta-1, as we also need some time to update to a new remmina once this is in main.

Ok, I reviewed 1.0 and it looks much better. I'm ok with the fallback to something in ~/.freerdp/cacert/ and that directory is created with safe permissions. The certificate verification is fine except for lack of server hostname checking (see below).

A few comments:
* I find it interesting that X509_* functions were used rather than SSL_* (see man X509_verify_cert), but that is not a problem in and of itself
* might be nice to use X509_STORE_CTX_get_error() on X509_verify_cert() error
* x509_verify_cert() and tls_verify_certificate() do not verify the server's hostname. I filed a separate bug #925657 for this.

ACK for main inclusion once bug #925657 is fixed.

I synced 1.0.1 from Debian, which should fix bug 925657. Good to go to main now?

Yes. I reviewed the fixes in bug 925657. ACK for main inclusion.

Promoted to main.

FYI, I need to revert the seed change for putting this into the desktop seed for now, due to bug 931931. Any ideas how to solve this are appreciated.

On 02/14/2012 09:39 AM, Martin Pitt wrote:
> FYI, I need to revert the seed change for putting this into the desktop
> seed for now, due to bug 931931. Any ideas how to solve this are
> appreciated.
>
Let's recompile without the multimedia redirection for now. It's not an
important feature. Anyway, rdesktop didn't have it, so we are not worse off.
I will talk to the project leader later today when he is up and ask him
to add a feature request for modifying the multimedia redirection to use
gstreamer.

Stephane

re-opening. what is the status about the multimedia redirection?

remmina was promoted in bug 673908 without mentioning this issue. Martin?

Is this link the new gstreamer multimedia redirection functionality?

https://github.com/FreeRDP/FreeRDP/pull/627

Yes, it is.