Opened 8 years ago

Closed 8 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:


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:

Attachments (1)

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

Download all attachments as: .zip

Change History (8)

comment:1 Changed 8 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 8 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.

Changed 8 years ago by Steffan Karger

comment:3 Changed 8 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 opted to check the cipher list on startup. Catching failures early probably saves some users a headache.

Version 0, edited 8 years ago by Steffan Karger (next)

comment:4 Changed 8 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 8 years ago by ValdikSS

Cron2, syzzer's patch works perfectly.

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

Owner: changed from valdikss to Steffan Karger

comment:7 Changed 8 years ago by Steffan Karger

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