4
\$\begingroup\$

I have been tinkering with a script that should list things that are listening on the network, but are not part of the base packaging manager. Currently I have only done support for fedora and debian based systems.

Due to constraints on the system I am working on I have to support Bash 3.2 and above. This meant I got a bunch of complaints from shellcheck. Some of the things I think I did optimally

  • How to properly read variables into arrays on older bash versions
  • How to set a command pkg from the commandline argument pkg (for instance)
  • Not sure how to avoid using "$?" in the last if as it complains if I push the entire function into the condition
  • The last part using lsof is a bit of an hack, as I was unsure on how to extract the correct path without resorting to using the PID

Any feedback or improvements are more than welcome. I did run shfmt over the code, so it should be decently formatted.

Example output

enter image description here

#!/bin/bash

# finds processes listening in on the network outside outside of the default package manager

usage="usage $(basename "$0").sh [-h] [-f] [-a] [-w] [-v] [-p]

Lists packages listening on the network outside of the default package manager

where:
    -h,  --help              show this help text
    -f,  --whitelist-file    pick a file[s] containing whitelisted programs
    -a,  --list-all          display all processes listening on the network
    -w,  --whitelist         explicitly add whitelisted programs
    -v,  --view-whitelist    view the complete whitelist
    -p,  --package-manager   which package-manager to use (rpm, dkpg, ...)

output:
  COMMAND    PATH                PID    USER     FD   TYPE  DEVICE    SIZE/OFF  NODE  NAME
  mattermos  /app/main/mattermo  3351   361000   25u  IPv4  84625698  0t0       TCP   129.240...(ESTABLISHED)

examples:
    List all processes listening on the network
      usit_network_listeners --list-all 

    List only packages not maintained by the central package manager system
      usit_network_listeners --list-all 

    Whitelist some processes 
      usit_network_listeners --whitelist mattermos,systemd
      usit_network_listeners --whitelist 'mattermos systemd'

    Whitelist some processes using whitelist files
      usit_network_listeners -f /etc/uio/uio_listeners_whitelist_01.txt

    Multiple whitelist files is supported
      usit_network_listeners --whitelist foo.txt,bar.txt

environment variables:
  The following environment variables are available
  
    USIT_LISTENER_WHITELIST
    USIT_LISTENER_WHITELIST_FILES
  
  These form the base for the whitelist and whitelist files respectively. 
  Use a string with spaces or comma [,] as seperator for the values.
"

# Transform long options to short ones
for arg in "$@"; do
    shift
    case "$arg" in
    '--whitelist')        set -- "$@" '-w' ;;
    '--whitelist-file')   set -- "$@" '-f' ;;
    '--list-all')         set -- "$@" '-a' ;;
    '--view-whitelist')   set -- "$@" '-v' ;;
    '--package-manager')  set -- "$@" '-p' ;;
    '--help')             set -- "$@" '-h' ;;
    *)                    set -- "$@" "$arg" ;;
    esac
done

