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

[Openvpn-devel] [PATCH] revocation

[Davide Brini]

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

On Thursday 22 April 2010, Davide Brini wrote:

> > > RFC 5280 says that "certificate users MUST be able to handle
> > > serialNumber values up to 20 octets", so a 16-byte value looks valid to
> > > me. I would say (without looking at the code) that it's OpenVPN that
> > > can't handle it and gives -1 to the user. However I'm performing some
> > > further
> > > verification, and will report back if I find something.
> >
> > Please do!  However, beware that this RFC separates 'certificate serial
> > number' and 'serial number'.  Which is probably why the confusion is all
> > present.  The specification also uses the ASN1 INTEGER definition,
> > AFAICU.  But I don't recall the expected size of those integer values,
> > but I believe it is 32bit in OpenSSL (that's what ASN1_INTEGER_get()
> > returns).  Going to the extreme, I don't think that ASN1 sets any
> > boundary on the min/max values of INTEGER - which makes it quite
> > interesting.
> >
> > You might want to try to patch ssl.c with a better suitable type in
> > ssl.c.  Have a look around line 850.  You could try to use 'unsigned
> > long int' and change the %d to %ld, or whatever the compiler accepts for
> > unsigned long int.  From what I can understand, ASN1_INTEGER_get()
> > should be correct.
> >
> > long is usually 32bit (4 bytes).  Making it unsigned gives you the 32th
> >  bit.

Ok, so I'm looking into the sources of openssl. ASN1_INTEGER is a typedef for 
"struct asn1_string_st"; and the comments in the code for that structure are 
quite explicit:

-
/* This is the base type that holds just about everything :-) */
typedef struct asn1_string_st
{
int length;
int type;
unsigned char *data;
/* The value of the following field depends on the type being
 * held.  It is mostly being used for BIT_STRING so if the
 * input data has a non-zero 'unused bits' value, it will be
 * handled correctly */
long flags;
} ASN1_STRING;
-

