From 87c65c3541c8e20315ade08cd584fd24f550c043 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Mon, 20 Jun 2022 15:08:50 +0200 Subject: [PATCH] Fixed: [RPC edit-config payloads are not fully validated](https://github.com/clicon/clixon/issues/337) --- CHANGELOG.md | 2 + apps/backend/backend_client.c | 21 +++++++-- apps/cli/cli_common.c | 1 + lib/clixon/clixon_validate.h | 1 + lib/src/clixon_validate.c | 14 +++--- test/lib.sh | 2 +- test/test_datastore_format.sh | 2 +- test/test_db.sh | 85 ++++++++++++++++++++++++++++++---- test/test_minmax.sh | 4 +- test/test_nacm_module_read.sh | 6 +-- test/test_unique_descendant.sh | 20 ++------ 11 files changed, 117 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4431b64d..24ae6967 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,8 @@ Developers may need to change their code ### Corrected Bugs +* Fixed: [RPC edit-config payloads are not fully validated](https://github.com/clicon/clixon/issues/337) + ## 5.7.0 17 May 2022 diff --git a/apps/backend/backend_client.c b/apps/backend/backend_client.c index 45dda83a..ba48ba52 100644 --- a/apps/backend/backend_client.c +++ b/apps/backend/backend_client.c @@ -274,7 +274,15 @@ clixon_stats_module_get(clicon_handle h, * @param[in] regarg User argument given at rpc_callback_register() * @retval 0 OK * @retval -1 Error - */ + * @note According to RFC 7950 8.3: constraints MUST be enforced: + * o during parsing of RPC payloads + * o during processing of the operation + * But Clixon is flexible in allowing invalid XML payload here, only some specialized + * validations are made. + * However, what really should be done is to apply the change to the datastore and then + * validate, if error, discard to previous state. + * But this could discard other previous changes to candidate. + */ static int from_client_edit_config(clicon_handle h, cxobj *xn, @@ -383,14 +391,19 @@ from_client_edit_config(clicon_handle h, goto done; goto ok; } + /* Limited validation of incoming payload + */ + if ((ret = xml_yang_check_list_unique_minmax(xc, &xret)) < 0) + goto done; /* xmldb_put (difflist handling) requires list keys */ - if ((ret = xml_yang_validate_list_key_only(xc, &xret)) < 0) + if (ret==1 && (ret = xml_yang_validate_list_key_only(xc, &xret)) < 0) goto done; if (ret == 0){ if (clixon_xml2cbuf(cbret, xret, 0, 0, -1, 0) < 0) goto done; goto ok; } + /* Cant do this earlier since we dont have a yang spec to * the upper part of the tree, until we get the "config" tree. */ @@ -1234,7 +1247,9 @@ from_client_msg(clicon_handle h, clicon_err(OE_XML, errno, "cbuf_new"); goto done; } - /* Decode msg from client -> xml top (ct) and session id */ + /* Decode msg from client -> xml top (ct) and session id + * Bind is a part of the decode function + */ if ((ret = clicon_msg_decode(msg, yspec, &id, &xt, &xret)) < 0){ if (netconf_malformed_message(cbret, "XML parse error") < 0) goto done; diff --git a/apps/cli/cli_common.c b/apps/cli/cli_common.c index 06aa3586..430868cc 100644 --- a/apps/cli/cli_common.c +++ b/apps/cli/cli_common.c @@ -797,6 +797,7 @@ compare_dbs(clicon_handle h, /*! Load a configuration file to candidate database * Utility function used by cligen spec file + * Note that the CLI function makes no Validation of the XML sent to the backend * @param[in] h CLICON handle * @param[in] cvv Vector of variables (where is found) * @param[in] argv A string: " []" diff --git a/lib/clixon/clixon_validate.h b/lib/clixon/clixon_validate.h index 10cc8a5f..aedc378d 100644 --- a/lib/clixon/clixon_validate.h +++ b/lib/clixon/clixon_validate.h @@ -45,6 +45,7 @@ */ int xml_yang_validate_rpc(clicon_handle h, cxobj *xrpc, cxobj **xret); int xml_yang_validate_rpc_reply(clicon_handle h, cxobj *xrpc, cxobj **xret); +int xml_yang_check_list_unique_minmax(cxobj *xt, cxobj **xret); int xml_yang_validate_add(clicon_handle h, cxobj *xt, cxobj **xret); int xml_yang_validate_list_key_only(cxobj *xt, cxobj **xret); int xml_yang_validate_all(clicon_handle h, cxobj *xt, cxobj **xret); diff --git a/lib/src/clixon_validate.c b/lib/src/clixon_validate.c index 01b92ba8..c9db689f 100644 --- a/lib/src/clixon_validate.c +++ b/lib/src/clixon_validate.c @@ -1093,9 +1093,9 @@ check_min_max(cxobj *xp, * XML node. This may not be a large problem since it would mean empty configs * are not allowed. */ -static int -check_list_unique_minmax(cxobj *xt, - cxobj **xret) +int +xml_yang_check_list_unique_minmax(cxobj *xt, + cxobj **xret) { int retval = -1; cxobj *x = NULL; @@ -1458,7 +1458,6 @@ xml_yang_validate_leaf_union(clicon_handle h, * @endcode * @see xml_yang_validate_add * @see xml_yang_validate_rpc - * @note Should need a variant accepting cxobj **xret */ int xml_yang_validate_all(clicon_handle h, @@ -1601,7 +1600,7 @@ xml_yang_validate_all(clicon_handle h, /* Check unique and min-max after choice test for example*/ if (yang_config(yt) != 0){ /* Checks if next level contains any unique list constraints */ - if ((ret = check_list_unique_minmax(xt, xret)) < 0) + if ((ret = xml_yang_check_list_unique_minmax(xt, xret)) < 0) goto done; if (ret == 0) goto fail; @@ -1618,7 +1617,8 @@ xml_yang_validate_all(clicon_handle h, retval = 0; goto done; } -/*! Translate a single xml node to a cligen variable vector. Note not recursive + +/*! Validate a single XML node with yang specification * @param[out] xret Error XML tree (if ret == 0). Free with xml_free after use * @retval 1 Validation OK * @retval 0 Validation failed (xret set) @@ -1637,7 +1637,7 @@ xml_yang_validate_all_top(clicon_handle h, if ((ret = xml_yang_validate_all(h, x, xret)) < 1) return ret; } - if ((ret = check_list_unique_minmax(xt, xret)) < 1) + if ((ret = xml_yang_check_list_unique_minmax(xt, xret)) < 1) return ret; return 1; } diff --git a/test/lib.sh b/test/lib.sh index 2539c766..7014f624 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -779,7 +779,7 @@ EOF r=$(echo "$ret" | grep --null -Go "$i") match=$? if [ $match -ne 0 ]; then - err "$i" "$ret" + err "$expectenc" "$ret" fi done <<< "$expectenc" fi diff --git a/test/test_datastore_format.sh b/test/test_datastore_format.sh index 821b2248..56ace059 100755 --- a/test/test_datastore_format.sh +++ b/test/test_datastore_format.sh @@ -136,7 +136,7 @@ if [ $BE -ne 0 ]; then err fi new "start backend -s init -f $cfg" - start_backend -s startup -f $cfg + start_backend -s init -f $cfg fi new "wait backend" diff --git a/test/test_db.sh b/test/test_db.sh index 0cf9eee7..3abf4a84 100755 --- a/test/test_db.sh +++ b/test/test_db.sh @@ -2,6 +2,7 @@ # Datastore tests: # - XML and JSON # - save and load config files +# Pretty and not # Magic line must be first in script (see README.md) s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi @@ -47,6 +48,11 @@ module clixon-example{ leaf value{ type string; } + container two{ + leaf a{ + type string; + } + } } } } @@ -69,6 +75,7 @@ discard("Discard edits (rollback 0)"), discard_changes(); load("Load configuration from XML file") ("Filename (local filename)"){ xml("Replace candidate with file containing XML"), load_config_file("","filename", "replace", "xml"); json("Replace candidate with file containing JSON"), load_config_file("","filename", "replace", "json"); + merge("Merge file with existent candidate"), load_config_file("filename", "merge"); } save("Save candidate configuration to XML file") ("Filename (local filename)"){ xml("Save configuration as XML"), save_config_file("candidate","filename", "xml"); @@ -181,17 +188,79 @@ EOF new "test params: -f $cfg" -new "test db xml" -testrun xml false +for format in xml json; do + for pretty in false true json; do + new "test db $format pretty=$pretty" + testrun xml false + done +done -new "test db xml pretty" -testrun xml true +# Negative test, load yang-invalid xml +if [ $BE -ne 0 ]; then + new "kill old backend" + sudo clixon_backend -z -f $cfg + if [ $? -ne 0 ]; then + err + fi + new "start backend -s init -f $cfg -o CLICON_XMLDB_FORMAT=$format -o CLICON_XMLDB_PRETTY=$pretty" + start_backend -s init -f $cfg -o CLICON_XMLDB_FORMAT=$format -o CLICON_XMLDB_PRETTY=$pretty +fi -new "test db json" -testrun json false +new "wait backend" +wait_backend -new "test db json pretty" -testrun json true +# Wrong: two toplevels +cat < $dir/myconfig +<${DATASTORE_TOP}> + + + a + 42 + +
+ + + b + 99 + +
+ +EOF + +new "load invalid file: 2 top-level containers, expect fail" +expectpart "$($clixon_cli -1 -f $cfg load $dir/myconfig xml 2>&1)" 0 "Editing configuration: protocol operation-failed : too-many-elements : /rpc/edit-config/config/table" + +# Wrong: two toplevels +cat < $dir/myconfig +<${DATASTORE_TOP}> + + + a + 42 + 1 + 2 + +
+ +EOF + +# XXX This is invalid but not detected at load will be checked with validate +new "load invalid file: 2 inner containers, expect fail" +expectpart "$($clixon_cli -1 -f $cfg load $dir/myconfig xml 2>&1)" 0 "" + +new "Validate expect fail" +expectpart "$($clixon_cli -1 -f $cfg validate 2>&1)" 255 "too-many-elements" + +if [ $BE -ne 0 ]; then + new "Kill backend" + # Check if premature kill + pid=$(pgrep -u root -f clixon_backend) + if [ -z "$pid" ]; then + err "backend already dead" + fi + # kill backend + stop_backend -f $cfg +fi rm -rf $dir diff --git a/test/test_minmax.sh b/test/test_minmax.sh index cf1da8a1..cc84c832 100755 --- a/test/test_minmax.sh +++ b/test/test_minmax.sh @@ -215,10 +215,10 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" " 0 1 2 -" "" "" +" "" "protocoloperation-failedtoo-many-elementserror/rpc/edit-config/config/b3" new "minmax top level too many should fail" -expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "protocoloperation-failedtoo-many-elementserror/b3" +#expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "protocoloperation-failedtoo-many-elementserror/b3" new "netconf discard-changes" expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "" "" "" diff --git a/test/test_nacm_module_read.sh b/test/test_nacm_module_read.sh index e0506efe..334f4a8f 100755 --- a/test/test_nacm_module_read.sh +++ b/test/test_nacm_module_read.sh @@ -65,7 +65,7 @@ module clixon-example{ yang-version 1.1; namespace "urn:example:clixon"; prefix ex; - container table{ + container table{ list parameter{ key name; leaf name{ @@ -167,8 +167,8 @@ RULES=$(cat < 42 - key42val42
- key43val43
+ key42val42 + bkey43val43
EOF ) diff --git a/test/test_unique_descendant.sh b/test/test_unique_descendant.sh index c042bfb6..e7f5fdd4 100755 --- a/test/test_unique_descendant.sh +++ b/test/test_unique_descendant.sh @@ -97,25 +97,13 @@ expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "xafoobfooyafiebfum" -new "Add invalid example" -expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "${rpc}" "" "" - -new "netconf validate same inner (should fail)" -expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "" "" "applicationoperation-faileddata-not-uniqueerrorc/inner/value" - -new "netconf discard-changes" -expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "" "" "" +new "Add invalid example 1" +expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "${rpc}" "" "applicationoperation-faileddata-not-uniqueerrorc/inner/value" rpc="replacexafoobbaryafiebbar" -new "Add invalid example" -expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "${rpc}" "" "" - -new "netconf validate same in different outers (should fail)" -expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "" "" "applicationoperation-faileddata-not-uniqueerrorc/inner/value" - -new "netconf discard-changes" -expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "" "" "" +new "Add invalid example 2" +expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "${rpc}" "" "applicationoperation-faileddata-not-uniqueerrorc/inner/value" if [ $BE -ne 0 ]; then new "Kill backend"