Opened 6 years ago

Closed 6 years ago

#402 closed Bug / Defect (fixed)

Incorrect strncmp() test breaks the "--x509-username-field" option

Reported by: andrisk Owned by: Steffan Karger
Priority: major Milestone: release 2.3.5
Component: Configuration Version: OpenVPN 2.3.4 (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords:
Cc:

Description

Based on a clarifying email exchange on the openvpn-devel list,

http://sourceforge.net/p/openvpn/mailman/message/32313687/

the certificate attribute name from the Subject field to be used in place
of the default Common Name (CN) attribute is wrongly uppercased due to a
mistake in testing the return value of strncmp(). This prevents the
argument to the "--x509-username-field" option from being matched.
The one-line patch to `src/openvpn/options.c' is attached.

Attachments (6)

options.c.patch (406 bytes) - added by andrisk 6 years ago.
Patch to src/openvpn/options.c
version1.patch.zip (4.8 KB) - added by andris 6 years ago.
version2.patch.zip (4.4 KB) - added by andris 6 years ago.
0001-Fix-some-typos-in-the-man-page.patch (1.9 KB) - added by Steffan Karger 6 years ago.
0002-Do-not-upcase-x509-username-field-for-mixed-case-arg.patch (4.8 KB) - added by Steffan Karger 6 years ago.
0003-Make-extract_x509_extension-in-ssl_verify_openssl.c-.patch (7.9 KB) - added by Steffan Karger 6 years ago.

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by andrisk

Attachment: options.c.patch added

Patch to src/openvpn/options.c

comment:1 Changed 6 years ago by Gert Döring

Milestone: release 2.3.5
Owner: set to Steffan Karger
Status: newassigned

syzzer: could you review the bug, patch, and send patch+ack to the list if fine?

comment:2 in reply to:  1 Changed 6 years ago by andrisk

Replying to cron2:

syzzer: could you review the bug, patch, and send patch+ack to the list if fine?

Yes, here are the relevant log entries from the following scenarios:

default CN authentication
=================================================
Thu May 14 12:00:38 2014 xx.xx.xx.xx:5288 VERIFY OK: depth=0, /O=Hewlett-Packard_Company

/OU=WEB/CN=Name_Surname/emailAddress=user@…


x509-username-field emailAddress (without patch)
=================================================
Thu May 14 12:05:44 2014 xx.xx.xx.xx:2732 VERIFY ERROR: could not extract EMAILADDRESS

from X509 subject string ('O=Hewlett-Packard Company, OU=WEB,
CN=Name Surname, emailAddress=user@…')


x509-username-field emailAddress (with patch)
=================================================
Thu May 15 12:10:05 2014 xx.xx.xx.xx:6062 VERIFY OK: depth=0, O=Hewlett-Packard Company,

OU=WEB, CN=Name Surname, emailAddress=user@…


comment:3 Changed 6 years ago by Steffan Karger

Thank you for reporting and supplying a patch. The patch however does not resolve the problem, it shifts it to a place where it doesn't harm your particular configuration. I think you were right in your first email; OpenVPN should probably not upper case at all.

There are two types of X509 attributes: 'standard' attributes (like, 'CN', 'S', 'OU', etc.), and 'extended' attributes. The patch removes the uppercasing for standard attributes, but introduces it for extended attributes. However, extended attributes can have lower case characters too.

From what I see in the git history, it seems that the original author of this code was a bit too helpful for administrators, automatically uppercasing for 'CN', 'OU', etc. Then someone else came along and added extended attribute matching, fixing the upper casing for his addition with the strncmp on 'ext:'.

So, I think the only sensible thing to do is to remove the upper casing from options.c, and match on both the upper cased and normal string in backend_x509_get_username(). I suggest issuing a deprecation warning when the upper casing was needed, and remove it all together in the future.

Last edited 6 years ago by Steffan Karger (previous) (diff)

Changed 6 years ago by andris

Attachment: version1.patch.zip added

Changed 6 years ago by andris

Attachment: version2.patch.zip added

comment:4 in reply to:  3 ; Changed 6 years ago by andris

Thanks for providing the background information on the --x509-username-field option.
I implemented your suggestion to give x509_get_username() the ability to retry a match
attempt with an uppercase field name in the attached file "version1.patch.zip".
Everything works fine if the first match succeeds. There's an undesirable side-effect,
however, when x509_get_username() does a retry under the following scenario:

  • Start OpenVPN with "--x509-username-field ou"
  • The following one-time error occurs after the first client connects:

Looking for 'ou' Subject attribute
VERIFY OK: depth=1, /O=hpl.hp.com/OU=Personal_Infrastructure/C=US/O=HP_Labs/CN=Andris_Kalnozols_Private_Certificate_Authority
Looking for 'ou' Subject attribute
Will retry with 'OU' Subject attribute
Looking for 'OU' Subject attribute
DEPRECATED FEATURE: matched --x509-username-field 'ou' username field only after upcasing it; please update your configuration
VERIFY OK: depth=0,/O=HP_Labs/OU=WEB/CN=Andris_Kalnozols/emailAddress=andris@…
TLS_ERROR: BIO read tls_read_plaintext error: error:0D06407A:asn1 encoding routines:a2d_ASN1_OBJECT:first num too large: error:0D06407A:asn1 encoding routines:a2d_ASN1_OBJECT:first num too large
TLS Error: TLS object -> incoming plaintext read error
TLS Error: TLS handshake failed
Fatal TLS error (check_tls_errors_co), restarting
SIGUSR1[soft,tls-error] received, client-instance restarting

  • Subsequent client connections succeed because x509_get_username() saves state via static variables and thereafter uses the uppercased field name as the first matching attempt.

It appears that tls_process() within the main event loop has a problem with extract_x509_field_ssl()
being called more than once by x509_get_username().

I then revisited options.c to refine its brute-force upcasing behavior. Now, the upcasing is done only if the option argument is all lowercase. Mixed-case arguments and those with the "ext:" prefix are left unchanged. This preserves the original intent of the "helpful" upcasing feature for backwards compatibility while limiting its scope in a straightforward way. I think this is a cleaner method to fix the uppercasing feature which is due to be deprecated anyway. These changes are in "version2.patch.zip".

I did enhance extract_x509_extension() in ssl_verify_openssl.c to be more informative but only when dealing with the client certificate. Getting access to the "cert_depth" variable required minor changes in the following files:

ssl_verify.c
ssl_verify_backend.h
ssl_verify_polarssl.c

Also, a gcc warning (return discards 'const' qualifier) was silenced by the changes to:

ssl_backend.h
ssl_openssl.c

These patches were tested with a client cert having the following Subject Alternative Name:

X509v3 Subject Alternative Name:
email:andris@hp.com, email:andris@hpl.hp.com, email:kalnozols@hpl.hp.com, DNS:andris.hpl.hp.com, URI:http://ftp.hpl.hp.com/pub/h2n/, Registered ID:1.2.3.4, IP Address:192.6.29.21, othername:<unsupported>, DirName:/C=US/O=hpl.hp.com/OU=IT Testing/CN=Andris Kalnozols Testing Authority

as well as compiling without the ENABLE_X509ALTUSERNAME feature.
The patch set was based on the 2.3.4 code base and includes updates to the openvpn(8) man page.

comment:5 Changed 6 years ago by Steffan Karger

Hi andris, thanks for your effort and the new patches. Just a quick heads-up for now; I will try to review this soon, but my spare time is a bit limited at the moment.

comment:6 in reply to:  4 ; Changed 6 years ago by Steffan Karger

Replying to andris:

I implemented your suggestion to give x509_get_username() the ability to retry a match
attempt with an uppercase field name in the attached file "version1.patch.zip".
Everything works fine if the first match succeeds. There's an undesirable side-effect,
however, when x509_get_username() does a retry under the following scenario:

  • Start OpenVPN with "--x509-username-field ou"
  • The following one-time error occurs after the first client connects:

[...]

  • Subsequent client connections succeed because x509_get_username() saves state via static variables and thereafter uses the uppercased field name as the first matching attempt.

It appears that tls_process() within the main event loop has a problem with extract_x509_field_ssl()
being called more than once by x509_get_username().

That is some nasty behaviour on the part of tls_process() and extract_x509_field_ssl(). I think it should be fixed at some point, but I agree that doesn't have to be part of this fix.

I then revisited options.c to refine its brute-force upcasing behavior. Now, the upcasing is done only if the option argument is all lowercase. Mixed-case arguments and those with the "ext:" prefix are left unchanged. This preserves the original intent of the "helpful" upcasing feature for backwards compatibility while limiting its scope in a straightforward way. I think this is a cleaner method to fix the uppercasing feature which is due to be deprecated anyway. These changes are in "version2.patch.zip".

I agree, so I worked with 'version 2' from here on. I applied your patch against the release/2.3 branch in git, and took the liberty to split the different additions into multiple commits - see attachments. This makes it easier to review and discuss them separately in multiple short time slots.

I'll send the first extracted commit (the manpage typo/spelling fixes) to the mailinglist, accompanied by an ACK in a minute.

The second extracted commit actually changes the x509-username-field behaviour discussed above. I'll send it to the mailinglist with some comments for further discussion soon.

I did enhance extract_x509_extension() in ssl_verify_openssl.c to be more informative but only when dealing with the client certificate. Getting access to the "cert_depth" variable required minor changes in the following files:

ssl_verify.c
ssl_verify_backend.h
ssl_verify_polarssl.c

Sounds welcome - extracted as commit 3. This basically replaces the previous implementation of extract_x509_extension, so it needs a proper review. I did not yet take a look at this, but I'll send it to the mailinglist for further discussion together with 'commit 2'.

Also, a gcc warning (return discards 'const' qualifier) was silenced by the changes to:

ssl_backend.h
ssl_openssl.c

This was already fixed in both the master and release/2.3 branches on git, so it's not part of the extracted patches anymore.

Changed 6 years ago by Steffan Karger

comment:7 in reply to:  6 Changed 6 years ago by andris

Steffan Karger from the mailing list improved the options.c patch from version 2 and the
issue with that appears to be settled.
In case my rework of extract_x509_extension() doesn't pass scrutiny, my reply to Steffan
suggested a change to a logging message in the existing function so that an ASN1 error
is not issued just because a non-email type GeneralName? appears in the extension field.

Thanks again for shepherding my suggested changes through the process.

comment:8 Changed 6 years ago by Steffan Karger

Resolution: fixed
Status: assignedclosed

... and finally the last patch has been applied too. 0003 did cause strange behaviour, so we settled for just fixing the log message.

Note: See TracTickets for help on using tickets.