Opened 4 years ago

Closed 4 years ago

#737 closed Bug / Defect (fixed)

ncp-ciphers are not validated early

Reported by: ValdikSS Owned by: Steffan Karger
Priority: minor Milestone: release 2.4
Component: Crypto Version: OpenVPN git master branch (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords:
Cc:

Description

ncp-ciphers are validated only upon connection. This is not a problem for a client but a problem for server. If you made a typo or just set invalid cipher in ncp-ciphers, the server would work fine with 2.3 clients but would terminate when 2.4 client would try to connect.

Proposed patch: https://github.com/ValdikSS/openvpn-with-patches/commit/74613253972252d871fd5855a934b8ce31010424

Attachments (1)

0001-Check-ncp-ciphers-list-on-startup.patch (6.2 KB) - added by Steffan Karger 4 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 4 years ago by Selva Nair

The patch uses strtok (p[1], ":"); which will modify p[1] and break later use of o->ncp_ciphers. Need to make a copy of p[1] using string_alloc (p[1], NULL) or some such and then tokenize the copy.

comment:2 Changed 4 years ago by ValdikSS

Wow, you're right. I didn't know strtok works that way and it worked as expected while I was testing. Check v2 please.
https://github.com/ValdikSS/openvpn-with-patches/commit/3d9e613b090212ea41a94ed11c44e4e94b1e3306

Changed 4 years ago by Steffan Karger

comment:3 Changed 4 years ago by Steffan Karger

Sorry, I didn't realize that there were already patches for this. Attached a patch that I just wrote to fix this.

Instead of checking this during connection, I also opted to check the cipher list on startup. Catching failures early probably saves some users a headache.

Edit:
Just looked at ValdikSS' patch. Checking options shouldn't be done in the options parsing, but rather in the options checking functions.

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

comment:4 Changed 4 years ago by Gert Döring

Owner: set to valdikss
Status: newassigned

Patch looks reasonable on first glane. @ValdikSS: can you please test and report back? Then syzzer can send to list, and we can merge...

comment:5 Changed 4 years ago by ValdikSS

Cron2, syzzer's patch works perfectly.

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

Owner: changed from valdikss to Steffan Karger

comment:7 Changed 4 years ago by Steffan Karger

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.