# The '//,/ ' allows us to input variables using , seperator as well as SPACE
read -ar whitelist <<<"${USIT_LISTENER_WHITELIST//,/ }"
read -ar whitelist_files <<<"${USIT_LISTENER_WHITELIST_FILES//,/ }"
list_all=false
view_whitelist=false
while getopts ":w:f:p:avh" flag; do
    case "${flag}" in
    h)
        echo "$usage"
        exit 0
        ;;
    a) list_all=true ;;
    v) view_whitelist=true ;;
    w) whitelist+=(${OPTARG//,/ }) ;;
    f) whitelist_files+=(${OPTARG//,/ }) ;;
    p) package_manager=${OPTARG} ;;
    \?)
        printf "illegal option: -%s\n" "$OPTARG" >&2
        echo "$usage" >&2
        exit 1
        ;;
    esac
done

# Some logic is needed to figure out which base package manager the system uses
dpkg_equivalents=("dpkg" "apt")
rpm_equivalents=("rpm" "yum" "dnf")
if [ -z "${package_manager}" ]; then
    if dpkg --version &>/dev/null; then
        package_manager="dpkg"
    elif rpm --version &>/dev/null; then
        package_manager="rpm"
    else
        echo "No supported package manager found! Try specifing one with -p --package-manager"
        exit 1
    fi
fi
if [[ ${dpkg_equivalents[*]} =~ ${package_manager} ]] && dpkg --version &>/dev/null; then
    package_manager="dpkg"
elif [[ ${rpm_equivalents[*]} =~ ${package_manager} ]] && rpm --version &>/dev/null; then
    package_manager="rpm"
else
    echo "ERROR: the package manager ${package_manager} is not supported on your system!"
    exit 1
fi

# The whitelist can also come from an array of files, add these to the global whitelist
while IFS= read -r whitelist_file; do
    if test -f "${whitelist_file}"; then
        readarray -t arr <"${whitelist_file}"
        whitelist=("${whitelist[@]}" "${arr[@]}")
    fi
done <<<"${whitelist_files[@]}"

# It could happen same file is whitelisted multiple times. We only keep unique values
whitelist=($(printf "%q\n" "${whitelist[@]}" | sort -u))

if [ "${view_whitelist}" = true ]; then
    if ((${#whitelist[@]})); then
        printf "%s\n" "${whitelist[@]}"
    fi
    exit
fi

# -i : This option filters only processes whith an IPv[46] address:
# -P : This option inhibits the conversion of port numbers to port names for network files.
# -n : This option inhibits the conversion of network numbers to host names for network files.
# -l : This option inhibits the conversion of user ID numbers to login names.
# +M : Enables the reporting of portmapper registrations for local TCP and UDP ports.
# The awk filters only unique PID's by  _[val] looks up val in the hash _(a regular variable).
# Tail is used to skip the header
listeners=$(
    lsof -Pnl +M -i |
        awk -F" " '!_[$1]++' |
        tail -n +2
)

# A bit cumbersome but this makes sure we define the correct package system only once
# Logic: If the command can not find $1 then output $2, we suppress any output as well
programname=""
if [ "${package_manager}" == "rpm" ]; then
    is_in_standard_repo() {
        rpm -qf "${1}" &>/dev/null
    }
elif [ "${package_manager}" == "dpkg" ]; then
    is_in_standard_repo() {
        dpkg -S "${1}" &>/dev/null
    }
else
    echo "The package manager ${package_manager} is not supported! Choose rpm (red hat) or dpkg (debian)"
    exit 1
fi

result=()
# Next step is extracting the paths from the PID's
while IFS= read -r line; do
    programname=$(echo "${line}" | awk '{print $1}')
    pid=$(echo "${line}" | awk '{print $2}')
    path=$(lsof -p "${pid}" 2>/dev/null | awk '/txt/ {print $9}')
    line=${line/$programname/$programname   $path}
    if [ "$list_all" = true ]; then
        result+=("${line}")
    else
        is_in_standard_repo "$path"
        # If path is in standard repo AND not in whitelist print the line
        if [ "$?" -gt 0 ] && [[ ! ${whitelist[*]} =~ ${programname} ]]; then
            result+=("${line}")
        fi
    fi
done <<<"$listeners"

# Pretty format the output
printf '%s\n' "${result[@]}" | column -t
\$\endgroup\$
3
  • \$\begingroup\$ Are you expecting this to run as a privileged (i.e. root) or as an unprivileged user? \$\endgroup\$ Aug 26, 2022 at 14:31
  • \$\begingroup\$ @TobySpeight This will be running as root =) \$\endgroup\$ Aug 26, 2022 at 15:02
  • \$\begingroup\$ In that case, you have full access to /proc, which might turn out more efficient than invoking lsof in that loop. \$\endgroup\$ Aug 26, 2022 at 17:22

2 Answers 2

4
\$\begingroup\$

First of all, pretty nice script! (I think you could drop the tag ;-)

Dodgy transformation of long options

When transforming the long options to short ones, I find it a bit dodgy to use set -- "$@" .... It feels like a dirty hack to iterate over the arguments \$n\$ to \$2n\$ times, and doing that in a for arg in "$@" loop with a shift, replacing the list being iterated on.

It would be cleaner to collect the transformed args in another array, and then replace $@ with the items of that array:

args=()

for arg; do
    case $arg in
    '--whitelist')        args+=(-w) ;;
    '--whitelist-file')   args+=(-f) ;;
    '--list-all')         args+=(-a) ;;
    '--view-whitelist')   args+=(-v) ;;
    '--package-manager')  args+=(-p) ;;
    '--help')             args+=(-h) ;;
    *)                    args+=("$arg") ;;
    esac
done

set -- "${args[@]}"

This is cleaner because the underlying argument list of the loop is not replaced in the middle of iterations, and $@ is not re-copied repeatedly, but replaced once at the end.

Always double-quote variables on the command line

I guess you don't support spaces in parameters of -w, because that would not work here:

