[precise] freerdp does not check the server's hostname when verifying ssl certificates

Hi,

I just worked on refactoring, simplifying and cleaning up the certificate validation code. It's now much easier to read but it's not fixed with regards to this bug report.

tls_verify_certificate() first uses x509_verify_certificate() to attempt to validate the certificate using OpenSSL and ~/.freerdp/certs as a lookup path. My understanding is that one can add trusted CAs in ~/.freerdp/certs and have that be used with this first technique. Would that be what the first issue was about?

If x509_verify_certificate() fails, we resort to use our equivalent of ssh's known_hosts file. A check is made with the hostname that was given and the fingerprint of the certificate. If they don't match, certificate validation fails. The user can manually accept untrusted certificates for later, as with SSH.

Now when working on this, I noticed that SSH had two different error messages, one for the fingerprint mismatch, and one for the hostname mismatch. I think this is what you are looking for if I understood you correctly.

SSH will show something like this when the hostname does not match:
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@ WARNING: POSSIBLE DNS SPOOFING DETECTED! @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@

I can work on adding an additional hostname checking function besides the fingerprint check. I'd just like to make sure:

if I connect to, let's say, freerdp.com using its IP address (173.236.214.234) I'll get a hostname of 173.236.214.234, not freerdp.com. In this case, how do I properly check that hostnames match? The certificate will probably give me a different hostname than the one which was used for connection.

Also, I guess that the known hosts should always store the hostname from the certificate, and not the one given by the user?

Should the IP address be used in this verification process in any way?

> I just worked on refactoring, simplifying and cleaning up the
> certificate validation code. It's now much easier to read but it's not
> fixed with regards to this bug report.

Hmmm, hopefully this doesn't void me review....

> tls_verify_certificate() first uses x509_verify_certificate() to attempt
> to validate the certificate using OpenSSL and ~/.freerdp/certs as a
> lookup path. My understanding is that one can add trusted CAs in
> ~/.freerdp/certs and have that be used with this first technique. Would
> that be what the first issue was about?

No. The issue is that the server's hostname is not validated against the Subject Alternative Name or the Common Name field of the X509 certificate that the server presents to freerdp. This is important for thwarting man in the middle attacks (part of the purpose of having certificate verification in the first place). Consider this: with no mitm attacker, suppose freerdp connects to the rdp.foo.com server, gets the rdp.foo.com certificate and then properly verifies it via libfreerdp-core/crypto.c's x509_verify_cert() by setting everything up right and calling openssl's X509_verify_cert(). X509_verify_cert() makes sure the trust chain validates all the way back to the root CA and if there is no error, we are sure that this certificate is ok. Now consider a mitm attacker obtains a properly signed certificate for mitm.attacker.org (which is easy in today's world) and our user is at a coffee shop and is mitm'd. The user tries to connect to rdp.foo.com but instead is redirected to the attacker's machine. The attacker presents the mitm.attacker.org certificate, which is verifiable via X509_verify_cert(), so freerdp continues without error and the user's communications are viewable by the attacker. To stop this, freerdp should verify that the server's hostname matches the Common Name or Subject Alternative Name that is in the X509 certificate. In this case, freerdp should check that rdp.foo.com (what it is trying to connect to) matches the certificate that mitm.attacker.org provides. Because rdp.foo.com won't match the mitm.attacker.org Common Name or Subject Alternative Name, verification fails.

Since libfreerdp-core/crypto.c's x509_verify_cert() failed we then hit the fallback code in tls_verify_certificate() in libfreerdp-core/tls.c, which also doesn't verify the server's hostname against the Common Name or Subject Alternative Name that is in the X509 certificate that is in the user's ~/.freerdp/cacert/ certificate store.

In one instance, this still sort of works: if rdp.foo.com uses a properly signed certificate then if this code is hit, freerdp will presumably prompt the user to accept the certificate for mitm.attacker.org. This dialog should show all the information that would indicate that there is a problem (ie, it is obvious the certificate is not for rdp.foo.com but instead for mitm.attacker.org). In this way there is hope that the user would not accept the certificate. But you can do better than that by doing the server hostname check you and denying the connection right there and not prompt the user at all.

The other instance is when rdp.foo.com uses a self-signed certificate that is in t...

