Skip to content

[Bug]: sanitize_input does not work as expected #324

Closed
@dimitry-ishenko

Description

What happened?

While testing PR #320 I would get the following error:

admin@armbian:~/configng$ bin/armbian-config --api pkg_install vim-airline

Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
E: Unable to locate package Invalid argument: vim-airline

Upon further testing, any package containing dash (-) in the name would produce similar error.

It turns out the error is generated by the sanitize_input function which does not allow dashes. However, the pipeline doesn't seem to work as expected, since instead of just reporting the error "Invalid argument: vim-airline," this output is passed further down the pipeline.

In the end the pkg_install function tries to install a package called Invalid argument: vim-airline, which results in a confusing error.

How to reproduce?

See above.

On which host OS are you running the build script and observing this problem?

Ubuntu 24.04 Noble

Code of Conduct

  • I agree to follow this project's Code of Conduct

Activity

dimitry-ishenko

dimitry-ishenko commented on Dec 14, 2024

@dimitry-ishenko
CollaboratorAuthor

Looking at

sanitize_input() {
local sanitized_input=()
for arg in "$@"; do
if [[ $arg =~ ^[a-zA-Z0-9_=]+$ ]]; then
sanitized_input+=("$arg")
else
echo "Invalid argument: $arg"
exit 1
fi
done
echo "${sanitized_input[@]}"
}

My guess is that the intended behavior was to terminate program execution and print out an error message. However, in most cases sanitize_input will be used in a sub-shell such as

args=$(sanitize_input "$@")

or
args=$(sanitize_input "$@")

This will terminate the sub-shell with exit code 1, but unless checked in the super-shell, will continue normal execution, which is what's happening here.

dimitry-ishenko

dimitry-ishenko commented on Dec 14, 2024

@dimitry-ishenko
CollaboratorAuthor

Suggestions to resolve the bug:

  • When reporting an error, direct it at stderr and not stdout (which is why we have it 😉 ), eg:

    echo "Invalid argument: $arg" >&2
    exit 1
    
  • Check for sub-shell exit code:

    args=$(sanitize_input "$@") || exit 1
    
  • We can also leverage set -e option, which will automatically abort script execution on error.

  • To add consistency and reduce boiler-plate we could introduce commands such as:

    error() { echo "$@" >&2; }
    die() { error "$@"; exit 1; }
    

    Then the above code becomes something like:

    sanitize_input() {
        ...
            die "Invalid argument: $arg"
        ...
    }

    and

    args=$(sanitize_input "$@") || die
dimitry-ishenko

dimitry-ishenko commented on Dec 14, 2024

@dimitry-ishenko
CollaboratorAuthor

And, obviously, sanitize_input should allow dashes in the input.

Tearran

Tearran commented on Dec 15, 2024

@Tearran
Member

@dimitry-ishenko Sanitise input contribution was aimed primarily at removing escape sequences.
This is one of the placeholder implementations that is incomplete and needs refinement or replacement. Another example you are familiar with is process_input.

This has been a place for expanding and learning: If a may ask :) Are you saying that while the implementation is flawed (BUG), the concept is not, and sanitize_input should handle input validation more comprehensively rather than be removed?

For later implementation and though.
If we validate, should we not include a manner to use the database to actually verify inputs?

dimitry-ishenko

dimitry-ishenko commented on Dec 15, 2024

@dimitry-ishenko
CollaboratorAuthor

Are you saying that while the implementation is flawed (BUG), the concept is not, and sanitize_input should handle input validation more comprehensively rather than be removed?

It's hard to tell without knowing the original intent, but I would say it should be removed... I am not a security specialist, but I don't see what problem it's trying to solve... Is there a case of using an escape sequence to compromise the system?

Or, if there is a valid need for it, then maybe it can be modified to use a blacklist to detect escape char in the input instead.

Tearran

Tearran commented on Dec 15, 2024

@Tearran
Member

The intent was precaution more than anything for when armbian-config was not strictly an admin tool as it is now.

dimitry-ishenko

dimitry-ishenko commented on Dec 15, 2024

@dimitry-ishenko
CollaboratorAuthor

Maybe it should be removed for now, and if the need arises later, it can be resurrected. Just my 2 cents.

dimitry-ishenko

dimitry-ishenko commented on Jan 29, 2025

@dimitry-ishenko
CollaboratorAuthor

@Tearran if you don't mind, I'll do a small PR that drops sanitize_input like you did in #326?

I want to get it in, so I can finish my service wrapper PR (#323) and then Samba. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    BugSomething isn't working as it should

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      [Bug]: `sanitize_input` does not work as expected · Issue #324 · armbian/configng