Skip to content

Commit

Permalink
WIP: Fix problems with early errors
Browse files Browse the repository at this point in the history 
This PR addresses the issues raised in drwetter#1397. Rather than just deal with the Bash error messages that occur if there is a problem with the command line and either $do_json or $do_pretty_json is set to true, this PR addresses the larger problem.

html_header() sets the name for the HTML file and ensures that it is okay to write to that file. So, nothing should be written to $HTMLFILE until html_header() has been called and has checked that $HTMLFILE may be written to. So, this PR defines, $HTMLFILENAME_SET which is false until html_header() successfully completes. Any place that wants to write to $HTMLFILE then checks that $HTMLFILENAME_SET is true before performing the write.

The same issues apply for the $CSVFILE, $JSONFILE, and $LOGFILE. So, similar changes are made for each of these files.

I believe that this PR fixes the issues raised in drwetter#1397. However, given the potential impact of the changes this PR makes, it needs to be tested very thoroughly before it is merged. So, I marked it as "WIP" until I've had a chance to perform more testing.
  • Loading branch information
@dcooper16
dcooper16 committed Nov 27, 2024
1 parent 701c606 commit 808358a
Showing 1 changed file with 33 additions and 23 deletions.
56 changes: 33 additions & 23 deletions testssl.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,10 @@ FIRST_FINDING=true # is this the first finding we are outpu
JSONHEADER=true # include JSON headers and footers in HTML file, if one is being created
CSVHEADER=true # same for CSV
HTMLHEADER=true # same for HTML
JSONFILENAME_SET=false # Indicates whether json_header() has been called to set up JSONFILE.
CSVFILENAME_SET=false # Indicates whether csv_header() has been called to set up CSVFILE.
HTMLFILENAME_SET=false # Indicates whether html_header() has been called to set up HTMLFILE.
LOGFILENAME_SET=false # Indicates whether prepare_logging() has been called to set up LOGFILE.
SECTION_FOOTER_NEEDED=false # kludge for tracking whether we need to close the JSON section object
GIVE_HINTS=false # give an additional info to findings
SERVER_SIZE_LIMIT_BUG=false # Some servers have either a ClientHello total size limit or a 128 cipher limit (e.g. old ASAs)
Expand Down Expand Up @@ -555,7 +559,7 @@ html_out() {
local outstr="$1"

"$do_html" || return 0
if [[ -n "$HTMLFILE" ]] && [[ ! -d "$HTMLFILE" ]]; then
if "$HTMLFILENAME_SET" && [[ -n "$HTMLFILE" ]] && [[ ! -d "$HTMLFILE" ]]; then
if [[ "$outstr" =~ [[:cntrl:]] ]]; then
outstr="$(sanitize_fileout "$outstr")"
fi
Expand Down Expand Up @@ -1199,7 +1203,7 @@ set_ciph_str_score() {
#################### START JSON file functions ####################

fileout_json_footer() {
if "$do_json"; then
if "$do_json" && "$JSONFILENAME_SET"; then
if [[ "$SCAN_TIME" -eq 0 ]]; then
fileout_json_finding "scanTime" "WARN" "Scan interrupted" "" "" ""
elif [[ $SEVERITY_LEVEL -lt $LOW ]] ; then
Expand All @@ -1208,7 +1212,7 @@ fileout_json_footer() {
fi
printf "]\n" >> "$JSONFILE"
fi
if "$do_pretty_json"; then
if "$do_pretty_json" && "$JSONFILENAME_SET"; then
if [[ "$SCAN_TIME" -eq 0 ]]; then
echo -e " ],\n \"scanTime\" : \"Scan interrupted\"\n}" >> "$JSONFILE"
else
Expand Down Expand Up @@ -1239,14 +1243,14 @@ fileout_json_section() {
fileout_section_header() {
local str=""
"$2" && str="$(fileout_section_footer false)"
"$do_pretty_json" && FIRST_FINDING=true && (printf "%s%s\n" "$str" "$(fileout_json_section "$1")") >> "$JSONFILE"
"$do_pretty_json" && "$JSONFILENAME_SET" && FIRST_FINDING=true && (printf "%s%s\n" "$str" "$(fileout_json_section "$1")") >> "$JSONFILE"
SECTION_FOOTER_NEEDED=true
}

# arg1: whether to end object too
fileout_section_footer() {
"$do_pretty_json" && FIRST_FINDING=false && printf "\n ]" >> "$JSONFILE"
"$do_pretty_json" && "$1" && echo -e "\n }" >> "$JSONFILE"
"$do_pretty_json" && "$JSONFILENAME_SET" && FIRST_FINDING=false && printf "\n ]" >> "$JSONFILE"
"$do_pretty_json" && "$JSONFILENAME_SET" && "$1" && echo -e "\n }" >> "$JSONFILE"
SECTION_FOOTER_NEEDED=false
}

Expand Down Expand Up @@ -1276,7 +1280,7 @@ fileout_json_finding() {
local cwe="$5"
local hint="$6"

if "$do_json"; then
if "$do_json" && "$JSONFILENAME_SET"; then
"$FIRST_FINDING" || echo -n "," >> "$JSONFILE"
echo -e " {" >> "$JSONFILE"
fileout_json_print_parameter "id" " " "$1" true
Expand All @@ -1289,7 +1293,7 @@ fileout_json_finding() {
fileout_json_print_parameter "finding" " " "$finding" false
echo -e "\n }" >> "$JSONFILE"
fi
if "$do_pretty_json"; then
if "$do_pretty_json" && "$JSONFILENAME_SET"; then
if [[ "$1" == service ]]; then
if [[ $SERVER_COUNTER -gt 1 ]]; then
echo " ," >> "$JSONFILE"
Expand Down Expand Up @@ -1340,14 +1344,14 @@ fileout_pretty_json_banner() {
fileout_banner() {
if "$JSONHEADER"; then
# "$do_json" && # here we maybe should add a banner, too
"$do_pretty_json" && FIRST_FINDING=true && (printf "%s\n" "$(fileout_pretty_json_banner)") >> "$JSONFILE"
"$do_pretty_json" && "$JSONFILENAME_SET" && FIRST_FINDING=true && (printf "%s\n" "$(fileout_pretty_json_banner)") >> "$JSONFILE"
fi
}

fileout_separator() {
if "$JSONHEADER"; then
"$do_pretty_json" && echo " ," >> "$JSONFILE"
"$do_json" && echo -n "," >> "$JSONFILE"
"$do_pretty_json" && "$JSONFILENAME_SET" && echo " ," >> "$JSONFILE"
"$do_json" && "$JSONFILENAME_SET" && echo -n "," >> "$JSONFILE"
fi
}

Expand Down Expand Up @@ -1375,13 +1379,13 @@ fileout_insert_warning() {
# is called before fileout_banner(). The only instance in which this function is called
# after fileout_banner() is in the case of a TLS 1.3 only server when $OPENSSL does not
# support TLS 1.3.
if "$do_pretty_json" && "$JSONHEADER" && ! "$TLS13_ONLY"; then
if "$do_pretty_json" && "$JSONFILENAME_SET" && "$JSONHEADER" && ! "$TLS13_ONLY"; then
echo -e " \"clientProblem${CLIENT_PROB_NO}\" : [" >>"$JSONFILE"
CLIENT_PROB_NO=$((CLIENT_PROB_NO + 1))
FIRST_FINDING=true # make sure we don't have a comma here
fi
fileout "$1" "$2" "$3"
if "$do_pretty_json" && ! "$TLS13_ONLY"; then
if "$do_pretty_json" && "$JSONFILENAME_SET" && ! "$TLS13_ONLY"; then
if "$JSONHEADER"; then
echo -e "\n ]," >>"$JSONFILE"
else
Expand Down Expand Up @@ -1423,8 +1427,8 @@ fileout() {

if { "$do_pretty_json" && [[ "$1" == service ]]; } || show_finding "$severity"; then
local finding=$(strip_lf "$(newline_to_spaces "$(strip_quote "$3")")") # additional quotes will mess up screen output
[[ -e "$JSONFILE" ]] && [[ ! -d "$JSONFILE" ]] && fileout_json_finding "$1" "$severity" "$finding" "$cve" "$cwe" "$hint"
"$do_csv" && [[ -n "$CSVFILE" ]] && [[ ! -d "$CSVFILE" ]] && \
"$JSONFILENAME_SET" && [[ -e "$JSONFILE" ]] && [[ ! -d "$JSONFILE" ]] && fileout_json_finding "$1" "$severity" "$finding" "$cve" "$cwe" "$hint"
"$do_csv" && "$CSVFILENAME_SET" && [[ -n "$CSVFILE" ]] && [[ ! -d "$CSVFILE" ]] && \
fileout_csv_finding "$1" "$NODE/$NODEIP" "$PORT" "$severity" "$finding" "$cve" "$cwe" "$hint"
"$FIRST_FINDING" && FIRST_FINDING=false
fi
Expand All @@ -1446,7 +1450,7 @@ json_header() {
# * this is an individual test within a mass test and all output is being placed in a single file.
! "$do_json" && ! "$do_pretty_json" && JSONHEADER=false && return 0
"$do_mass_testing" && ! "$filename_provided" && JSONHEADER=false && return 0
"$CHILD_MASS_TESTING" && "$filename_provided" && [[ -n "$PARENT_JSONFILE" ]] && JSONHEADER=false && return 0
"$CHILD_MASS_TESTING" && "$filename_provided" && [[ -n "$PARENT_JSONFILE" ]] && JSONHEADER=false && JSONFILENAME_SET=true && return 0

if "$do_display_only"; then
fname_prefix="local-ciphers"
Expand Down Expand Up @@ -1478,6 +1482,7 @@ json_header() {
"$do_json" && echo "[" > "$JSONFILE"
"$do_pretty_json" && echo "{" > "$JSONFILE"
fi
JSONFILENAME_SET=true
return 0
}

Expand All @@ -1494,7 +1499,7 @@ csv_header() {
# CSV similar to JSON
! "$do_csv" && CSVHEADER=false && return 0
"$do_mass_testing" && ! "$filename_provided" && CSVHEADER=false && return 0
"$CHILD_MASS_TESTING" && "$filename_provided" && [[ -n "$PARENT_CSVFILE" ]] && CSVHEADER=false && return 0
"$CHILD_MASS_TESTING" && "$filename_provided" && [[ -n "$PARENT_CSVFILE" ]] && CSVHEADER=false && CSVFILENAME_SET=true && return 0

if "$do_display_only"; then
fname_prefix="local-ciphers"
Expand All @@ -1518,11 +1523,13 @@ csv_header() {
fi
if "$APPEND"; then
CSVHEADER=false
CSVFILENAME_SET=true
else
if [[ -s "$CSVFILE" ]]; then
"$OVERWRITE" || fatal_cmd_line "non-empty \"$CSVFILE\" exists. Either use \"--append\" or (re)move it" $ERR_FCREATE
cp /dev/null "$CSVFILE"
fi
CSVFILENAME_SET=true
touch "$CSVFILE"
if "$GIVE_HINTS"; then
fileout_csv_finding "id" "fqdn/ip" "port" "severity" "finding" "cve" "cwe" "hint"
Expand Down Expand Up @@ -1551,7 +1558,7 @@ html_header() {
# * this is an individual test within a mass test and all HTML output is being placed in a single file.
! "$do_html" && HTMLHEADER=false && return 0
"$do_mass_testing" && ! "$filename_provided" && HTMLHEADER=false && return 0
"$CHILD_MASS_TESTING" && "$filename_provided" && [[ -n "$PARENT_HTMLFILE" ]] && HTMLHEADER=false && return 0
"$CHILD_MASS_TESTING" && "$filename_provided" && [[ -n "$PARENT_HTMLFILE" ]] && HTMLHEADER=false && HTMLFILENAME_SET=true && return 0

if "$do_display_only"; then
fname_prefix="local-ciphers"
Expand All @@ -1575,11 +1582,13 @@ html_header() {
fi
if "$APPEND"; then
HTMLHEADER=false
HTMLFILENAME_SET=true
else
if [[ -s "$HTMLFILE" ]]; then
"$OVERWRITE" || fatal_cmd_line "non-empty \"$HTMLFILE\" exists. Either use \"--append\" or (re)move it" $ERR_FCREATE
cp /dev/null "$HTMLFILE"
fi
HTMLFILENAME_SET=true
html_out "<?xml version=\"1.0\" encoding=\"UTF-8\" ?>\n"
html_out "<!DOCTYPE html PUBLIC \"-//W3C//DTD XHTML 1.0 Strict//EN\" \"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd\">\n"
html_out "<!-- This file was created with testssl.sh. https://testssl.sh -->\n"
Expand Down Expand Up @@ -1628,7 +1637,7 @@ prepare_logging() {
# Similar to html_header():
! "$do_logging" && return 0
"$do_mass_testing" && ! "$filename_provided" && return 0
"$CHILD_MASS_TESTING" && "$filename_provided" && [[ -n "$PARENT_LOGFILE" ]] && return 0
"$CHILD_MASS_TESTING" && "$filename_provided" && [[ -n "$PARENT_LOGFILE" ]] && LOGFILENAME_SET=true && return 0

[[ -z "$fname_prefix" ]] && fname_prefix="${FNAME_PREFIX}${NODE}_p${PORT}"

Expand All @@ -1647,6 +1656,7 @@ prepare_logging() {
cp /dev/null "$LOGFILE"
fi
fi
LOGFILENAME_SET=true
tmln_out "## Scan started as: \"$PROG_NAME $CMDLINE\"" >>"$LOGFILE"
tmln_out "## at $HNAME:$OPENSSL_LOCATION" >>"$LOGFILE"
tmln_out "## version testssl: $VERSION ${GIT_REL_SHORT} from $REL_DATE" >>"$LOGFILE"
Expand Down Expand Up @@ -21008,10 +21018,10 @@ child_error() {
fatal() {
outln
prln_magenta "Fatal error: $1" >&2
[[ -n "$LOGFILE" ]] && prln_magenta "Fatal error: $1" >>$LOGFILE
"$LOGFILENAME_SET" && [[ -n "$LOGFILE" ]] && prln_magenta "Fatal error: $1" >>$LOGFILE
if [[ -n "$3" ]]; then
outln "$3" >&2
[[ -n "$LOGFILE" ]] && outln "$3" >>$LOGFILE
"$LOGFILENAME_SET" && [[ -n "$LOGFILE" ]] && outln "$3" >>$LOGFILE
fi
# Make sure we don't try to write into files when not created yet.
# No shorthand expression to avoid errors when $CMDLINE_PARSED haven't been filled yet.
Expand Down Expand Up @@ -21048,7 +21058,7 @@ fatal_cmd_line() {
ip_fatal() {
outln
prln_magenta "Fatal error: $1, proceeding with next IP (if any)" >&2
[[ -n "$LOGFILE" ]] && prln_magenta "Fatal error: $1, proceeding with next IP (if any)" >>$LOGFILE
"$LOGFILENAME_SET" && [[ -n "$LOGFILE" ]] && prln_magenta "Fatal error: $1, proceeding with next IP (if any)" >>$LOGFILE
outln
fileout "scanProblem" "FATAL" "$1, proceeding with next IP (if any)"
return 0
Expand All @@ -21060,7 +21070,7 @@ ip_fatal() {
generic_nonfatal() {
prln_magenta "$1" >&2
[[ -n $2 ]] && outln "$2"
[[ -n "$LOGFILE" ]] && prln_magenta "$1" >>$LOGFILE && [[ -n $2 ]] && outln "$2" >>$LOGFILE
"$LOGFILENAME_SET" && [[ -n "$LOGFILE" ]] && prln_magenta "$1" >>$LOGFILE && [[ -n $2 ]] && outln "$2" >>$LOGFILE
outln
fileout "scanProblem" "WARN" "$1"
return 0
Expand Down

0 comments on commit 808358a

Please sign in to comment.