w) whitelist+=(${OPTARG//,/ }) ;;

When appending the contents of a variable to an array, the correct form is:

arr+=("$somevar")

A clean solution would be:

IFS=, read -ra parts <<< "$OPTARG"
whitelist+=("${parts[@]}")
;;

Review the order or operations

A sequence of operations should be in order such that:

  • Avoid unnecessary work
  • Related operations are close together

Consider this part from the posted code, with some details hidden:

if [[ ${dpkg_equivalents[*]} =~ ${package_manager} ]] && ...; then
    package_manager="dpkg"
elif ...
    package_manager="rpm"
else
    exit 1
fi

while IFS= read -r whitelist_file; do
    # ...

listeners=$(lsof ...)

programname=""
if [ "${package_manager}" == "rpm" ]; then
    # ...
else
    exit 1
fi

while IFS= read -r line; do
    programname=...
    # ...
done <<<"$listeners"

At first I though that the output of lsof might not be used, since there's a conditional exit after it, independently.

Then I noticed that package_manager was already validated earlier in the code. Why is it checked again? The else branch will never be taken. If the code pieces related to package_manager were closer to each other, it would be easier to see that something is off.

Also notice that before the validation of package_manager, there is also an auto-detection mechanism. The validation should not be necessary when the value was set by auto-detection.

The lsof should be called right before the loop that uses its result. Also, instead of storing its output in a variable, you could pipe that directly into the while loop.

Finally, notice that programname is initialized too early. It appears right before an if-else, and I'm confused to not find any references to it in that if-else. It is updated in a while loop later, so it would be good to move the initialization to right in front of that loop.

Don't define functions in conditional blocks

It's cool that you can define functions in an if-else in Bash, but I think it's unusual. I would use a variable to store the function name, something like this:

is_in_rpm_repo() {
    rpm -qf "${1}" &>/dev/null
}

is_in_standard_repo=
if [ "${package_manager}" == "rpm" ]; then
    is_in_standard_repo=is_in_rpm_repo
    # ...


if "$is_in_standard_repo" "$path"; then
    # ...

Use consistent style

Most conditions use [ ... ], but there is one instance of test. I recommend [ ... ] consistently.

Many places use <<< "..." here-strings, but there are a few echo | .... I recommend here-strings consistently.

Some of the error messages are correctly printed to stderr. Some are printed to stdout. And since it's a bit annoying to type >&2 and exit 1 repeatedly, I usually define a fatal helper function:

fatal() {
    echo "fatal: $*" >&2
    exit 1
}

Use a here-document instead of a multiline double-quoted string

With a multiline double-quoted string as in the definition of the usage variable, I would be afraid that someday somebody will accidentally add a double-quote somewhere in the content and forgets to escape it properly, breaking the script. I would sleep better if the long string was defined as a here-document, which is harder to mess up.

read -r usage <<< EOF
# ...
EOF
\$\endgroup\$
0
4
\$\begingroup\$

This chain looks like a candidate for casein:

if [ "${package_manager}" == "rpm" ]; then
⋮ 
elif [ "${package_manager}" == "dpkg" ]; then
⋮ 
else
⋮ 
fi

Also, the error message in the else block should go to the error stream, not the output stream. That appears to be an oversight, given that other error messages go to the right place.


If $package_manager can only have one of a few values, we could name our function alternatives thus:

is_in_standard_repo_rmp() {
    rpm -qf "${1}" &>/dev/null
}
is_in_standard_repo_dpkg() {
    dpkg -S "${1}" &>/dev/null
}

and then invoke is_in_standard_repo_$package_manager to get the correct one. This is particularly useful once you accumulate additional per-platform functions and/or more platforms.


Why not sort/uniq the whitelist before assigning to array? I.e. replace this:

while IFS= read -r whitelist_file; do
        readarray -t arr <"${whitelist_file}"
        whitelist=("${whitelist[@]}" "${arr[@]}")
done <<<"${whitelist_files[@]}"

whitelist=($(printf "%q\n" "${whitelist[@]}" | sort -u))

with:

readarray -t whitelist < <(sort -u "${whitelist_files[@]}")

The comment here is missing a negative if it's to agree with the code:

    is_in_standard_repo "$path"
    # If path is in standard repo AND not in whitelist print the line
    if [ "$?" -gt 0 ] && [[ ! ${whitelist[*]} =~ ${programname} ]]

if [ "$?" -gt 0 ] is a well-known anti-pattern. Just use the command's exit status directly rather than examining $?:

if ! is_in_standard_repo "$path" && …

We can simplify here:

if [ "$list_all" = true ]; then

Given that we know the variable must be either true or false, we can just execute it:

if "$list_all"
\$\endgroup\$
2
  • \$\begingroup\$ Great answer! So many useful tips and tricks. Unfortunately the readarray did not work as I need to support bash 3.2+ \$\endgroup\$ Aug 27, 2022 at 9:36
  • \$\begingroup\$ I wasn't quite sure how that fitted in with the version requirements, but glad the rest was helpful. \$\endgroup\$ Aug 29, 2022 at 6:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.