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

[Marc-André Lureau]

Hi

----- Original Message -----
> On Tue, Jan 25, 2011 at 07:17:27PM +0100, Marc-André Lureau wrote:
> 
> Missing #ifdef HAVE_CONFIG_H
> #include <config.h>
> #endif
> 

It didn't use any config.h define, and the files in common/ don't include config.h. But I would prefer if they do. So, I don't mind having it.

> > +
> > +#include "mem.h"
> > +#include "ssl_verify.h"
> > +
> > +#include <alloca.h>
> > +#include <sys/socket.h>
> > +#include <netinet/in.h>
> > +#include <arpa/inet.h>
> > +
> > +
> > +#ifndef SPICE_DEBUG
> > +# define SPICE_DEBUG(format, ...) printf("%s: " format "\n",
> > __FUNCTION__, ## __VA_ARGS__)
> > +#endif
> 
> Use the macro from spice_common.h

But it's not there (at least today):

http://cgit.freedesktop.org/spice/spice/tree/common/spice_common.h

> > +
> > +static int verify_pubkey(X509* cert, const char *key, size_t
> > key_size)
> 
> the corresponding code in red_peer.c returns a bool, I thought we were
> using C99 and that this was allowed? Most of the boolean in the
> earlier
> code were changed to int.

I changed it to return an int, that way negative values can indicate errors, so we don't loose informations from EVP_PKEY_cmp():

"-1 if the key types are different and -2 if the operation is not supported"

It's also more future proof. Also, we use C99, but it doesn't mean we follow strictly C99, imho that wouldn't make sense.

> > +X509_NAME* subject_to_x509_name(const char *subject, int *nentries)
> > +{
> > + X509_NAME* in_subject;
> 
> What does the in_ prefix stands for here?

I can't remember. We could rename it xsubject if you prefer. Other ideas?

> 
> > + const char *p;
> > + char *key, *val, *k, *v = NULL;
> > + int escape;
> > + enum {
> > + KEY,
> > + VALUE
> > + } state;
> > +
> > + key = (char*)alloca(strlen(subject));
> > + val = (char*)alloca(strlen(subject));
> 
> Any idea if this implies potential portability issues on windows or
> macosx?
> glib provides g_alloca so I assume it's available on the platforms
> we're
> interested in?
> Reading
> http://www.gnu.org/s/hello/manual/autoconf/Particular-Functions.html
> we may
> need something more complicated for win32

Correct, I used this in spice-gtk, it could be included in the patch series.

https://gitorious.org/spice-gtk/spice-gtk/commit/46be1ca0485fd2e6dcb1b43fb43ffa37e7f3a3f7

Or we could use the snippet from the autoconf manual, which looks good as well.

> > + in_subject = X509_NAME_new();
> > +
> > + if (!in_subject || !key || !val) {
> > + SPICE_DEBUG("failed to allocate");
> > + return NULL;
> > + }
> > +
> > + *nentries = 0;
> > +
> > + k = key;
> > + state = KEY;
> > + for (p = subject;; ++p) {
> > + escape = 0;
> 
> I'd move int escape; here instead of at the beginning of the function

ack
 
> > + if (*p == '\\') {
> > + ++p;
> > + if (*p != '\\' && *p != ',') {
> > + SPICE_DEBUG("Invalid character after \\");
> > + goto fail;
> > + }
> > + escape = 1;
> > + }
> > +
> > + switch (state) {
> > + case KEY:
> > + if (*p == 0) {
> > + if (k == key) /* empty key */
> > + goto success;
> > + goto fail;
> > + } else if (*p == '=' && !escape) {
> > + state = VALUE;
> > + *k = 0;
> > + v = val;
> > + } else
> > + *k++ = *p;
> > + break;
> 
> This code should check for ',' somewhere if we want to get the code to
> behave as Application::set_host_cert_subject: when parsing
> "foo,bar=foobar", this code will parse (foo,bar ; foobar) while the
> old code would have failed with "host_subject bad format: assignment
> for
> foo is missing"

I didn't test it, but With "foo,", v == val and it will go to fail.

> It will also silently go to the VALUE state when the key is empty (
> "=foo"), and the VALUE parsing isn't checking for tha either,I don't
> know
> if that's desirable

It rely on X509_NAME_add_entry_by_txt() to do the right thing when key is empty instead.

I don't know if an empty VALUE is correct either, but I guess it's fine.

> > + case VALUE:
> > + if (*p == 0 || (*p == ',' && !escape)) {
> > + if (v == val) /* empty value */
> > + goto fail;
> > +
> > + *v = 0;
> > +
> > + if (!X509_NAME_add_entry_by_txt(in_subject, key,
> > + MBSTRING_UTF8,
> > + (const unsigned char*)val,
> > + strlen(val), -1, 0)) {
> 
> -1 instead of strlen(val) would work here

ack

> > + SPICE_DEBUG("warning: failed to add entry %s=%s to X509_NAME",
> > + key, val);
> > + goto fail;
> > + }
> > + *nentries += 1;
> > +
> > + if (*p == 0)
> > + goto success;
> > +
> > + state = KEY;
> > + k = key;
> > + } else
> > + *v++ = *p;
> 
> Similarly to the "KEY" state, here we will parse foo=bar=foobar as
> (foo ;
> bar=foobar) unless X509_NAME_add_entry_by_txt complains about this,
> but the
> old code behaved the same as far as I can tell. Reading
> apps/apps.c:parse_name in openssl source seems to imply that anything
> after
> \\ is to be taken verbatim, ie the only invalid use of \\ is when it's
> at
> the end of a string. It also seems that '+' is a magic value. But the
> parsed string has a slightly different format (values separated by '/'
> instead of ','), so maybe we can't compare the 2 implementations.
> This code also lost the fix from 12f99d3 which skips spaces between
> ',' and
> keys, but I can't blame you for that since this patch predates this
> commit
> :)
> 

Why do you think that foo=bar=foobar -> (foo ; bar=foobar) should be an error?

We should skip spaces after comma, that should be easy to add.

> > + /* Note: this check is redundant with the pre-condition in
> > X509_NAME_cmp */
> 
> This was already the case in openssl 0.9.8, I'd just drop this test

But this way we get an error message that we can actually debug easily.
 
> > +
> > +#ifndef __SSL_VERIFY_H
> > +#define __SSL_VERIFY_H
> > +
> 
> Can you add c++ guards too?
> 
> #ifdef __cplusplus
> extern "C" {
> #endif
> 

Yup

> Since the verification operation are mutually exclusive, another
> possibility is to have 3 or 4 different constructors, and to drop the
> SPICE_SSL_VERIFY_OP enum from the public API.

Why do you think they are mutually exclusive?

Thanks for the review!