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

[Daniel Lenski]

On Wed, Mar 7, 2018 at 11:34 AM, David Woodhouse  wrote:
>
>
> 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...

Right, it doesn't work.
https://stackoverflow.com/questions/10363380/libxml2-can%C2%B4t-get-content-from-node

>> > > +/* 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", ))
> vpninfo->ip_info.addr = add_option(vpninfo, "ipaddr", 
> s);
>
> That add_option() call *is* assuming that the 's' value will be stolen, 
> surely?

Indeed you're right. Current version is leaking memory every time I do
this and then don't free(s) … :facepalm:

This botched add_option() is the source of the memory leaks in the GP
version, as I'm now clearly seeing with valgrind. Will resubmit the patch.

Thanks,
dan

___
openconnect-devel mailing list
openconnect-devel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/openconnect-devel

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

[David Woodhouse]

On Wed, 2018-03-07 at 09:34 +, David Woodhouse wrote:
> 
> 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);

There's a problem with that suggestion if xmlNodeGetContent() on the
node returns NULL. Previously, xmlnode_get_text() would fail on that
one, while xmlnode_is_named() would have matched it. Is that a thing
that can actually happen? Do we need to cope gracefully, or merely make
sure we don't crash? 

smime.p7s
Description: S/MIME cryptographic signature
___
openconnect-devel mailing list
openconnect-devel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/openconnect-devel

Re: [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", ))
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 
#include 

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

smime.p7s
Description: S/MIME cryptographic signature
___
openconnect-devel mailing list
openconnect-devel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/openconnect-devel

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

[Daniel Lenski]

On Tue, Mar 6, 2018 at 11:40 AM, David Woodhouse  wrote:
> Thanks for tidying this up. Pushed to my gpst branch with one fixup so
> far, still reading through...
>
> On Sun, 2018-03-04 at 11:31 +0200, Daniel Lenski wrote:
>>
>> +/* similar to auth.c's xmlnode_get_text, including that *var should be 
>> freed by the caller,
>> +   but without the hackish param / %s handling that Cisco needs. And 
>> without freeing up
>> +   the old contents of *var, which is likely to lead to bugs? */
>
> Yeah... the way you're using it in gpst.c does lend mean that it's
> slightly more convenient to *not* free the previous contents. Otherwise
> various call sites would have to explicitly set s=NULL after
> xmlnode_get_text();add_option().
>
> I'm slightly concerned about having different semantics for the same
> (or similar) function in different files though; that is *also* likely
> to lead to bugs.

What do you prefer? Refactoring the two versions of xmlnode_get_text()
down to a single function, renaming the gpst.c version, something
else…?

>
>> +static int xmlnode_get_text(xmlNode *xml_node, const char *name, char **var)
>> +{
>> +   char *str;
>> +
>> +   if (name && !xmlnode_is_named(xml_node, name))
>> +   return -EINVAL;
>> +
>> +   str = (char *)xmlNodeGetContent(xml_node);
>> +   if (!str)
>> +   return -ENOENT;
>> +
>> +   *var = str;
>> +   return 0;
>> +}
>> +
>> +/* 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.

> 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.

-Dan

___
openconnect-devel mailing list
openconnect-devel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/openconnect-devel

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

[Daniel Lenski]

Here is a small patch to fix the comment on add_option…

diff --git a/gpst.c b/gpst.c
index 85987b2..1d5c748 100644
--- a/gpst.c
+++ b/gpst.c
@@ -84,11 +84,10 @@ static int xmlnode_get_text(xmlNode *xml_node,
const char *name, char **var)
 return 0;
 }

-/* We behave like CSTP — create a linked list in vpninfo->cstp_options
+/* We behave like CSTP and ONCP — 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) */
+ */

 static const char *add_option(struct openconnect_info *vpninfo, const
char *opt, const char *val)
 {

On Wed, Mar 7, 2018 at 10:01 AM, Daniel Lenski  wrote:
> On Tue, Mar 6, 2018 at 11:40 AM, David Woodhouse  wrote:
>> Thanks for tidying this up. Pushed to my gpst branch with one fixup so
>> far, still reading through...
>>
>> On Sun, 2018-03-04 at 11:31 +0200, Daniel Lenski wrote:
>>>
>>> +/* similar to auth.c's xmlnode_get_text, including that *var should be 
>>> freed by the caller,
>>> +   but without the hackish param / %s handling that Cisco needs. And 
>>> without freeing up
>>> +   the old contents of *var, which is likely to lead to bugs? */
>>
>> Yeah... the way you're using it in gpst.c does lend mean that it's
>> slightly more convenient to *not* free the previous contents. Otherwise
>> various call sites would have to explicitly set s=NULL after
>> xmlnode_get_text();add_option().
>>
>> I'm slightly concerned about having different semantics for the same
>> (or similar) function in different files though; that is *also* likely
>> to lead to bugs.
>
> What do you prefer? Refactoring the two versions of xmlnode_get_text()
> down to a single function, renaming the gpst.c version, something
> else…?
>
>>
>>> +static int xmlnode_get_text(xmlNode *xml_node, const char *name, char 
>>> **var)
>>> +{
>>> +   char *str;
>>> +
>>> +   if (name && !xmlnode_is_named(xml_node, name))
>>> +   return -EINVAL;
>>> +
>>> +   str = (char *)xmlNodeGetContent(xml_node);
>>> +   if (!str)
>>> +   return -ENOENT;
>>> +
>>> +   *var = str;
>>> +   return 0;
>>> +}
>>> +
>>> +/* 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.
>
>> 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.
>
> -Dan

___
openconnect-devel mailing list
openconnect-devel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/openconnect-devel

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

[David Woodhouse]

Thanks for tidying this up. Pushed to my gpst branch with one fixup so
far, still reading through...

On Sun, 2018-03-04 at 11:31 +0200, Daniel Lenski wrote:
> 
> +/* similar to auth.c's xmlnode_get_text, including that *var should be freed 
> by the caller,
> +   but without the hackish param / %s handling that Cisco needs. And without 
> freeing up
> +   the old contents of *var, which is likely to lead to bugs? */

Yeah... the way you're using it in gpst.c does lend mean that it's
slightly more convenient to *not* free the previous contents. Otherwise
various call sites would have to explicitly set s=NULL after
xmlnode_get_text();add_option().

I'm slightly concerned about having different semantics for the same
(or similar) function in different files though; that is *also* likely
to lead to bugs.

> +static int xmlnode_get_text(xmlNode *xml_node, const char *name, char **var)
> +{
> +   char *str;
> +
> +   if (name && !xmlnode_is_named(xml_node, name))
> +   return -EINVAL;
> +
> +   str = (char *)xmlNodeGetContent(xml_node);
> +   if (!str)
> +   return -ENOENT;
> +
> +   *var = str;
> +   return 0;
> +}
> +
> +/* 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?

Unless I'm being stupid, this should be a memory leak. Have you run in
valgrind with --leak-check=full? Please do.


> +   new->next = vpninfo->cstp_options;
> +   vpninfo->cstp_options = new;
> +
> +   return new->value;
> +}
> +

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'?


smime.p7s
Description: S/MIME cryptographic signature
___
openconnect-devel mailing list
openconnect-devel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/openconnect-devel