So an ASN1_INTEGER in OpenSSL can really contain arbitrarily long values (many 
other types are typedef'd to "struct asn1_string_st").

More to the point, when one runs "openssl x509 -text", it turns out that it 
explicitly checks for the serial number's length. If it's up to 4 bytes, it's 
printed like

Serial Number: 927650371 (0x374ad243)

if instead it's longer, it's printed in the form

Serial Number:
05:40:69:bf:0f:c2:4f:f4:b0:05:ee:95:3f:18:71:17

(crypto/asn1/t_x509.c, line 138 and following), which confirms my initial 
guess. So in fact it's just a different output format, and the serial number 
is just an (almost) arbitrarily large number. Why would a CA choose such a 
serial number? I don't know, it's their business but we should be able to 
handle it.

Sorry for the - somewhat - OT introduction, but I felt it was necessary to 
clarify the puzzling output I was seeing earlier. Now let's turn to OpenVPN.

ssl.c, line 789:

-
 /* export serial number as environmental variable */
const int serial = (int) ASN1_INTEGER_get (X509_get_serialNumber (ctx-
>current_cert));
-

Here is ASN1_INTEGER_get (from openssl's crypto/asn1/a_int.c, line 376):

-
long ASN1_INTEGER_get(ASN1_INTEGER *a)
{
int neg=0,i;
long r=0;

if (a == NULL) return(0L);
i=a->type;
if (i == V_ASN1_NEG_INTEGER)
neg=1;
else if (i != V_ASN1_INTEGER)
return -1;

if (a->length > (int)sizeof(long))
{
/* hmm... a bit ugly */
return(0xL);
}
if (a->data == NULL)
return 0;

for (i=0; ilength; i++)
{
r<<=8;
r|=(unsigned char)a->data[i];
}
if (neg) r= -r;
return(r);
}
-

and that explains why we get -1 with big serial numbers on 32 bit.

Now, it seems that the "right" fix for OpenVPN would be to use a memory BIO 
object as explained in the docs. That way, the serial number is "printed" to 
the BIO object and can be read back into an array of characters. In that form, 
it could be exported into the environment using setenv_str() instead of 
setenv_int() (environment variables are all strings anyway). Also, due to the 
way i2a_ASN1_INTEGER() prints stuff, it will be a long hexadecimal string, 
which integrates nicely with OCSP, so it is ready to use when doing OCSP 
queries (by doing '-serial "0x${tls_serial_0}"' etc.)

So here's a patch that does exactly that; it's quite straightforward in fact. 
I have tested it on two Gentoo boxes and it seems to work fine, although of 
course more testing never hurts; here's a dump of the serial numbers in a full 
certificate chain:

054069bf0fc24ff4b005ee953f187117

[Openvpn-devel] OpenVPN 2.1.1 and "inactive". "ping" is retrigger the inactivity timeout!!

[Siegfried Müller - MB Connect Line GmbH]

Hi developers,

does anyone tried the "inactive" option with the openvpn version 2.1.1?
I step up from 2.0.1 to 2.1.1 and get the problem, that the client does not 
disconnect after inactivity.
If i set the option "ping 0" instead of "ping 10" the inactivity works. Could 
that be a problem, that the new version does retrigger the inactivity-timeout 
with his own ping?
any ideas?

regards
siegfried

Re: [Openvpn-devel] [PATCH] Add comile time settings from ./configure to --version

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 13/04/10 15:49, David Sommerseth wrote:
[...snip...]
> 
> I've attached three patches, which cleans up this feature further.
> 

This is an enhanced patch, based on review comments from Gert Doering.
He mentioned that the configure_log.awk could be made a bit more
"understandable".  I've done some minor adjustments to improve that,
also for people not too much acquainted with awk.  It is also made safer
to if the needed information is not found in config.log.

Patch attached.


kind regards,

David Sommerseth

- ---
 Makefile.am   |   10 +++---
 configure_log.awk |7 ++-
 options.c |2 ++
 3 files changed, 11 insertions(+), 8 deletions(-)



>> On Sat, Apr 10, 2010 at 10:05 AM, Alon Bar-Lev  wrote:
>>> Highly none standard autotools usage.
>>> Also requires awk which may not be available.
>>> Please REVERT.
>>>
>>> On Thu, Apr 8, 2010 at 10:44 PM, David Sommerseth
>>>  wrote:
>> On 30/03/10 14:12, David Sommerseth wrote:
>> This patch will create ./configure.h which will contain two new #define
>> strings.  CONFIGURE_DEFINES will contain all USE, ENABLED, DISABLED and
>> DEPRECATED defines from ./config.h.  CONFIGURE_CALL will contain the
>> complete ./configure line which was used when configuring the package
>> for building.
>>
>> Signed-off-by: David Sommerseth 
>> ---
>>  Makefile.am   |7 +++
>>  configure_h.awk   |   39 +++
>>  configure_log.awk |   36 
>>  options.c |3 +++
>>  4 files changed, 85 insertions(+), 0 deletions(-)
>>  create mode 100644 configure_h.awk
>>  create mode 100644 configure_log.awk
>>
>> Applied to the openvpn-testing.git feat_misc branch.  To be merged into
>> allmerged soon.
>> Commit f27bf509315a48b0070294c3993a718df0c2626c
>>
>>
>> kind regards,
>>
>> David Sommerseth

> --
> Download Intel Parallel Studio Eval
> Try the new software tools for yourself. Speed compiling, find bugs
> proactively, and fine-tune applications for parallel performance.
> See why Intel Parallel Studio got high marks during beta.
> http://p.sf.net/sfu/intel-sw-dev
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

>>>
> 
> 
> 
> --
> Download Intel Parallel Studio Eval
> Try the new software tools for yourself. Speed compiling, find bugs
> proactively, and fine-tune applications for parallel performance.
> See why Intel Parallel Studio got high marks during beta.
> http://p.sf.net/sfu/intel-sw-dev
> 
> 
> 
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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

iEYEARECAAYFAkvQZIkACgkQDC186MBRfrq3RgCeKFbqnn4ApvAvFxCkT1daedj/
vr0An0gRtTyhZJUZFQDpo3taFJ4o7pB2
=wWpo
-END PGP SIGNATURE-
>From 239ed4ed97d12997a552524f30d81a88ec8b231b Mon Sep 17 00:00:00 2001
From: David Sommerseth 
List-Post: openvpn-devel@lists.sourceforge.net
Date: Thu, 22 Apr 2010 16:08:34 +0200
Subject: [PATCH 1/3] Fix dependency checking for configure.h (v2)

Alon Bar-Lev indicated commit f27bf509315a48b0070294c3993a718df0c2626c
was missing proper dependency checking.  This patch corrects this and
fixes an issue when creating configure.h via make distcheck.

This is an enhanced version of the one sent to the openvpn-devel mailing
list April 13, 2010 [1], after having received some feedback from Gert
Doering, cleaning up configure_log.awk further.

[1] 

Signed-off-by: David Sommerseth 
---
 Makefile.am   |   10 +++---
 configure_log.awk |7 ++-
 options.c |2 ++
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 20453d0..f509a4b 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -66,7 +66,8 @@ dist_noinst_SCRIPTS = \
 	$(TESTS) \
 	doclean \
 	domake-win \
-	t_cltsrv-down.sh
+	t_cltsrv-down.sh \
+	configure_h.awk configure_log.awk

 dist_noinst_DATA = \
 	openvpn.spec \
@@ -141,9 +142,12 @@ openvpn_SOURCES = \
 	win32.h win32.c \
 	cryptoapi.h cryptoapi.c

+nodist_openvpn_SOURCES = configure.h
+options.$(OBJEXT): configure.h
+
 configure.h: Makefile
-	awk -f configure_h.awk config.h > $@
-	awk -f configure_log.awk config.log >> $@
+	awk -f $(srcdir)/configure_h.awk config.h > $@
+	awk -f 

[Openvpn-devel] ESET Smart Security interferes with TAP driver install

[James Yonan]

I've observed a case on Windows Server 2008R2 where an antivirus product 
called ESET Smart Security prevents install of the TAP driver.


The net effect in tapinstall (devcon) is that 
UpdateDriverForPlugAndPlayDevices returns error code 1450 
(ERROR_NO_SYSTEM_RESOURCES).


Apparently ESET interferes with other VPN clients as well:

http://social.technet.microsoft.com/Forums/en/w7itpronetworking/thread/f92abbdf-a340-4629-a46c-f186f7c82340

James

Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 22/04/10 10:15, Davide Brini wrote:
> On Thursday 22 Apr 2010 09:02:23 David Sommerseth wrote:
> 
>> For future patches, would you mind adding a little bit more descriptive
>> text which can be used as commit log messages.  I do write those commit
>> logs when I find it is needed, but adding a little bit more descriptions
>> of what the patch does is a nice and good habit - no matter how easy the
>> patch looks like ... and it makes my maintenance job a lot easier and
>> quicker.
> 
> Sure, no problem. But just to make it clear (please forgive my ignorance), my 
> original message for the patch contained quite a detailed explanation of what 
> it does. 

Sorry, I was a bit unclear ... the initial patch had good info!  And as
you can see in the commit log, I've used a lot of that text.

> I assume that was too long, and you need a 1-2 lines summary to use 
> for the commit? 

It doesn't need to be a novel, but I do prefer longer ones and not just
1-2 lines.  So not too short, please :)

> Of course this is not to say that the detailed explanation 
> should be missing, only that you need the brief summary as well. Is my 
> understanding correct?

Ideally a commit log should state the problem the patch should fix and
give the basic ideas of how you solved it - without being too technical,
as the patch will describe the technical details.  But things which
might be difficult to understand in the code, should be explained in
"plain English" in the commit log.

If applicable, also add references to mail discussions, chat logs, etc,
if that is relevant and can give more background information.  This is
important when someone wants to investigate the patches later on, maybe
even someone who is fresh to the OpenVPN code.

And if your text can be used as a "copy-paste" into the git commit,
that's what I'd prefer :)

