diff --git a/CHANGELOG.md b/CHANGELOG.md index b99fb1a5..990e3b68 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/README.md b/README.md index 7fab0f3b..333ed83d 100644 --- a/README.md +++ b/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). \ No newline at end of file diff --git a/apps/netconf/netconf_main.c b/apps/netconf/netconf_main.c index 7703ba3b..f18bfd3d 100644 --- a/apps/netconf/netconf_main.c +++ b/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 - if ((frame_size = clicon_option_int(h, NETCONF_FRAME_SIZE)) < 0) + else{ + 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", diff --git a/apps/restconf/clixon_http_data.c b/apps/restconf/clixon_http_data.c index ccfc85cc..4977ebca 100644 --- a/apps/restconf/clixon_http_data.c +++ b/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? */ diff --git a/lib/src/clixon_xpath_eval.c b/lib/src/clixon_xpath_eval.c index 22067666..59b9c308 100644 --- a/lib/src/clixon_xpath_eval.c +++ b/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 */ - /* Loop over each node in the nodeset */ - assert (xr0->xc_type == XT_NODESET); + // alt set nodeset to NULL + if (xs->xs_c1 && xr0->xc_type == XT_NODESET){ /* Second child */ + /* 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; ixc_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; jxc_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; ixc_size; i++){ - x = xc1->xc_nodeset[i]; /* node in nodeset */ - s1 = xml_body(x); + /* node in nodeset */ + 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; ixc_size; i++){ - x = xc1->xc_nodeset[i]; /* node in nodeset */ - if (sscanf(xml_body(x), "%lf", &n1) != 1) + /* node in nodeset */ + 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){ diff --git a/lib/src/clixon_xpath_parse.y b/lib/src/clixon_xpath_parse.y index 321b199f..a17a86c3 100644 --- a/lib/src/clixon_xpath_parse.y +++ b/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: diff --git a/lib/src/clixon_yang_cardinality.c b/lib/src/clixon_yang_cardinality.c index 5175efa3..a37f7600 100644 --- a/lib/src/clixon_yang_cardinality.c +++ b/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 diff --git a/test/config.sh.in b/test/config.sh.in index 0d378b35..9f8561c6 100755 --- a/test/config.sh.in +++ b/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) diff --git a/test/fuzz/xpath/README.md b/test/fuzz/xpath/README.md new file mode 100644 index 00000000..4ef03ba0 --- /dev/null +++ b/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. diff --git a/test/fuzz/xpath/input/1.xpath b/test/fuzz/xpath/input/1.xpath new file mode 100644 index 00000000..9f749ab8 --- /dev/null +++ b/test/fuzz/xpath/input/1.xpath @@ -0,0 +1 @@ +/ex:table[ex:parameter='x'] diff --git a/test/fuzz/xpath/runfuzz.sh b/test/fuzz/xpath/runfuzz.sh new file mode 100755 index 00000000..6c36a6fc --- /dev/null +++ b/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 < $xml + + + x + 42 + +
+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 diff --git a/test/test_autocli_extension.sh b/test/test_autocli_extension.sh index 805636f5..c92b9ded 100755 --- a/test/test_autocli_extension.sh +++ b/test/test_autocli_extension.sh @@ -133,10 +133,6 @@ EOF new "check datastore using netconf" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "$XML" - # Test not right context but could not find other test where it fits - new "negative test" - expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "" - new "check datastore direct access" expectpart "$($clixon_util_datastore -d candidate -b $dir -y $fyang -Y ${YANG_INSTALLDIR} -Y $dir get /)" 0 "$XML" diff --git a/test/test_netconf.sh b/test/test_netconf.sh index bb57fd3f..660861dc 100755 --- a/test/test_netconf.sh +++ b/test/test_netconf.sh @@ -431,9 +431,17 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" " new "netconf xpath syntax error (api-path not xpath) should fail" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "applicationoperation-failederrorxpath parser on line 1: syntax error at or before: ','" -new "netconf wrong xpath should fail" +new "netconf xpath syntax error" +rpc="" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "$rpc" "" "applicationoperation-failederrorxpath parser on line 1: syntax error at or before: ']'" + +new "netconf not found xpath should fail" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "" +new "netconf xpath mixed types" +rpc="@er='x']\" xmlns:if=\"urn:ietf:params:xml:ns:yang:ietf-interfaces\"/>" +expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "$rpc" "" "applicationoperation-failederrorGet candidate datastore: Mixed types not supported, 1 3" + if [ $BE -ne 0 ]; then new "Kill backend" # Check if premature kill diff --git a/test/test_xpath.sh b/test/test_xpath.sh index efc2b129..9d1dbbc1 100755 --- a/test/test_xpath.sh +++ b/test/test_xpath.sh @@ -120,7 +120,6 @@ cat < $xmlfn 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:99$" +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 "73" +# Negative tests from fuzz crashes +cat < $dir/1.xml + + + x + 42 + +
+EOF + +cat < $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 < $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 < $dir/1.xpath +/ex:table $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 < $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 < $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 < $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 < $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 diff --git a/util/Makefile.in b/util/Makefile.in index 9b21b6bb..c60eaf38 100644 --- a/util/Makefile.in +++ b/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) - $(CC) $(INCLUDES) $(CPPFLAGS) @CFLAGS@ $(LDFLAGS) $^ -l clixon_backend -o $@ $(LIBS) +clixon_util_validate: clixon_util_validate.c $(LIBDEPS) $(BELIBDEPS) + $(CC) $(INCLUDES) $(CPPFLAGS) @CFLAGS@ $(LDFLAGS) $^ -l clixon_backend -o $@ $(LIBS) $(BELIBS) -clixon_util_dispatcher: clixon_util_dispatcher.c $(LIBDEPS) - $(CC) $(INCLUDES) $(CPPFLAGS) @CFLAGS@ $(LDFLAGS) $^ -l clixon_backend -o $@ $(LIBS) +clixon_util_dispatcher: clixon_util_dispatcher.c $(LIBDEPS) $(BELIBDEPS) + $(CC) $(INCLUDES) $(CPPFLAGS) @CFLAGS@ $(LDFLAGS) $^ -l clixon_backend -o $@ $(LIBS) $(BELIBS) ifdef with_restconf clixon_util_stream: clixon_util_stream.c $(LIBDEPS) diff --git a/util/clixon_util_xpath.c b/util/clixon_util_xpath.c index 8b9a9c22..0be82b2d 100644 --- a/util/clixon_util_xpath.c +++ b/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;