search

OpenVPN / openvpn

OpenVPN is an open source VPN daemon
http://openvpn.net
Other
10.25k stars 2.92k forks source link

key_state_gen_auth_control_files has subtle logic mistake #535

Open gcoxmoz opened 2 months ago

gcoxmoz commented 2 months ago

Describe the bug https://github.com/OpenVPN/openvpn/blob/32e6586687a548174b88b64fe54bfae6c74d4c19/src/openvpn/ssl_verify.c#L996-L1013

It feels like this function has grown from 2 to 3 files over time, so minimally it feels like you want to make the if and return lines be if (acf && apf && afr) to cover afr's creation. That is, you could end up in the true areas if afr failed but the other two succeeded.
OR, if this is intentional, it looks like a miss and might warrant a comment.

To Reproduce Unknown, found by code reading. It's highly unlikely this edge case would fail you.

Expected behavior

Version information (please complete the following information):

Additional context

flichtenheld commented 2 months ago

This code was added in commit 8893fe49a4c593387d469ccc4a73ec0714f69315 before 2.6.0. It definitely looks like an oversight at first glance. On the other hand key_state_check_auth_failed_message_file has a check if (ads->auth_failed_reason_file) so it is handled and might be intentional. Since this wasn't brought up in the code review only @schwabe might know.

cron2 commented 2 months ago

This is safe (string_alloc() is well-defined for NULL pointers) but indeed looks a bit like an oversight. If creating any of these fails, "something is wrong with the system" (out of memory, disk full, ...) so I wouldn't consider "succeeding authentication" a good idea in that case - no matter which file failed. Anyway, assigning to @schwabe :-)