From d56165d9303bd899f96b31eb86c081c66ee28dbb Mon Sep 17 00:00:00 2001 From: David Woodhouse Date: Tue, 15 Aug 2017 15:34:29 +0100 Subject: [PATCH] Clean up gpst parse_portal_xml() a little There was a potential crash if building up the XML in 'buf' didn't work, because buf->data could be NULL yet we still passed it back to vpninfo->write_new_config(). So check with buf_error() first. We can't use a 'static' form because in theory this can be invoked twice simultaneously for different VPNs, in the same process (and it isn't even *impossible* the way kde-plasma-nm handled authentication). Add a FIXME for the fact that we aren't escaping characters which need it in the XML we build up. Some other cosmetics, like using calloc() as $DEITY intended instead of duing the multiplication ourselves. Signed-off-by: David Woodhouse --- auth-globalprotect.c | 50 ++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/auth-globalprotect.c b/auth-globalprotect.c index 9035f9c5..f5d877b8 100644 --- a/auth-globalprotect.c +++ b/auth-globalprotect.c @@ -158,21 +158,25 @@ err_out: static int parse_portal_xml(struct openconnect_info *vpninfo, xmlNode *xml_node) { - static struct oc_auth_form form = {.message=(char *)"Please select GlobalProtect gateway.", .auth_id=(char *)"_portal"}; - + struct oc_auth_form form; xmlNode *x; struct oc_form_opt_select *opt; - struct oc_text_buf *buf; + struct oc_text_buf *buf = NULL; int max_choices = 0, result; char *portal = NULL; - opt = calloc(1, sizeof(*opt)); + form.message = (char *)_("Please select GlobalProtect gateway."); + form.auth_id = (char *)"_portal"; + + form.authgroup_opt = opt = calloc(1, sizeof(*opt)); if (!opt) return -ENOMEM; opt->form.type = OC_FORM_OPT_SELECT; opt->form.name = strdup("gateway"); opt->form.label = strdup(_("GATEWAY:")); + form.opts = (void *)opt; + /* The portal contains a ton of stuff, but basically none of it is useful to a VPN client * that wishes to give control to the client user, as opposed to the VPN administrator. * The exception is the list of gateways in policy/gateways/external/list @@ -188,38 +192,45 @@ static int parse_portal_xml(struct openconnect_info *vpninfo, xmlNode *xml_node) if (xmlnode_is_named(xml_node, "list")) goto gateways; result = -EINVAL; + free_opt(form.opts); free(portal); goto out; gateways: - buf = buf_alloc(); - buf_append(buf, "\n \n"); - if (portal) { - buf_append(buf, " %s%s", portal, vpninfo->hostname); - if (vpninfo->port!=443) - buf_append(buf, ":%d", vpninfo->port); - buf_append(buf, "/global-protect\n"); + if (vpninfo->write_new_config) { + buf = buf_alloc(); + buf_append(buf, "\n \n"); + if (portal) { + /* XXX: What if the name in 'portal' has characters which need to be + * escaped in XML? Either build up a tree using libxml "properly" + * so it does it for us, or at the very least we need a + * buf_append_xmlescaped(), don't we? */ + buf_append(buf, " %s%s", portal, vpninfo->hostname); + if (vpninfo->port!=443) + buf_append(buf, ":%d", vpninfo->port); + buf_append(buf, "/global-protect\n"); + } } free(portal); /* first, count the number of gateways */ - for (x = xml_node->children; x; x=x->next) + for (x = xml_node->children; x; x = x->next) if (xmlnode_is_named(x, "entry")) max_choices++; - opt->choices = calloc(1, max_choices * sizeof(struct oc_choice *)); + opt->choices = calloc(max_choices, sizeof(opt->choices[0])); if (!opt->choices) { - free_opt((struct oc_form_opt *)opt); + free_opt(form.opts); return -ENOMEM; } /* each entry looks like Label */ vpn_progress(vpninfo, PRG_INFO, _("%d gateway servers available:\n"), max_choices); - for (xml_node = xml_node->children; xml_node; xml_node=xml_node->next) { + for (xml_node = xml_node->children; xml_node; xml_node = xml_node->next) { if (xmlnode_is_named(xml_node, "entry")) { struct oc_choice *choice = calloc(1, sizeof(*choice)); if (!choice) { - free_opt((struct oc_form_opt *)opt); + free_opt(form.opts); return -ENOMEM; } @@ -232,17 +243,16 @@ gateways: opt->choices[opt->nr_choices++] = choice; vpn_progress(vpninfo, PRG_INFO, _(" %s (%s)\n"), - choice->label, choice->name); + choice->label, choice->name); } } buf_append(buf, " \n\n"); - if (vpninfo->write_new_config) + if (vpninfo->write_new_config && !buf_error(buf)) result = vpninfo->write_new_config(vpninfo->cbdata, buf->data, buf->pos); buf_free(buf); /* process static auth form to select gateway */ - form.opts = (struct oc_form_opt *)(form.authgroup_opt = opt); result = process_auth_form(vpninfo, &form); if (result != OC_FORM_RESULT_NEWGROUP) goto out; @@ -256,7 +266,7 @@ gateways: result = handle_redirect(vpninfo); out: - free_opt((struct oc_form_opt *)opt); + free_opt(form.opts); return result; } -- 2.49.0