Another thing I've began to add to commit logs are those
"Signed-off-by:" lines for those submitting the patches.  So, please add
"Signed-off-by:" lines as well.




To explain the Signed-off-by and Acked-by process a bit further.  I do
now separate commit Author and sender.  The difference is that the
author field should be a reference to the one who *wrote* the patch.
This person sends it to someone, and would add the first "Signed-off-by"
message as a "signature" of the patch.  This indicates that the author
finds the patch ready to be sent further.

Then the one who receives the patch and finds it good to go further,
will add his/hers "Signed-off-by" reference.  That way we can track who
have been looking at the patch.  Before it goes into the tree, a final
"Acked-by:" note is added, indicating who accepted it for inclusion into
the tree.

So, if John Doe writes a patch, sends it to Jane Doe and she sends it
Bob Boss for inclusion the process would be:

John Doe:
Author: John Doe 
Patch fixing x.y.z

explaining the patch

Signed-off-by: John Doe 



Jane Doe will send it further as:
Author: John Doe 
Patch fixing x.y.z

explaining the patch

Signed-off-by: John Doe 
Signed-off-by: Jane Doe   



And when Bob Boss ACK's the patch, the final commit to be found in the
tree will be:
Author: John Doe 
Patch fixing x.y.z

explaining the patch

