Re: [Openvpn-devel] [PATCH-fixed] revocation

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 23/04/10 12:56, Heikki Kallasjoki wrote:
> A minor nitpick, but...
> 
> On Fri, Apr 23, 2010 at 11:35:05AM +0100, Davide Brini wrote:
>> On Friday 23 Apr 2010 11:13:21 David Sommerseth wrote:
>>> On 22/04/10 23:37, Davide Brini wrote:
 --- openvpn-2.1.1/ssl.c2010-02-28 22:17:45.0 +
 +++ openvpn-2.1.1-a/ssl.c  2010-04-22 22:33:40.0 +0100
 @@ -788,9 +788,28 @@ verify_callback (int preverify_ok, X509_

/* export serial number as environmental variable */
{
 -const int serial = (int) ASN1_INTEGER_get (X509_get_serialNumber
 (ctx->current_cert)); +BIO *bio = NULL;
 +char serial[1024] = {0};
> [...]
>>> The initialisation is also rather bogus, as it will only put \0 in the
>>> first byte, not the complete string.  It's better to instead use
>>> CLEAR (serial) afterwards (which is a wrapper for memset()).
>>
>> Ok, will do that. But anyway, since it's meant to be a string, I thought 
>> that 
>> initializing it to '\0' would mean "empty string".

Yes, that is kind of right. The '\0' do give the needed NULL termination
which tells most string functions when to stop reading further.  But if
you do printf("%s", serial+1) you'll "escape" that single NULL
termination.  If that memory region is not cleared, you're going to get
a lot of garbage out.

> The initialization will, in fact, set all bytes of "serial" to zero.
> Quoting the C standard (well, C89 draft is the closest I have):
> 
> "If there are fewer initializers in a list than there are members of
> an aggregate, the remainder of the aggregate shall be initialized
> implicitly the same as objects that have static storage duration."

On modern systems, this is most often the case.

> Which means filling with zeroes:
> 
> "If an object that has static storage duration is not initialized
> explicitly, it is initialized implicitly as if every member that has
> arithmetic type were assigned 0 and [--]"
> 
> I.e. you cannot "partially" initialize an object: the array elements not
> mentioned in the initializer are implicitly initialized to zero.  You
> only get indeterminate values by omitting the initializer completely.
> I guess it's concievable some compiler could get this wrong, though...

That is as my main concern.  Most GCC compilers nowadays, do give 0'ed
values on statically allocated variables, at least on Linux kernel.
IIRC, those local variables are allocated on the stack, which gets the
memory allocation via the kernel - so it's also up to the kernel if it
returns a zero'd memory buffer.  But this code might be built on less
"clever" systems as well.

And the OpenVPN code uses CLEAR() and even memset() a lot of places
throughout the code.  This is merely to align with the coding "standard"
in OpenVPN, and to follow more a defensive coding style.


kind regards,

David Sommerseth
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkvRkvQACgkQDC186MBRfrrU/QCcCk/JxpcAxtb//ZGD1vSGNEwi
H/AAn0tMN5Ov3kydEDStoif7p6WKBqCT
=ONp1
-END PGP SIGNATURE-

Re: [Openvpn-devel] [PATCH-fixed] revocation

[Heikki Kallasjoki]

A minor nitpick, but...

On Fri, Apr 23, 2010 at 11:35:05AM +0100, Davide Brini wrote:
> On Friday 23 Apr 2010 11:13:21 David Sommerseth wrote:
> > On 22/04/10 23:37, Davide Brini wrote:
> > > --- openvpn-2.1.1/ssl.c   2010-02-28 22:17:45.0 +
> > > +++ openvpn-2.1.1-a/ssl.c 2010-04-22 22:33:40.0 +0100
> > > @@ -788,9 +788,28 @@ verify_callback (int preverify_ok, X509_
> > >
> > >/* export serial number as environmental variable */
> > >{
> > > -const int serial = (int) ASN1_INTEGER_get (X509_get_serialNumber
> > > (ctx->current_cert)); +BIO *bio = NULL;
> > > +char serial[1024] = {0};
[...]
> > The initialisation is also rather bogus, as it will only put \0 in the
> > first byte, not the complete string.  It's better to instead use
> > CLEAR (serial) afterwards (which is a wrapper for memset()).
> 
> Ok, will do that. But anyway, since it's meant to be a string, I thought that 
> initializing it to '\0' would mean "empty string".

The initialization will, in fact, set all bytes of "serial" to zero.
Quoting the C standard (well, C89 draft is the closest I have):

"If there are fewer initializers in a list than there are members of
an aggregate, the remainder of the aggregate shall be initialized
implicitly the same as objects that have static storage duration."

Which means filling with zeroes:

"If an object that has static storage duration is not initialized
explicitly, it is initialized implicitly as if every member that has
arithmetic type were assigned 0 and [--]"

I.e. you cannot "partially" initialize an object: the array elements not
mentioned in the initializer are implicitly initialized to zero.  You
only get indeterminate values by omitting the initializer completely.
I guess it's concievable some compiler could get this wrong, though...

-- 
Heikki Kallasjoki
heikki.kallasj...@iki.fi

Re: [Openvpn-devel] [PATCH-fixed] revocation

[Davide Brini]

On Friday 23 Apr 2010 11:13:21 David Sommerseth wrote:

> On 22/04/10 23:37, Davide Brini wrote:
> > --- openvpn-2.1.1/ssl.c 2010-02-28 22:17:45.0 +
> > +++ openvpn-2.1.1-a/ssl.c   2010-04-22 22:33:40.0 +0100
> > @@ -788,9 +788,28 @@ verify_callback (int preverify_ok, X509_
> >
> >/* export serial number as environmental variable */
> >{
> > -const int serial = (int) ASN1_INTEGER_get (X509_get_serialNumber
> > (ctx->current_cert)); +BIO *bio = NULL;
> > +char serial[1024] = {0};
> 
> Do we need 1024 bytes?  RFC5280 indicates 20 octets (read: bytes) as the
> expected maximum size.  To print 20 octets as hex, you need 40 bytes,
> and with delimiters for each octet it will be 59 bytes.

Right. Even if the variable is short-lived, and even anticipating future 
needs...I suppose 100 bytes will do for now.

> The initialisation is also rather bogus, as it will only put \0 in the
> first byte, not the complete string.  It's better to instead use
> CLEAR (serial) afterwards (which is a wrapper for memset()).

Ok, will do that. But anyway, since it's meant to be a string, I thought that 
initializing it to '\0' would mean "empty string".

> > +int n;
> > +
> > +if ((bio = BIO_new (BIO_s_mem ())) == NULL) {
> > +  msg (M_WARN, "CALLBACK: Cannot create BIO (for tls_serial_%d)",
> > ctx->error_depth); +}
> > +else {
> > +  /* "prints" the serial number onto the BIO */
> > +  i2a_ASN1_INTEGER(bio, X509_get_serialNumber (ctx->current_cert));
> > +  n = BIO_read (bio, serial, sizeof (serial)-1);
> > +  if (n < 0) {
> > +serial[0] = '\x0';
> > +  }
> > +  else {
> > +serial[n] = 0;
> > +  }
> 
> This if-block is really not needed if you memset() serial first, unless
> there are known issues with OpenSSL that it might place garbage after
> the n'th byte.  I don't think that is the case, though.

Neither do I, but I thought that "better safe then sorry". And btw, I saw that 
other OpenVPN code is /already/ doing the same thing (in fact, I wrote my code 
modeled after that): see pkcs11.c, from line 935.

> It should also be some error checking here as well.
> 
> Another thing is if BIO_read() returns 0, the serial number should not
> result in an empty string.  I'm wondering if it would be appropriate for
> us to return "0" zero as a string) or "-1" as a string to the
> environmental variable.  The former implementation would probably return
> 0, but I'm wondering if -1 would be even better to indicate an error.

Unfortunately, both 0 and negative numbers are all possible serial numbers 
(although they are now forbidden and not found in practice, but the openssl 
routines DO handle them). The RFC also says

"CAs MUST force the serialNumber to be a non-negative integer.
...
Note: Non-conforming CAs may issue certificates with serial numbers that are 
negative or zero.  Certificate users SHOULD be prepared to gracefully handle 
such certificates."

So I maintain that an empty string is the right thing to do. It will then be 
up to whatever code uses the environment variable to check that it's not empty 
(or negative, etc.).

> Maybe even consider to check the return value of i2a_ASN1_INTEGER() as
> well.

See above. Since the other code fragment doesn't do that, neither did I. But 
right, I will add a check and a warning if it returns a value < 0, which will 
result in an empty string put into the environment variable.

Let me know what you think, and I'll send a revised patch (yes, with summary, 
Signed-off-by and all that; I'm learning, slowly).
Thanks for reviewing!

-- 
D.

Re: [Openvpn-devel] [PATCH-fixed] revocation

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1


Hi Davide!

Thanks a lot!  This is looking pretty well, I have a few comments, though.

On 22/04/10 23:37, Davide Brini wrote:
> 
> --- openvpn-2.1.1/ssl.c   2010-02-28 22:17:45.0 +
> +++ openvpn-2.1.1-a/ssl.c 2010-04-22 22:33:40.0 +0100
> @@ -788,9 +788,28 @@ verify_callback (int preverify_ok, X509_
>  
>/* export serial number as environmental variable */
>{
> -const int serial = (int) ASN1_INTEGER_get (X509_get_serialNumber 
> (ctx->current_cert));
> +BIO *bio = NULL;
> +char serial[1024] = {0};

Do we need 1024 bytes?  RFC5280 indicates 20 octets (read: bytes) as the
expected maximum size.  To print 20 octets as hex, you need 40 bytes,
and with delimiters for each octet it will be 59 bytes.

The initialisation is also rather bogus, as it will only put \0 in the
first byte, not the complete string.  It's better to instead use
CLEAR (serial) afterwards (which is a wrapper for memset()).

> +int n;
> +
> +if ((bio = BIO_new (BIO_s_mem ())) == NULL) {
> +  msg (M_WARN, "CALLBACK: Cannot create BIO (for tls_serial_%d)", 
> ctx->error_depth);
> +}
> +else {
> +  /* "prints" the serial number onto the BIO */
> +  i2a_ASN1_INTEGER(bio, X509_get_serialNumber (ctx->current_cert));
> +  n = BIO_read (bio, serial, sizeof (serial)-1);
> +  if (n < 0) {
> +serial[0] = '\x0';
> +  }
> +  else {
> +serial[n] = 0;
> +  }

This if-block is really not needed if you memset() serial first, unless
there are known issues with OpenSSL that it might place garbage after
the n'th byte.  I don't think that is the case, though.

It should also be some error checking here as well.

Another thing is if BIO_read() returns 0, the serial number should not
result in an empty string.  I'm wondering if it would be appropriate for
us to return "0" zero as a string) or "-1" as a string to the
environmental variable.  The former implementation would probably return
0, but I'm wondering if -1 would be even better to indicate an error.

Maybe even consider to check the return value of i2a_ASN1_INTEGER() as well.

>  openvpn_snprintf (envname, sizeof(envname), "tls_serial_%d", 
> ctx->error_depth);
> -setenv_int (opt->es, envname, serial);
> +  setenv_str (opt->es, envname, serial);
> +  BIO_free(bio);
> +}
>}


kind regards,

David Sommerseth
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkvRcr4ACgkQDC186MBRfrqraQCggpyJ+DKcmzyK1uhmodj1cLIT
LEMAniCuXp1HZ5WM8lhrGZ9F+kyDrka7
=kgsa
-END PGP SIGNATURE-

Re: [Openvpn-devel] [PATCH-fixed] revocation

[Davide Brini]

On Thursday 22 April 2010, Jan Just Keijser wrote:

> > The only doubt I have is about error handling; in this case, if the
> > allocation of the BIO fails, an error message is logged and nothing is
> > done. Is this the right thing to do?
> 
> I don't know if a FATAL error is such a good thing - not being able to
> set an env var does not warrant a server crash , I'd say.

Ok, thanks. It's now a warning (with a clearer message as well).

> Also, you're not freeing the BIO as far as I can tell.

Right, well spotted! The variables are in a block which makes them local, but 
of course the memory pointed to by bio must be freed. Thanks for the heads-up.

Please find attached the revised patch.

-- 
D.
--- openvpn-2.1.1/ssl.c	2010-02-28 22:17:45.0 +
+++ openvpn-2.1.1-a/ssl.c	2010-04-22 22:33:40.0 +0100
@@ -788,9 +788,28 @@ verify_callback (int preverify_ok, X509_

   /* export serial number as environmental variable */
   {
-const int serial = (int) ASN1_INTEGER_get (X509_get_serialNumber (ctx->current_cert));
+BIO *bio = NULL;
+char serial[1024] = {0};
+int n;
+
+if ((bio = BIO_new (BIO_s_mem ())) == NULL) {
+  msg (M_WARN, "CALLBACK: Cannot create BIO (for tls_serial_%d)", ctx->error_depth);
+}
+else {
+  /* "prints" the serial number onto the BIO */
+  i2a_ASN1_INTEGER(bio, X509_get_serialNumber (ctx->current_cert));
+  n = BIO_read (bio, serial, sizeof (serial)-1);
+  if (n < 0) {
+serial[0] = '\x0';
+  }
+  else {
+serial[n] = 0;
+  }
+
 openvpn_snprintf (envname, sizeof(envname), "tls_serial_%d", ctx->error_depth);
-setenv_int (opt->es, envname, serial);
+  setenv_str (opt->es, envname, serial);
+  BIO_free(bio);
+}
   }

   /* export current untrusted IP */

Re: [Openvpn-devel] [PATCH-fixed] revocation

[Jan Just Keijser]

Davide Brini wrote:

On Thursday 22 April 2010, Davide Brini wrote:
  

(moving to -devel as this is obviously pertains there more than -users)



Sorry, too quick! I posted an incomplete version of the patch. The attached 
one should be better.


The only doubt I have is about error handling; in this case, if the allocation 
of the BIO fails, an error message is logged and nothing is done. Is this the 
right thing to do?


  
I don't know if a FATAL error is such a good thing - not being able to 
set an env var does not warrant a server crash , I'd say.

Also, you're not freeing the BIO as far as I can tell.

apart from that: nice patch !

cheers,

JJK