From c405a08ff84de802f17c348035accfcb9d69ad23 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Fri, 28 May 2021 15:42:15 +0200 Subject: [PATCH] Restconf https fixes after nmap and fuzz: - segv in PUT/POST of / - Dont return bad request on error, just close socket --- apps/restconf/restconf_main_native.c | 24 +++- apps/restconf/restconf_methods.c | 2 +- test/test_restconf.sh | 13 +- test/test_restconf_internal.sh | 2 + test/test_restconf_nmap.sh | 201 +++++++++++++++++++++++++++ 5 files changed, 233 insertions(+), 9 deletions(-) create mode 100755 test/test_restconf_nmap.sh diff --git a/apps/restconf/restconf_main_native.c b/apps/restconf/restconf_main_native.c index bb1872b1..49f2d05c 100644 --- a/apps/restconf/restconf_main_native.c +++ b/apps/restconf/restconf_main_native.c @@ -799,6 +799,7 @@ alpn_select_proto_cb(SSL *ssl, unsigned char *inp; unsigned char len; + clicon_debug(1, "%s", __FUNCTION__); if (clicon_debug_get()) dump_alpn_proto_list(in, inlen); /* select http/1.1 */ @@ -839,7 +840,7 @@ restconf_ssl_context_create(clicon_handle h) * The question is which TLS to disable * SSL_OP_NO_TLSv1, SSL_OP_NO_TLSv1_1, SSL_OP_NO_TLSv1_2 */ - SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv3 | SSL_OP_NO_TLSv1); + SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv3 | SSL_OP_NO_TLSv1 | SSL_OP_NO_TLSv1_1); SSL_CTX_set_options(ctx, SSL_MODE_RELEASE_BUFFERS | SSL_OP_NO_COMPRESSION); // SSL_CTX_set_timeout(ctx, cfg->ssl_ctx_timeout); /* default 300s */ @@ -1307,9 +1308,14 @@ restconf_accept_client(int fd, switch (e){ case SSL_ERROR_SSL: /* 1 */ clicon_debug(1, "%s SSL_ERROR_SSL (non-ssl message on ssl socket)", __FUNCTION__); +#if 0 + /* XXX sending a bad request here may crash + * eg when run nmap --script ssl* + */ if (send_badrequest(h, s, NULL, "application/yang-data+xml", "protocolmalformed-messageThe plain HTTP request was sent to HTTPS port") < 0) goto done; +#endif SSL_free(ssl); if (close(s) < 0){ clicon_err(OE_UNIX, errno, "close"); @@ -1394,8 +1400,20 @@ restconf_accept_client(int fd, } clicon_debug(1, "%s ALPN: %d %s", __FUNCTION__, alpnlen, alpn); if (alpn == NULL || alpnlen != 8 || memcmp("http/1.1", alpn, 8) != 0) { - clicon_err(OE_RESTCONF, 0, "Protocol http/1.1 not selected: %s", alpn); - goto done; + clicon_log(LOG_NOTICE, "------Warning1 Protocol http/1.1 not selected: %s", alpn); + if (send_badrequest(h, s, ssl, "application/yang-data+xml", "protocolmalformed-messagePeer certificate required") < 0) + goto done; + restconf_conn_free(conn); + evhtp_connection_free(conn); /* evhtp */ + if (ssl){ + if ((ret = SSL_shutdown(ssl)) < 0){ + int e = SSL_get_error(ssl, ret); + clicon_err(OE_SSL, 0, "SSL_shutdown, err:%d", e); + goto done; + } + SSL_free(ssl); + } + goto ok; } /* Get the actual peer, XXX this maybe could be done in ca-auth client-cert code ? * Note this _only_ works if SSL_set1_host() was set previously,... diff --git a/apps/restconf/restconf_methods.c b/apps/restconf/restconf_methods.c index a4c1f51a..0bfdf689 100644 --- a/apps/restconf/restconf_methods.c +++ b/apps/restconf/restconf_methods.c @@ -287,7 +287,7 @@ api_data_write(clicon_handle h, cxobj *xfrom; cxobj *xac; - xfrom = api_path?xml_parent(xbot):xbot; + xfrom = (api_path && strcmp(api_path,"/"))?xml_parent(xbot):xbot; // XXX xbot is /config has NULL parent if (xml_copy_one(xfrom, xdata0) < 0) goto done; xa = NULL; diff --git a/test/test_restconf.sh b/test/test_restconf.sh index b5e0d3f7..9e59a8af 100755 --- a/test/test_restconf.sh +++ b/test/test_restconf.sh @@ -158,6 +158,7 @@ function testrun() new "start restconf daemon" # inline of start_restconf, cant make quotes to work + echo "sudo -u $wwwstartuser -s $clixon_restconf $RCLOG -D $DBG -f $cfg -R " sudo -u $wwwstartuser -s $clixon_restconf $RCLOG -D $DBG -f $cfg -R "$RESTCONFIG1" & if [ $? -ne 0 ]; then err1 "expected 0" "$?" @@ -170,10 +171,10 @@ function testrun() new "restconf root discovery. RFC 8040 3.1 (xml+xrd)" expectpart "$(curl $CURLOPTS -X GET $proto://$addr/.well-known/host-meta)" 0 'HTTP/1.1 200 OK' "" "" "" -if [ "${WITH_RESTCONF}" = "native" ]; then # XXX does not work with nginx - new "restconf GET http/1.0 - returns 1.0" - expectpart "$(curl $CURLOPTS --http1.0 -X GET $proto://$addr/.well-known/host-meta)" 0 'HTTP/1.0 200 OK' "" "" "" -fi + if [ "${WITH_RESTCONF}" = "native" ]; then # XXX does not work with nginx + new "restconf GET http/1.0 - returns 1.0" + expectpart "$(curl $CURLOPTS --http1.0 -X GET $proto://$addr/.well-known/host-meta)" 0 'HTTP/1.0 200 OK' "" "" "" + fi new "restconf GET http/1.1" expectpart "$(curl $CURLOPTS --http1.1 -X GET $proto://$addr/.well-known/host-meta)" 0 'HTTP/1.1 200 OK' "" "" "" @@ -194,7 +195,9 @@ fi expectpart "$(curl $CURLOPTS -X GET https://$addr:80/.well-known/host-meta 2>&1)" 35 #"wrong version number" # dependent on curl version else # see (1) http to https port in restconf_main_native.c new "Wrong proto=http on https port, expect bad request" - expectpart "$(curl $CURLOPTS -X GET http://$addr:443/.well-known/host-meta)" 0 "HTTP/1.1 400 Bad Request" + # When run nmap --script ssl* I can crash restconf if try to return a bad request here + # expectpart "$(curl $CURLOPTS -X GET http://$addr:443/.well-known/host-meta)" 0 "HTTP/1.1 400 Bad Request" + expectpart "$(curl $CURLOPTS -X GET http://$addr:443/.well-known/host-meta 2>&1)" 56 "Connection reset by peer" fi # Exact match diff --git a/test/test_restconf_internal.sh b/test/test_restconf_internal.sh index 41bb05f8..41e63bf4 100755 --- a/test/test_restconf_internal.sh +++ b/test/test_restconf_internal.sh @@ -422,6 +422,8 @@ wait_restconf new "Edit a restconf field via restconf" # XXX fcgi fails here expectpart "$(curl $CURLOPTS -X PUT -H "Content-Type: application/yang-data+json" $RCPROTO://localhost/restconf/data/clixon-restconf:restconf/pretty -d '{"clixon-restconf:pretty":true}' )" 0 "HTTP/1.1 204 No Content" +sleep $DEMSLEEP + new "check status RPC new pid" rpcstatus true running pid2=$pid diff --git a/test/test_restconf_nmap.sh b/test/test_restconf_nmap.sh new file mode 100755 index 00000000..9337fa23 --- /dev/null +++ b/test/test_restconf_nmap.sh @@ -0,0 +1,201 @@ +#!/usr/bin/env bash +# Restconf basic functionality also uri encoding using eth/0/0 +# Note there are many variants: (1)fcgi/native, (2) http/https, (3) IPv4/IPv6, (4)local or backend-config +# (1) fcgi/native +# This is compile-time --with-restconf=fcgi or native, so either or +# - fcgi: Assume http server setup, such as nginx described in apps/restconf/README.md +# - native: test both local config and get config from backend +# (2) http/https +# - fcgi: relies on nginx has https setup +# - native: generate self-signed server certs +# (3) IPv4/IPv6 (only loopback 127.0.0.1 / ::1) +# - The tests runs through both +# - IPv6 by default disabled since docker does not support it out-of-the box +# (4) local/backend config. Native only +# - The tests runs through both (if compiled with native) +# See also test_restconf_op.sh +# See test_restconf_rpc.sh for cases when CLICON_BACKEND_RESTCONF_PROCESS is set + +# Magic line must be first in script (see README.md) +s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi + +# Only works with native and https +if [ "${WITH_RESTCONF}" != "native" ]; then + if [ "$s" = $0 ]; then exit 0; else return 0; fi # skip +fi + +RCPROTO=https +APPNAME=example + +cfg=$dir/conf.xml + +# If nmap not installed just quietly quit +if [ ! -n "$(type nmap 2> /dev/null)" ]; then + if [ "$s" = $0 ]; then exit 0; else return 0; fi # skip +fi + +# clixon-example and clixon-restconf is used in the test, need local copy +# This is a kludge: look in src otherwise assume it is installed in /usr/local/share +# Note that revisions may change and may need to be updated +y="clixon-example@${CLIXON_EXAMPLE_REV}.yang" + +if [ -d ${TOP_SRCDIR}/example/main/$y ]; then + cp ${TOP_SRCDIR}/example/main/$y $dir/ +else + cp /usr/local/share/clixon/$y $dir/ +fi +y=clixon-restconf@${CLIXON_RESTCONF_REV}.yang +if [ -d ${TOP_SRCDIR}/yang/clixon ]; then + cp ${TOP_SRCDIR}/yang/clixon/$y $dir/ +else + cp /usr/local/share/clixon/$y $dir/ +fi + +if [ "${WITH_RESTCONF}" = "native" ]; then + # Create server certs + certdir=$dir/certs + srvkey=$certdir/srv_key.pem + srvcert=$certdir/srv_cert.pem + cakey=$certdir/ca_key.pem # needed? + cacert=$certdir/ca_cert.pem + test -d $certdir || mkdir $certdir + # Create server certs and CA + cacerts $cakey $cacert + servercerts $cakey $cacert $srvkey $srvcert + USEBACKEND=true +else + # Define default restconfig config: RESTCONFIG + RESTCONFIG=$(restconf_config none false) + USEBACKEND=false +fi + +# This is a fixed 'state' implemented in routing_backend. It is assumed to be always there +state='{"clixon-example:state":{"op":\["41","42","43"\]}' + +if $IPv6; then + # For backend config, create 4 sockets, all combinations IPv4/IPv6 + http/https + RESTCONFIG1=$(cat < + true + none + $srvcert + $srvkey + $cakey + false + default
0.0.0.0
80false + default
0.0.0.0
443true + default
::
80false + default
::
443true + +EOF +) +else + # For backend config, create 2 sockets, all combinations IPv4 + http/https + RESTCONFIG1=$(cat < + true + none + $srvcert + $srvkey + $cakey + false + default
0.0.0.0
80false + default
0.0.0.0
443true + +EOF +) +fi + +# Clixon config +cat < $cfg + + $cfg + clixon-restconf:allow-auth-none + /usr/local/share/clixon + $IETFRFC + $dir + /usr/local/lib/$APPNAME/clispec + /usr/local/lib/$APPNAME/backend + example_backend.so$ + /usr/local/lib/$APPNAME/restconf + /usr/local/lib/$APPNAME/cli + $APPNAME + /usr/local/var/$APPNAME/$APPNAME.sock + /usr/local/var/$APPNAME/$APPNAME.pidfile + /usr/local/var/$APPNAME + true + $USEBACKEND + $RESTCONFIG + +EOF + +new "test params: -f $cfg -- -s" +if [ $BE -ne 0 ]; then + new "kill old backend" + sudo clixon_backend -zf $cfg + if [ $? -ne 0 ]; then + err + fi + sudo pkill -f clixon_backend # to be sure + + new "start backend -s init -f $cfg -- -s" + start_backend -s init -f $cfg -- -s +fi + +new "wait backend" +wait_backend + +new "netconf edit config" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO$RESTCONFIG1]]>]]>" "^]]>]]>$" + +new "netconf commit" +expecteof "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO]]>]]>" "^]]>]]>$" + +if [ $RC -ne 0 ]; then + new "kill old restconf daemon" + stop_restconf_pre + + new "start restconf daemon" + # inline of start_restconf, cant make quotes to work + echo "sudo -u $wwwstartuser -s $clixon_restconf $RCLOG -D $DBG -f $cfg -R " + sudo -u $wwwstartuser -s $clixon_restconf $RCLOG -D $DBG -f $cfg -R "$RESTCONFIG1" & + if [ $? -ne 0 ]; then + err1 "expected 0" "$?" + fi +fi + +new "wait restconf" +wait_restconf + +sleep 1 # Sometimes nmap test fails with no reply from server, _maybe_ this helps? +new "nmap test" + +expectpart "$(nmap --script ssl* -p 443 127.0.0.1)" 0 "443/tcp open https" "least strength: A" "Nmap done: 1 IP address (1 host up) scanned in" --not-- "No reply from server" "TLSv1.0:" "TLSv1.1:" + +if [ $RC -ne 0 ]; then + new "Kill restconf daemon" + stop_restconf +fi + +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 + +# unset conditional parameters +unset RCPROTO + +# Set by restconf_config +unset RESTCONFIG +unset RESTCONFIG1 + +rm -rf $dir + +new "endtest" +endtest