Signed-off-by: John Doe 
Signed-off-by: Jane Doe   
Signed-off-by: David Sommerseth 
Acked-by: Bob Boss 



The Acked-by: can also be someone who don't do the final commit[1].  Or
it can be my patches which I've sent for review before including
them[2].  And it can of course be ACKed by the same person who do the
final commit.  The last Signed-off-by should normally be the person the
one who does the final commit.  This is the person  who "touched" the
patch by adding the Acked-by line(s).

However, you should not see any commits which is written by (author) and
Acked-by by the same person.  Then the process have failed to give a
qualified review.

You may ask why we have this "bureaucracy" (which is a fair question!)
It is simply to keep all who send in patches, those who reviews them and
finally accepts them into the source tree more accountable for their
work.  And to document that each accepted patch has been through a
certain set of reviews.

As these patches will go back into SVN, it's not possible to, AFAIK, to
track authors and committers explicitly - so we track it in the commit
log as well.  Just to keep this project as transparent as possible.

For now, the only patches which which "breaks" this in the git tree are
the patches coming in from James' SVN BETA21 branch.  But I trust that
James tracks the patches he pulls in according to the standards he

Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[Davide Brini]

On Thursday 22 Apr 2010 09:02:23 David Sommerseth wrote:

> For future patches, would you mind adding a little bit more descriptive
> text which can be used as commit log messages.  I do write those commit
> logs when I find it is needed, but adding a little bit more descriptions
> of what the patch does is a nice and good habit - no matter how easy the
> patch looks like ... and it makes my maintenance job a lot easier and
> quicker.

Sure, no problem. But just to make it clear (please forgive my ignorance), my 
original message for the patch contained quite a detailed explanation of what 
it does. I assume that was too long, and you need a 1-2 lines summary to use 
for the commit? Of course this is not to say that the detailed explanation 
should be missing, only that you need the brief summary as well. Is my 
understanding correct?

Thanks.

-- 
D.

Re: [Openvpn-devel] [PATCH] bash->bourne script cleanup

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 19/04/10 23:37, Davide Brini wrote:
> On Monday 19 April 2010, David Sommerseth wrote:
> 
>> I've done a quick test on one of my connections on Fedora 12 without any
>> resolvconf package (meaning it invokes the simple cp approach), and it
>> worked like a charm.
>>
>> Applied to bugfix2.1 and merged into allmerged.
>> Commit a9c9a89e96dc1e4e843e05ecadc4349b81606b06
> 
> Sorry, I just realized that I didn't change the sha-bang line in client.down. 
> Apologies. Fix attached.

No problem!  And thanks a lot for fixing it!  I should have caught that
one during my review as well.  I looked at "everything", but managed to
not notice this one as well.  Anyway, I gave it an ACK and it's now in
the bugfix2.1 branch.  As this is a non-critical fix, I'll delay pushing
it to allmerged until more patches comes in.  You'll find it as commit
2d9c14b435ae4083aa3f6bce803ede27cce2b6cb

For future patches, would you mind adding a little bit more descriptive
text which can be used as commit log messages.  I do write those commit
logs when I find it is needed, but adding a little bit more descriptions
of what the patch does is a nice and good habit - no matter how easy the
patch looks like ... and it makes my maintenance job a lot easier and
quicker.

I won't reject good patches due to missing commit messages, but it I'm
not picking these patches first for inclusion - as they require more
work.  So the easier the patches are for me to work with, the quicker
they go into the tree after the required review.


kind regards,

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

iEYEARECAAYFAkvQAowACgkQDC186MBRfrrhCQCfdrZTU2ilBNQ+K8FC8iDguloq
X04An0/BxnoqesB1CqCS+BzYdreb8rrh
=q4xW
-END PGP SIGNATURE-