Fixed xpath crashes discovered by fuzzing

Added fuzz code for xpath
Test: added negative xpath tests

CHANGELOG.md

@@ -80,13 +80,15 @@ Developers may need to change their code
   
 ### Minor features
 
-* List-pagination: Adhere to ietf-draft: Removed list-pagination "presence"
+* Added fuzz code for xpath
+* List-pagination: Follow ietf-draft 100%: Removed list-pagination "presence"
 * Main example: Removed dependency of external IETF RFCs
   * See [Can't initiate clixon_backend](https://github.com/clicon/clixon/issues/382)
 * Added warning if modstate is not present in datastore if `CLICON_XMLDB_MODSTATE` is set.
 
 ### Corrected Bugs
 
+* Fixed several xpath crashes discovered by unit xpath fuzzing
 * Fixed: SEGV when using NETCONF get filter xpath and non-existent key
   * eg `select="/ex:table[ex:non-exist='a']`
 * Fixed: [CLI Show config JSON with multiple top-level elements is broken](https://github.com/clicon/clixon/issues/381)

README.md

@@ -19,4 +19,4 @@ Clixon interaction is best done posting issues, pull requests, or joining the
 [slack channel](https://clixondev.slack.com).
 [Slack invite](https://join.slack.com/t/clixondev/shared_invite/zt-1fz8at20n-KGNmKD7KH2j2jIFGU_EhMQ)(updated 15/9 2022, valid 30d)
 
-Clixon is sponsored by [Rubicon Communications LLC(Netgate)](https://www.netgate.com/)
+Clixon is sponsored by [Rubicon Communications LLC(Netgate)](https://www.netgate.com/) and [Akamai Technologies, Inc.](https://www.akamai.com).

apps/netconf/netconf_main.c

@@ -533,9 +533,11 @@ netconf_input_cb(int   s,
             goto done;
     if (clicon_option_exists(h, NETCONF_FRAME_SIZE) == 0)
         frame_size = 0;
-    else
+    else{
-        if ((frame_size = clicon_option_int(h, NETCONF_FRAME_SIZE)) < 0)
+        if ((ret = clicon_option_int(h, NETCONF_FRAME_SIZE)) < 0)
             goto done;
+        frame_size = (size_t)ret;
+    }
     if ((ptr = clicon_hash_value(cdat, NETCONF_HASH_BUF, &cdatlen)) != NULL){
         if (cdatlen != sizeof(cb)){
             clicon_err(OE_XML, errno, "size mismatch %lu %lu",

apps/restconf/clixon_http_data.c

@@ -351,10 +351,11 @@ api_http_data_file(clicon_handle h,
         clicon_err(OE_UNIX, errno, "malloc");
         goto done;
     }
-    if ((sz = fread(buf, fsize, 1, f)) < 0){
+    if ((ret = fread(buf, fsize, 1, f)) < 0){
         clicon_err(OE_UNIX, errno, "fread");
         goto done;
     }
+    sz = (size_t)ret;
     if (sz != 1){
         clicon_debug(1, "%s Error fread(%s) sz:%zu", __FUNCTION__, filename, sz);
         if (api_http_data_err(h, req, 500) < 0) /* Internal error? */

lib/src/clixon_xpath_eval.c

@@ -433,7 +433,10 @@ xp_eval_step(xp_ctx     *xc0,
         *xrp = xc;
         xc = NULL;
     }
-    assert(*xrp);
+    if (*xrp == NULL){
+        clicon_err(OE_XML, 0, "Internal error xrp is NULL");
+        goto done;
+    }
     retval = 0;
  done:
     if (xc)
@@ -488,9 +491,11 @@ xp_eval_predicate(xp_ctx     *xc,
         if ((xr0 = ctx_dup(xc)) == NULL)
             goto done;
     }
-    if (xs->xs_c1){ /* Second child */
+    // alt set nodeset to NULL
-        /* Loop over each node in the nodeset */
+    if (xs->xs_c1 && xr0->xc_type == XT_NODESET){ /* Second child */
-        assert (xr0->xc_type == XT_NODESET);
+        /* Loop over each node in the nodeset 
+         * XXX: alt to check xr0 is nodeset: set new var nodeset to NULL
+         */
         if ((xr1 = malloc(sizeof(*xr1))) == NULL){
             clicon_err(OE_UNIX, errno, "malloc");
             goto done;
@@ -707,6 +712,7 @@ xp_relop(xp_ctx    *xc1,
     char   *s2;
     int     reverse = 0;
     double  n1, n2;
+    char   *xb;
     
     if (xc1 == NULL || xc2 == NULL){
         clicon_err(OE_UNIX, EINVAL, "xc1 or xc2 NULL");
@@ -725,12 +731,15 @@ xp_relop(xp_ctx    *xc1,
             /* If both are node-sets, then it is true iff the string value of one 
                node in the first node-set and one in the second node-set is true */
             for (i=0; i<xc1->xc_size; i++){
-                if ((s1 = xml_body(xc1->xc_nodeset[i])) == NULL){
+                /* node in nodeset */
+                if ((x = xc1->xc_nodeset[i]) == NULL ||
+                    (s1 = xml_body(x)) == NULL){
                     xr->xc_bool = 0;
                     goto ok;
                 }
                 for (j=0; j<xc2->xc_size; j++){
-                    if ((s2 = xml_body(xc2->xc_nodeset[j])) == NULL){
+                    if ((x = xc2->xc_nodeset[j]) == NULL ||
+                        (s2 = xml_body(x)) == NULL){
                         xr->xc_bool = 0;
                         goto ok;
                     }
@@ -839,8 +848,11 @@ xp_relop(xp_ctx    *xc1,
                the other string is true.*/
             s2 = xc2->xc_string;
             for (i=0; i<xc1->xc_size; i++){
-                x = xc1->xc_nodeset[i]; /* node in nodeset */
+                /* node in nodeset */
-                s1 = xml_body(x);
+                if ((x = xc1->xc_nodeset[i]) == NULL)
+                    s1 = NULL;
+                else
+                    s1 = xml_body(x);
                 switch(op){
                 case XO_EQ:
                     if (s1 == NULL && s2 == NULL)
@@ -877,8 +889,10 @@ xp_relop(xp_ctx    *xc1,
             break;
         case XT_NUMBER:
             for (i=0; i<xc1->xc_size; i++){
-                x = xc1->xc_nodeset[i]; /* node in nodeset */
+                /* node in nodeset */
-                if (sscanf(xml_body(x), "%lf", &n1) != 1)
+                if ((x = xc1->xc_nodeset[i]) == NULL ||
+                    (xb = xml_body(x)) == NULL ||
+                    sscanf(xb, "%lf", &n1) != 1)
                     n1 = NAN;
                 n2 = xc2->xc_number;
                 switch(op){

lib/src/clixon_xpath_parse.y

@@ -330,8 +330,9 @@ xp_nodetest_function(clixon_xpath_yacc *xpy,
     xpath_tree                *xtret = NULL;
     cbuf                      *cb = NULL;
     enum clixon_xpath_function fn;
+    int                        ret;
 
-    if ((fn = xp_fnname_str2int(name)) < 0){
+    if ((ret = xp_fnname_str2int(name)) < 0){
         if ((cb = cbuf_new()) == NULL){
             clicon_err(OE_XML, errno, "cbuf_new");
             goto done;
@@ -340,6 +341,7 @@ xp_nodetest_function(clixon_xpath_yacc *xpy,
         clixon_xpath_parseerror(xpy, cbuf_get(cb));
         goto done;
     }
+    fn = (enum clixon_xpath_function)ret;
     switch (fn){
     case XPATHFN_COMMENT:  /* Group of not implemented node functions */
     case XPATHFN_PROCESSING_INSTRUCTIONS:

lib/src/clixon_yang_cardinality.c

@@ -457,7 +457,7 @@ static const struct ycard _yclist[] = {
 };
 
 /* Search matrix for lookups */
-static const struct ycard *_yc_search[Y_SPEC][Y_SPEC] = {0,};
+static const struct ycard *_yc_search[Y_SPEC][Y_SPEC] = {{0,},{0,}};
 
 /* Set to 1 if exists in search 
  * Some yang statements are not explicitly given cardinalities in RFC7950, although they are

test/config.sh.in

@@ -76,6 +76,7 @@ CLIXON_AUTOCLI_REV="2022-02-11"
 CLIXON_LIB_REV="2021-12-05"
 CLIXON_CONFIG_REV="2022-03-21"
 CLIXON_RESTCONF_REV="2022-08-01"
+CLIXON_EXAMPLE_REV="2022-11-01"
 
 # Length of TSL RSA key
 # Problem with small key such as 1024 not allowed in centos8 for example (why is this)

test/fuzz/xpath/README.md

@@ -0,0 +1,31 @@
+# Clixon xpath fuzzing
+
+This dir contains code for fuzzing clixon xpaths. 
+
+## Prereqs
+
+Install AFL, see [..](..)
+
+## Build
+
+Build clixon clixon_util_xpath statically with the afl-clang compiler:
+
+```
+  CC=/usr/bin/afl-clang-fast LINKAGE=static INSTALLFLAGS="" ./configure
+  make clean
+  cd lib
+  make
+  sudo make install
+  cd ../util
+  make clixon_util_xpath
+  sudo install clixon_util_xpath /usr/local/bin/ # some utils have complex dependencies
+```
+
+## Run tests
+
+Run the script `runfuzz.sh` to run one test with a yang spec and an input string, eg:
+```
+  ./runfuzz.sh
+```
+
+After (or during) the test, investigate results in the output dir.

test/fuzz/xpath/input/1.xpath

@@ -0,0 +1 @@
+/ex:table[ex:parameter='x']

test/fuzz/xpath/runfuzz.sh

@@ -0,0 +1,34 @@
+#!/usr/bin/env bash
+# Run a fuzzing test using american fuzzy lop
+set -eux
+
+if [ $# -ne 0 ]; then 
+    echo "usage: $0\n"
+    exit 255
+fi
+
+APPNAME=example
+xml=example.xml
+
+
+cat <<EOF > $xml
+<table xmlns="urn:example:clixon">
+   <parameter>
+      <name>x</name>
+      <value>42</value>
+   </parameter>
+</table>
+EOF
+
+MEGS=500 # memory limit for child process (50 MB)
+
+# remove input and input dirs
+#test ! -d input || rm -rf input
+test ! -d output || rm -rf output
+
+# create if dirs dont exists
+#test -d input || mkdir input
+test -d output || mkdir output
+
+# Run script 
+afl-fuzz -i input -o output -m $MEGS -- clixon_util_xpath -f $xml -n ex:urn:example:clixon -y /usr/local/share/clixon/clixon-example@2022-11-01.yang  -Y /usr/local/share/clixon

test/test_autocli_extension.sh

@@ -133,10 +133,6 @@ EOF
     new "check datastore using netconf"
     expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><get-config><source><candidate/></source><filter type=\"xpath\" select=\"/ex:table/ex:parameter[ex:name='x']\" xmlns:ex=\"urn:example:clixon\" /></get-config></rpc>" "" "<rpc-reply $DEFAULTNS><data>$XML</data></rpc-reply>"
 
-    # Test not right context but could not find other test where it fits
-    new "negative test"
-    expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><get-config><source><candidate/></source><filter type=\"xpath\" select=\"/ex:table[ex:parameter='x']\" xmlns:ex=\"urn:example:clixon\" /></get-config></rpc>" "" "<rpc-reply $DEFAULTNS><data/></rpc-reply>"
-
     new "check datastore direct access"
     expectpart "$($clixon_util_datastore -d candidate -b $dir -y $fyang -Y ${YANG_INSTALLDIR} -Y $dir get /)" 0 "$XML"
 

test/test_netconf.sh

@@ -431,9 +431,17 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS>
 new "netconf xpath syntax error (api-path not xpath) should fail"
 expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><get-config><source><candidate/></source><filter type=\"xpath\" select=\"/if:interfaces/interface=eth2f0,foo/fii\" xmlns:if=\"urn:ietf:params:xml:ns:yang:ietf-interfaces\"/></get-config></rpc>" "" "<rpc-reply $DEFAULTNS><rpc-error><error-type>application</error-type><error-tag>operation-failed</error-tag><error-severity>error</error-severity><error-message>xpath parser on line 1: syntax error at or before: ','</error-message></rpc-error></rpc-reply>"
 
-new "netconf wrong xpath should fail"
+new "netconf xpath syntax error"
+rpc="<rpc $DEFAULTNS><get-config><source><candidate/></source><filter type=\"xpath\" select=\"/if:interfaces=ex*paramet='x']\" xmlns:if=\"urn:ietf:params:xml:ns:yang:ietf-interfaces\"/></get-config></rpc>"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "$rpc" "" "<rpc-reply $DEFAULTNS><rpc-error><error-type>application</error-type><error-tag>operation-failed</error-tag><error-severity>error</error-severity><error-message>xpath parser on line 1: syntax error at or before: ']'</error-message></rpc-error></rpc-reply>"
+
+new "netconf not found xpath should fail"
 expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><get-config><source><candidate/></source><filter type=\"xpath\" select=\"/if:interfaces/interface=eth2f0/fii\" xmlns:if=\"urn:ietf:params:xml:ns:yang:ietf-interfaces\"/></get-config></rpc>" "" "<rpc-reply $DEFAULTNS><data/></rpc-reply>"
 
+new "netconf xpath mixed types"
+rpc="<rpc $DEFAULTNS><get-config><source><candidate/></source><filter type=\"xpath\" select=\"/if:interfaces[ex*p>@er='x']\" xmlns:if=\"urn:ietf:params:xml:ns:yang:ietf-interfaces\"/></get-config></rpc>"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "$rpc" "" "<rpc-reply $DEFAULTNS><rpc-error><error-type>application</error-type><error-tag>operation-failed</error-tag><error-severity>error</error-severity><error-message>Get candidate datastore: Mixed types not supported, 1 3</error-message></rpc-error></rpc-reply>"
+
 if [ $BE -ne 0 ]; then
     new "Kill backend"
     # Check if premature kill

test/test_xpath.sh

@@ -120,7 +120,6 @@ cat <<EOF > $xmlfn
 </root>
 EOF
 
-
 new "xpath not(aaa)"
 expectpart "$($clixon_util_xpath -D $DBG -f $xml -p "not(aaa)")" 0 "bool:false"
 
@@ -170,6 +169,9 @@ expectpart "$($clixon_util_xpath -D $DBG -f $xml -p //bbb[0])" 0 "^nodeset:0:<bb
 new "xpath //bbb[ccc=99]"
 expectpart "$($clixon_util_xpath -D $DBG -f $xml -p //bbb[ccc=99])" 0 "^nodeset:0:<bbb x=\"bye\"><ccc>99</ccc></bbb>$"
 
+new "Negative: xpath [x=] on a variable that has no body"
+expectpart "$($clixon_util_xpath -D $DBG -f $xml -p "/aaa[bbb='a']")" 0 "nodeset:"
+
 new "xpath ../connection-type = 'responder-only'"
 expectpart "$($clixon_util_xpath -D $DBG -f $xml2 -p "../connection-type='responder-only'" -i /aaa/bbb/here)" 0 "^bool:true$"
 
@@ -323,6 +325,72 @@ expectpart "$($clixon_util_xpath -D $DBG -f $xmlfn -p "root/count/node[99=ancest
 new "xpath functions as ncname: functioname:count"
 expectpart "$($clixon_util_xpath -D $DBG -f $xmlfn -p "root/node/ancestor[73=count]")" 0 "<ancestor><count>73</count></ancestor>"
 
+# Negative tests from fuzz crashes
+cat <<EOF > $dir/1.xml
+<table xmlns="urn:example:clixon">
+   <parameter>
+      <name>x</name>
+      <value>42</value>
+   </parameter>
+</table>
+EOF
+
+cat <<EOF > $dir/1.xpath
+/ex:table=ex*paramet
+EOF
+
+new "negative xpath 1"
+expectpart "$($clixon_util_xpath -D $DBG -f $dir/1.xml -n ex:urn:example:clixon -y /usr/local/share/clixon/clixon-example@${CLIXON_EXAMPLE_REV}.yang  -Y /usr/local/share/clixon < $dir/1.xpath)" 0 "bool:false"
+
+cat <<EOF > $dir/1.xpath
+ter='x'/ex:table[exmeter='x']
+EOF
+
+new "negative xpath 2"
+expectpart "$($clixon_util_xpath -D $DBG -f $dir/1.xml -n ex:urn:example:clixon -y /usr/local/share/clixon/clixon-example@${CLIXON_EXAMPLE_REV}.yang  -Y /usr/local/share/clixon < $dir/1.xpath)" 0 "bool:false"
+
+cat <<EOF > $dir/1.xpath
+/ex:table<ex*ptramble
+EOF
+
+new "negative xpath 3"
+expectpart "$($clixon_util_xpath -D $DBG -f $dir/1.xml -n ex:urn:example:clixon -y /usr/local/share/clixon/clixon-example@${CLIXON_EXAMPLE_REV}.yang  -Y /usr/local/share/clixon < $dir/1.xpath)" 0 "bool:false"
+
+cat <<EOF > $dir/1.xpath
+7/ex:table['x']
+EOF
+
+new "negative xpath 4"
+expectpart "$($clixon_util_xpath -D $DBG -f $dir/1.xml -n ex:urn:example:clixon -y /usr/local/share/clixon/clixon-example@${CLIXON_EXAMPLE_REV}.yang  -Y /usr/local/share/clixon < $dir/1.xpath)" 0 "number:7"
+
+cat <<EOF > $dir/1.xpath
+/>meter*//ter
+EOF
+
+new "negative xpath 5"
+expectpart "$($clixon_util_xpath -D $DBG -f $dir/1.xml -n ex:urn:example:clixon -y /usr/local/share/clixon/clixon-example@${CLIXON_EXAMPLE_REV}.yang  -Y /usr/local/share/clixon < $dir/1.xpath)" 0 "bool:false"
+
+cat <<EOF > $dir/1.xpath
+7=/ ter
+EOF
+
+new "negative xpath 6"
+expectpart "$($clixon_util_xpath -D $DBG -f $dir/1.xml -n ex:urn:example:clixon -y /usr/local/share/clixon/clixon-example@${CLIXON_EXAMPLE_REV}.yang  -Y /usr/local/share/clixon < $dir/1.xpath)" 0 "bool:false"
+
+cat <<EOF > $dir/1.xpath
+/=7 ter
+EOF
+
+new "negative xpath 7"
+#expectpart "$($clixon_util_xpath -D $DBG -f $dir/1.xml -n ex:urn:example:clixon -y /usr/local/share/clixon/clixon-example@${CLIXON_EXAMPLE_REV}.yang  -Y /usr/local/share/clixon < $dir/1.xpath)" 0 "bool:false"
+
+cat <<EOF > $dir/1.xpath
+*<-9****
+EOF
+
+new "negative xpath 8"
+expectpart "$($clixon_util_xpath -D $DBG -f $dir/1.xml -n ex:urn:example:clixon -y /usr/local/share/clixon/clixon-example@${CLIXON_EXAMPLE_REV}.yang  -Y /usr/local/share/clixon < $dir/1.xpath)" 0 "bool:false"
+
 rm -rf $dir
 
 # unset conditional parameters 

util/Makefile.in

@@ -83,9 +83,9 @@ endif
 
 # For dependency. A little strange that we rely on it being built in the src dir
 # even though it may exist in $(libdir). But the new version may not have been installed yet.
-LIBDEPS	    = $(top_srcdir)/apps/backend/$(CLIXON_BACKEND_LIB)
-LIBDEPS	   += $(top_srcdir)/lib/src/$(CLIXON_LIB)
 
+LIBDEPS	   = $(top_srcdir)/lib/src/$(CLIXON_LIB)
+BELIBDEPS  = $(top_srcdir)/apps/backend/$(CLIXON_BACKEND_LIB)
 
 # Utilities, unit testings. Not installed.
 APPSRC    = clixon_util_xml.c
@@ -155,11 +155,11 @@ clixon_util_regexp: clixon_util_regexp.c $(LIBDEPS)
 clixon_util_socket: clixon_util_socket.c $(LIBDEPS)
 	$(CC) $(INCLUDES) $(CPPFLAGS) @CFLAGS@ $(LDFLAGS) $^ $(LIBS) -o $@
 
-clixon_util_validate: clixon_util_validate.c $(LIBDEPS) 
+clixon_util_validate: clixon_util_validate.c $(LIBDEPS) $(BELIBDEPS) 
-	$(CC) $(INCLUDES) $(CPPFLAGS) @CFLAGS@ $(LDFLAGS) $^ -l clixon_backend -o $@ $(LIBS)
+	$(CC) $(INCLUDES) $(CPPFLAGS) @CFLAGS@ $(LDFLAGS) $^ -l clixon_backend -o $@ $(LIBS) $(BELIBS)
 
-clixon_util_dispatcher: clixon_util_dispatcher.c $(LIBDEPS) 
+clixon_util_dispatcher: clixon_util_dispatcher.c $(LIBDEPS) $(BELIBDEPS) 
-	$(CC) $(INCLUDES) $(CPPFLAGS) @CFLAGS@ $(LDFLAGS) $^ -l clixon_backend -o $@ $(LIBS)
+	$(CC) $(INCLUDES) $(CPPFLAGS) @CFLAGS@ $(LDFLAGS) $^ -l clixon_backend -o $@ $(LIBS) $(BELIBS)
 
 ifdef with_restconf
 clixon_util_stream: clixon_util_stream.c $(LIBDEPS)

util/clixon_util_xpath.c

@@ -367,8 +367,16 @@ main(int    argc,
     }
     else
         x = x0;
+#if 0 // filter syntax errors
+    {
+        xpath_tree *xptree = NULL;
+        if (xpath_parse(xpath, &xptree) < 0)
+            goto ok; // Parse errors returns OK
+    }
+#endif
     if (xpath_vec_ctx(x, nsc, xpath, 0, &xc) < 0)
         return -1;
+
     /* Check inverse, eg XML back to xpath and compare with original, only if nodes */
     if (xpath_inverse && xc->xc_type == XT_NODESET){
         cxobj *xi;