Opened 5 months ago

Last modified 5 weeks ago

#1391 new Bug / Defect

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

Reported by: wdoekes Owned by:
Priority: major Milestone:
Component: Generic / unclassified Version: OpenVPN git master branch (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords:
Cc:

Description

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.

Change History (3)

comment:1 Changed 4 months ago by 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.

comment:2 Changed 4 months ago by 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().

comment:3 Changed 5 weeks ago by tct

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

Note: See TracTickets for help on using tickets.