| Message ID | 20230201041831.2259419-1-selva.nair@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] block-dns using iservice: fix a potential double free | expand |
Hi, I made a slightly different fix but then noticed your mail. Indeed the problem is that get/set_interface_metric fails, and we call FwpmEngineClose0 after updating the undo list. When openvpn process exits, we execute commands in undo list, and second call to FwpmEngineClose0 causes Access Violation inside WFP. Note that since get_interface_metric() fails, it sets interface_data->metric_v4 to -1. In this case we should not restore metrics back (which will fail in any case). We have a very similar code (including metrics value check) just below which handles msg_del_block_dns. Can we factor it out into a separate function?
Hi, On Wed, Feb 1, 2023 at 4:37 AM Lev Stipakov <lstipakov@gmail.com> wrote: > Hi, > > I made a slightly different fix but then noticed your mail. > > Indeed the problem is that get/set_interface_metric fails, > and we call FwpmEngineClose0 after updating the undo list. When > openvpn process exits, we execute commands in undo list, > and second call to FwpmEngineClose0 causes Access Violation > inside WFP. > > Note that since get_interface_metric() fails, it sets > interface_data->metric_v4 > to -1. In this case we should not restore metrics back (which will > fail in any case). > We have a very similar code (including metrics value check) just below > which > handles msg_del_block_dns. Can we factor it out into a separate function? > Good point. I have a version that splits "add" and "delete" actions into separate functions and does something like this. Please take a look here: https://github.com/selvanair/openvpn/tree/block-dns-fix The add and delete functions are in that order (with a forward declaration) only to make review easier (less changes with ignore-all-spaces), will move the latter up if it looks okay. That said, if you have a better fix, I'm ready to review it in lieu of this. Thanks, Selva
Hi, > Good point. I have a version that splits "add" and "delete" actions into separate functions and does something like this. > > Please take a look here: https://github.com/selvanair/openvpn/tree/block-dns-fix > The add and delete functions are in that order (with a forward declaration) only to make review easier (less changes with ignore-all-spaces), will move the latter up if it looks okay. Looks good to me, especially without whitespaces https://github.com/OpenVPN/openvpn/pull/235/files?diff=unified&w=1 > That said, if you have a better fix, I'm ready to review it in lieu of this. My change is more intrusive, idea is to bail out if get_interface_metrics fails and update undo_list only if both filters and metrics are applied, https://github.com/lstipakov/openvpn/commit/8c2d0de178e0771295764ea773596c13ff5f2aa6 It is a bit sloppy on metrics cleanup, though - for example if get_interface_metric for ipv6 fails but ipv4 is not - it won't restore ipv4 metrics. On the other hand this is unlikely to happen. Also I replaced 0x%x with %u in win_block_dns_service() for consistency. You may want to do it in your patch too :)
> > > Also I replaced 0x%x with %u in win_block_dns_service() for > consistency. You may want to do it in your patch too :) > We have at least another place where it's %x, so will leave that for another day. btw, shouldn't it be %d? Selva
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index 03361d66..636f32e9 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -780,7 +780,7 @@ static DWORD HandleBlockDNSMessage(const block_dns_message_t *msg, undo_lists_t *lists) { DWORD err = 0; - block_dns_data_t *interface_data; + block_dns_data_t *interface_data = NULL; HANDLE engine = NULL; LPCWSTR exe_path; @@ -794,6 +794,7 @@ HandleBlockDNSMessage(const block_dns_message_t *msg, undo_lists_t *lists) interface_data = malloc(sizeof(block_dns_data_t)); if (!interface_data) { + delete_block_dns_filters(engine); return ERROR_OUTOFMEMORY; } interface_data->engine = engine; @@ -818,8 +819,17 @@ HandleBlockDNSMessage(const block_dns_message_t *msg, undo_lists_t *lists) BLOCK_DNS_IFACE_METRIC); if (!err) { - set_interface_metric(msg->iface.index, AF_INET6, - BLOCK_DNS_IFACE_METRIC); + err = set_interface_metric(msg->iface.index, AF_INET6, + BLOCK_DNS_IFACE_METRIC); + } + if (err) + { + /* try to restore the interface metric values in case changed */ + set_interface_metric(msg->iface.index, AF_INET, + interface_data->metric_v4); + set_interface_metric(msg->iface.index, AF_INET, + interface_data->metric_v4); + RemoveListItem(&(*lists)[block_dns], CmpAny, NULL); } } } @@ -853,6 +863,7 @@ HandleBlockDNSMessage(const block_dns_message_t *msg, undo_lists_t *lists) if (err && engine) { delete_block_dns_filters(engine); + free(interface_data); } return err;