evgeny-gridasov / openvpn-otp

OpenVPN OTP token support plugin
GNU General Public License v3.0
279 stars 74 forks source link

resolve a possible null pointer derefence found by cppcheck #28

Open chipitsine opened 6 years ago

chipitsine commented 6 years ago

[src/otp.c:366] -> [src/otp.c:357]: (warning) Either the condition 'if(vpn_username&&!strcmp(vpn_username,user_entry.name)&&vpn_secret&&password&&!strcmp(vpn_secret,password))' is redundant or there is possible null pointer dereference: vpn_username.

evgeny-gridasov commented 6 years ago

vpn_username comes from openvpn_plugin_func_v1(...):

 const char *username = get_env ("username", envp);
 const char *password = get_env ("password", envp);

I think when openvpn plugin intercepts OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, the username and password will be set in the environment *envp[].

That check is indeed redundant, and I'd rather fail early in the openvpn_plugin_func_v1(...) function instead and return OPENVPN_PLUGIN_FUNC_ERROR if username or password ==NULL.

chipitsine commented 6 years ago

do you mean this https://github.com/evgeny-gridasov/openvpn-otp/blob/master/src/otp.c#L667-L669 ? or some extra check ?

evgeny-gridasov commented 6 years ago

Yes, I did mean that. Will add extra check soon to be safe. But I'm pretty sure there is a promise from openvpn to set those variables in environment.

chipitsine commented 6 years ago

ok, so you'll remove reduntant check by yourself, right ?

evgeny-gridasov commented 6 years ago

Yes.

k0ste commented 5 years ago

@chipitsine, resolved via https://github.com/evgeny-gridasov/openvpn-otp/commit/5fa5582305ee0211c2eb66188a74c9b5866bbb19?