Oh, as for the IP address-- it depends on if you want to support an IP address in your certificate's Common Name or Subject Alternative Name . If you do you also want to verify it and do a reverse lookup on the IP to make sure that everything is ok. Ie, lookup the IP for rdp.foo.com, then lookup that IP to make sure that you get back rdp.foo.com. Error out if they don't match. If they do match, proceed to check that the IP address listed in the Common Name or Subject Alternative Name matches what you just verified in your reverse lookup. Supporting IP addresses means that you could be mitm via DNS attacks (ie, the DNS server resolves the attacker's IP back to rdp.foo.com and the attacker presents a verifiable certificate for his IP, and since everything matches, it is accepted).

Ok, I have just added some improvements. Now I get something like this the first time I connect to one of my servers with a self-signed certificate:

connected to 192.168.1.175:3389
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@ WARNING: CERTIFICATE NAME MISMATCH! @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
The hostname used for this connection (192.168.1.175)
does not match the name given in the certificate:
ANGRYBIRDS.awakecoding.com
A valid certificate for the wrong name should NOT be trusted!
Certificate details:
 Subject: CN = ANGRYBIRDS.awakecoding.com
 Issuer: CN = ANGRYBIRDS.awakecoding.com
 Thumbprint: 1a:e6:2b:74:78:e3:1f:eb:83:cb:28:8a:3b:c7:98:76:bd:b8:c2
The above X.509 certificate could not be verified, possibly because you do not have the CA certificate in your certificate store, or the certificate has expired. Please look at the documentation on how to create local certificate store for a private CA.
Do you trust the above certificate? (Y/N)

In this case, the hostname does not match, and the certificate cannot be validated. I modified the code such that if the certificate is validated by x509_verify_cert it still won't get accepted if the hostname does not match either Common Name or one of the Subject Alternate Names. Is that what should be done?

Please take a look and tell me what would still be lacking after these improvements.

Regards,
- Marc-Andre

I have just pushed further improvements, I have clarified and commented the verification procedure in tls_verify_certificate(). The certificate name should now be properly checked. When prompting the user for manual verification for a certificate with an incorrect name, a warning is shown. A valid certificate with the wrong name is not considered valid and a warning is also displayed.

I have also just added a --certificate-name option for the case where the user is using an IP address instead of the correct name. I think it's better than doing a DNS resolve since the user must explicitly add this as a command-line argument, instead of doing something automatic. For instance, if freerdp.com has a valid certificate but I'm using it's IP address to connect to it, validation would succeed if I passed --certificate-name freerdp.com as an argument.

Anything else?

Thanks for the changes. I reviewed the git commits and this is exactly what you want to be doing. I do have some comments:
crypto_cert_subject_common_name():
* Need to verify subject_name != NULL
* Need to verify index != -1
* Need to verify entry != NULL
* Need to verify entry_data != NULL
* Need to verify length is not < 0
* Need to verify common_name does not have embedded '\0' characters be verifying the length of common_name with ASN1_STRING_length

Similar checks need to be done in crypto_cert_subject_alt_name() (ie, check return codes for *everything*).

From man ASN1_STRING_data:
"In general it cannot be assumed that the data returned by ASN1_STRING_data() is null terminated or does not contain embedded nulls."

As such you should:
a) make sure that the strings you are returning in crypto_cert_subject_common_name() and crypto_cert_subject_alt_name() are NULL terminated, otherwise the strcmp()s you do elsewhere on these can cause you trouble.
b) leverage the fact that differences between ASN1_STRING_length and a (properly terminated) strlen means that you have embedded NULLs.

Why are embedded NULLs bad? Consider if an attacker used this for the common name: CN=rdp.foo.com\0mitm.attacker.org\0

Ok, I added a couple of checks like you suggested. I modified the methods such that they return the length as obtained from ASN1_STRING_length, and this length is used instead of strlen(). Comparison is now done by first comparing if lengths are equal, and then using memcmp() between the two strings. If embedded nulls are present, comparison should fail.

Can you check and see if anything else is missing?

Also, I plan on releasing FreeRDP 1.0.1 and Remmina 1.0 as soon as we get the green light with this, such that you guys get a release that meets Canonical's standards before the feature freeze date. I added a prompt in Remmina for certificate validation, which is basically an equivalent of xfreerdp's command-line prompt for asking the user to accept or deny a certificate. Remmina uses the code from FreeRDP, so if FreeRDP passes the test, Remmina should pass it as well. I'd like to release this week, since we're getting pretty close to the feature freeze date of February 16th.

Seems fine, thanks! :)

Jamie, by "seems fine", do you mean this issue is considered resolved, or that it looks like it could be but you haven't confirmed it yet? I'm waiting for a confirmation before releasing FreeRDP 1.0.1

Sorry. I looked at the changes and they look good.

Ok, I have just released 1.0.1: http://www.freerdp.com/2012/02/09/freerdp-1-0-1-released/

this release should satisfy your security requirements. Could you change the status of this ticket?

When this is packaged for Ubuntu, please include 'LP: #925657' in the debian/changelog and it will automatically close. If it is omitted, I'd be happy to fix it.

Synced 1.0.1 from Debian:

freerdp (1.0.1-1) unstable; urgency=low

  [ Jeremy Bicha ]
  * New upstream release. Closes: #659332.
  * Updated symbols

 -- Otavio Salvador <email address hidden> Sat, 11 Feb 2012 10:34:05 -0200