experiments/clixon
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fixed: [RPC edit-config payloads are not fully validated](https://github.com/clicon/clixon/issues/337)

Browse source
This commit is contained in:
Olof hagsand 2022-06-20 15:08:50 +02:00
parent 7c22021242
commit 87c65c3541
11 changed files with 117 additions and 41 deletions
Download patch file Download diff file Expand all files Collapse all files

2
CHANGELOG.md
View file

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

19
apps/backend/backend_client.c
View file

@ -274,6 +274,14 @@ 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 <edit-config> 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,
@ -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;

1
apps/cli/cli_common.c
View file

@ -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 <varname> is found)
* @param[in] argv A string: "<varname> <operation> [<format>]"

1
lib/clixon/clixon_validate.h
View file

@ -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);

12
lib/src/clixon_validate.c
View file

@ -1093,8 +1093,8 @@ 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,
int
xml_yang_check_list_unique_minmax(cxobj *xt,
cxobj **xret)
{
int retval = -1;
@ -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;
}

2
test/lib.sh
View file

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

2
test/test_datastore_format.sh
View file

@ -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"

83
test/test_db.sh
View file

@ -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:string>("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:string>("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"
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 <<EOF > $dir/myconfig
<${DATASTORE_TOP}>
<table xmlns="urn:example:clixon">
<parameter>
<name>a</name>
<value>42</value>
</parameter>
</table>
<table xmlns="urn:example:clixon">
<parameter>
<name>b</name>
<value>99</value>
</parameter>
</table>
</${DATASTORE_TOP}>
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 <<EOF > $dir/myconfig
<${DATASTORE_TOP}>
<table xmlns="urn:example:clixon">
<parameter>
<name>a</name>
<value>42</value>
<two><a>1</a></two>
<two><a>2</a></two>
</parameter>
</table>
</${DATASTORE_TOP}>
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

4
test/test_minmax.sh
View file

@ -215,10 +215,10 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS>
<b3 xmlns=\"urn:example:clixon\">0</b3>
<b3 xmlns=\"urn:example:clixon\">1</b3>
<b3 xmlns=\"urn:example:clixon\">2</b3>
</config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
</config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><rpc-error><error-type>protocol</error-type><error-tag>operation-failed</error-tag><error-app-tag>too-many-elements</error-app-tag><error-severity>error</error-severity><error-path>/rpc/edit-config/config/b3</error-path></rpc-error></rpc-reply>"
new "minmax top level too many should fail"
expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><validate><source><candidate/></source></validate></rpc>" "" "<rpc-reply $DEFAULTNS><rpc-error><error-type>protocol</error-type><error-tag>operation-failed</error-tag><error-app-tag>too-many-elements</error-app-tag><error-severity>error</error-severity><error-path>/b3</error-path></rpc-error></rpc-reply>"
#expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><validate><source><candidate/></source></validate></rpc>" "" "<rpc-reply $DEFAULTNS><rpc-error><error-type>protocol</error-type><error-tag>operation-failed</error-tag><error-app-tag>too-many-elements</error-app-tag><error-severity>error</error-severity><error-path>/b3</error-path></rpc-error></rpc-reply>"
new "netconf discard-changes"
expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><discard-changes/></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"

4
test/test_nacm_module_read.sh
View file

@ -167,8 +167,8 @@ RULES=$(cat <<EOF
</nacm>
<x xmlns="urn:example:nacm">42</x>
<table xmlns="urn:example:clixon"><parameter><name>key42</name><value>val42</value></parameter></table>
<table xmlns="urn:example:clixon"><parameter><name>key43</name><value>val43</value></parameter></table>
<table xmlns="urn:example:clixon"><parameter><name>key42</name><value>val42</value></parameter>
b<parameter><name>key43</name><value>val43</value></parameter></table>
EOF
)

20
test/test_unique_descendant.sh
View file

@ -97,25 +97,13 @@ expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS
rpc="<rpc $DEFAULTNS><edit-config><target><candidate/></target><default-operation>replace</default-operation><config><outer xmlns=\"urn:example:clixon\"><name>x</name><c><inner><name>a</name><value>foo</value></inner><inner><name>b</name><value>foo</value></inner></c></outer><outer xmlns=\"urn:example:clixon\"><name>y</name><c><inner><name>a</name><value>fie</value></inner><inner><name>b</name><value>fum</value></inner></c></outer></config></edit-config></rpc>"
new "Add invalid example"
expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "${rpc}" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
new "netconf validate same inner (should fail)"
expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><validate><source><candidate/></source></validate></rpc>" "" "<rpc-reply $DEFAULTNS><rpc-error><error-type>application</error-type><error-tag>operation-failed</error-tag><error-app-tag>data-not-unique</error-app-tag><error-severity>error</error-severity><error-info><non-unique>c/inner/value</non-unique></error-info></rpc-error></rpc-reply>"
new "netconf discard-changes"
expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><discard-changes/></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
new "Add invalid example 1"
expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "${rpc}" "" "<rpc-reply $DEFAULTNS><rpc-error><error-type>application</error-type><error-tag>operation-failed</error-tag><error-app-tag>data-not-unique</error-app-tag><error-severity>error</error-severity><error-info><non-unique>c/inner/value</non-unique></error-info></rpc-error></rpc-reply>"
rpc="<rpc $DEFAULTNS><edit-config><target><candidate/></target><default-operation>replace</default-operation><config><outer xmlns=\"urn:example:clixon\"><name>x</name><c><inner><name>a</name><value>foo</value></inner><inner><name>b</name><value>bar</value></inner></c></outer><outer xmlns=\"urn:example:clixon\"><name>y</name><c><inner><name>a</name><value>fie</value></inner><inner><name>b</name><value>bar</value></inner></c></outer></config></edit-config></rpc>"
new "Add invalid example"
expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "${rpc}" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
new "netconf validate same in different outers (should fail)"
expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><validate><source><candidate/></source></validate></rpc>" "" "<rpc-reply $DEFAULTNS><rpc-error><error-type>application</error-type><error-tag>operation-failed</error-tag><error-app-tag>data-not-unique</error-app-tag><error-severity>error</error-severity><error-info><non-unique>c/inner/value</non-unique></error-info></rpc-error></rpc-reply>"
new "netconf discard-changes"
expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><discard-changes/></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
new "Add invalid example 2"
expecteof_netconf "$clixon_netconf -qef $cfg" 0 "$DEFAULTHELLO" "${rpc}" "" "<rpc-reply $DEFAULTNS><rpc-error><error-type>application</error-type><error-tag>operation-failed</error-tag><error-app-tag>data-not-unique</error-app-tag><error-severity>error</error-severity><error-info><non-unique>c/inner/value</non-unique></error-info></rpc-error></rpc-reply>"
if [ $BE -ne 0 ]; then
new "Kill backend"