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: | |
| 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)
Change History (8)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
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 8 years ago by
| Attachment: | 0001-Check-ncp-ciphers-list-on-startup.patch added |
|---|
comment:3 Changed 8 years ago by
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.
comment:4 Changed 8 years ago by
| Owner: | set to valdikss |
|---|---|
| Status: | new → assigned |
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:6 Changed 8 years ago by
| Owner: | changed from valdikss to Steffan Karger |
|---|
comment:7 Changed 8 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
Patch has been applied:
https://github.com/OpenVPN/openvpn/commit/dc4fa3c4
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] usingstring_alloc (p[1], NULL)or some such and then tokenize the copy.