[PATCH 2/5] add PAN GlobalProtect protocol support (HTTPS tunnel only)

[David Woodhouse]

On Wed, 2018-03-07 at 10:01 +0200, Daniel Lenski wrote:
> What do you prefer? Refactoring the two versions of xmlnode_get_text()
> down to a single function, renaming the gpst.c version, something
> else…?

Don't know... one option is to ditch it entirely. Some of those cases
where you're just using atoi(s); free(s); after it might be better done
with xmlnode_is_named() and atoi(xml_node->content) perhaps? 

ISTR you saying that referencing node->content doesn't work? I don't
recall the details...


> > > +/* We behave like CSTP — create a linked list in vpninfo->cstp_options
> > > + * with the strings containing the information we got from the server,
> > > + * and oc_ip_info contains const copies of those pointers.
> > > + *
> > > + * (unlike version in oncp.c, val is stolen rather than strdup'ed) */
> > Er...
> > 
> > > 
> > > +
> > > +static const char *add_option(struct openconnect_info *vpninfo, const char *opt, const char *val)
> > .... make it 'char *val' then, if it's stolen...
> > 
> > > 
> > > +{
> > > +       struct oc_vpn_option *new = malloc(sizeof(*new));
> > > +       if (!new)
> > > +               return NULL;
> > > +
> > > +       new->option = strdup(opt);
> > > +       if (!new->option) {
> > > +               free(new);
> > > +               return NULL;
> > > +       }
> > > +       new->value = strdup(val);
> >
> > ... but I still see a strdup(). And should it *free* 'val' in the error
> > patch if it was going to take ownership of it?
>
> Comment is straight-up wrong and obsolete. We changed this in a
> previous iteration of the patches. Good catch.

But then you are *using* add_option as the comment describes it:

	for (xml_node = xml_node->children; xml_node; xml_node=xml_node->next) {
		if (!xmlnode_get_text(xml_node, "ip-address", &s))
			vpninfo->ip_info.addr = add_option(vpninfo, "ipaddr", s);

That add_option() call *is* assuming that the 's' value will be stolen, surely?

Maybe we should pass the xmlNode into add_option() not a string? Then
it's nice and simple. And more xmlnode_get_text() invocations can turn
into simple xmlnode_is_named()? The above code becomes

	for (xml_node = xml_node->children; xml_node; xml_node=xml_node->next) {
		if (!xmlnode_is_named(xml_node, "ip-address"))
			vpninfo->ip_info.addr = add_option(vpninfo, "ipaddr", xml_node);


> > 
> > Also, in parse_javascript(), consider an input line which looks like:
> > 
> >    var respMsg = ";
> > 
> > When you set '*prompt = strndup(start, end-start-2);
> > 
> > ... what is the value of 'end-start-2'?
> 0 in that case. I've already verified that end[-1] == ';' && end[-2]
> == '"' before getting to this point, so it's not possible for it to be
> negative.

#include <stdio.h>
#include <string.h>

int main(void)
{
	const char *pre_prompt = "var respMsg = \"";
	char *attack = "var respMsg = \";\n";
	char *start, *end = attack;

	start = end+strlen(pre_prompt);
	end = strchr(start, '\n');
	if (!end || end[-1] != ';' || end[-2] != '"') {
		printf("no match\n");
		return 1;
	}
	printf("end is %p, start %p, end-start-2 %ld\n",
	       end, start, (long)(end-start-2));
	return 0;
}

$ ./f
end is 0x400758, start 0x400757, end-start-2 -1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5213 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openconnect-devel/attachments/20180307/966d8a17/attachment-0001.bin>