Commas in commonName conflict with status_open files like --ifconfig-pool-persist and --status

[wdoekes]

As the subject already concisely points out, if you have a comma in your commonName, you get entries like these:

# cat /run/openvpn/server-status.log 
OpenVPN CLIENT LIST
Updated,Tue Mar  9 09:41:40 2021
Common Name,Real Address,Bytes Received,Bytes Sent,Connected Since
John Doe, Company,1.2.3.4:60900,1399590,4125576,Tue Mar  9 08:17:21 2021

^{- where "John Doe, Company" is the commonName, but is spread out over CSV fields 1 and 2.}

For the ifconfig-pool-persist file, this means you could have a commonName with an IP in it, like CN = John Doe,2.2.2.2 and the persist file could look like this:

# cat /var/spool/openvpn/client-addresses.txt 
John Doe,2.2.2.2,10.0.0.3
Someone Else,10.0.0.2

As far as I understand, there is no escaping in place and the comma is read as a simple delimiter:

​https://github.com/OpenVPN/openvpn/blob/v2.4.10/src/openvpn/pool.c#L492-L509

Can we do something about this? The current situation makes CNs with commas troublesome for machine reading of the status files. And it opens up a small possibility for tampering (moving someone else to a different IP, if you can control your CN).

Best would probably be to escape the field altogether (percent-encoding the comma and percents, for instance). That would make it backwards incompatible though.

Cheers,
Walter Doekes
OSSO B.V.

[Gert Döring]

Do not use commas in CNs. The company name should go into the O or OU field, not comma-separated into the CN...

Wrt "tampering" - do not use CAs that you do not trust.

What OpenVPN could do here is to string-replace the "," in the CN with something not breaking the file format (like "_"). Code needs to be written and tested to do so.

[Gert Döring]

That said, it's propably not very hard to do.

multi.c, multi_print_status() calls tls_common_name() to get the CN.

That output would need to be stuffed into string_mod_const() which makes a copy in the GC and modifies it

char *cn = tls_common_name(...);
char *cn_mod = string_mod_const(cn, CC_ANY, CC_COMMA, '_', &gc);

and then pass "cn_mod" to status_printf().

[tct]

Why not create compatible CNs in the first place: John_Doe-Company_Name

[wdoekes]

Hi!

(I'm sorry, I did not have email notifications explicitly enabled, so I hadn't noticed any replies.)

Thanks for you replies. Some feedback of mine below:

Do not use commas in CNs. The company name should go into the O or OU field, not comma-separated into the CN...

While I agree that that should have been done, I believe the CN does not disallow commas (*). So chances are they will be included by some people under some circumstances. And those who generate the certificates may not know that these are frowned upon.

Why not create compatible CNs in the first place: John_Doe-Company_Name

Sure. But I'm dealing with existing certificates here, created elsewhere. I cannot just create new certificates for everyone. If handing out certificates was zero effort, I would certainly have opted to generate new certificates. I'm sure you understand.

What OpenVPN could do here is to string-replace the "," in the CN with something not breaking the file format (like "_"). Code needs to be written and tested to do so.

I did consider this, but I figured that would make two CNs match the same entry: "foo,bar" == "foo_bar". But yeah, as you say: we should trust the CA too that there is some common sense there. Such a change would certainly be an improvement.

Cheers,
Walter

(*) As far as X.509 is concerned (see RFC 5280), the contents of [commonName] elements are irrelevant beyond equality comparisons; which means that you can put whatever sequence of characters you wish, as long as you do so consistently. [According to the internet.]