[Spice-devel] [PATCH 8/9] common: add ssl_verify.c common code

[Christophe Fergeau]

On Tue, Jan 25, 2011 at 07:17:27PM +0100, Marc-André Lureau wrote:
> Code adapter from RedPeer::ssl_verify_callback() and used by
> spice-gtk.

I looked at this one, and was quickly concerned about the amount of
security checks we're trying to do on our own. Basically, we let openssl do
the certificate verification by itself. However, we try to do 3 additional
checks by ourselves as part of the certificate verification:
- we try to check if the host we're connecting to  (specified through
  --host) is mentionned in the certificate (maybe to handle certificates
  stored on vhosts )
- or (mutually exclusive as mentionned in the log) we check the certificate
  subject matches what was passed on the command line (and there's too much
  string parsing involved for my taste)
- or we check some public key

#3 seems to be only used in migration cases, and even there, the pubkey
code is just a stub, I couldn't find anything setting the public key data
we want to use. It might be better to just drop this part of the code.

#1 and #2 probably have their uses (I'd be interested to know what drove
adding this code btw), but it's quite some code, and I am no x509 security
specialist (far from it, maybe what I'm saying doesn't make anything).
Given that proper certificate handling is really tricky (there are
regular security bugs in the related code in eg browsers) and that it's a
fair amount of code, I have big doubts we can get it 100% right. I'd love
to be proven wrong and have someone with deep x509/tls knowledge jump in
and tell me he/she wrote this code and she/he is really confident it's
right.
If not, and if we are not sure why we are doing these additional
hostname/subject checks in this code, maybe we should consider disabling
these checks and just relying on the builtin openssl verification code.

So far, no ACK/NACK on this patch, from a quick glance I couldn't see any
big issues, but I need to look at it more closely for the details :)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20110429/a4345e08/attachment.pgp>