Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature suggestion: Warnings regarding (automatic) recursive indirection/eval within [[ (( and $(( blocks #3067

Open
fedy-cz opened this issue Oct 5, 2024 · 2 comments

Comments

@fedy-cz
Copy link

fedy-cz commented Oct 5, 2024

Came across this (poorly documented) feature of bash arithmetic expansion within the [[ , (( and $(( blocks (my quick interpretation):

Within the arithmetic expansion in these blocks all variables are recursively expanded (using indirection) until the content can be parsed as an integer or no further indirection is possible. This includes evaluating arithmetic expressions (contained as a string within the variable).

Example:

#!/bin/bash
a=b
b=c
c=10
if [[ $a -gt 5 ]] ; then echo Greater ; else "Not greater"  ; fi

Outputs:

Greater

Side note: Interestingly shellcheck complains about b and c being unused here, which in fact is not true :)

This "feature" is discussed (for example) in this bash mailing list thread:
https://lists.gnu.org/archive/html/help-bash/2018-10/msg00011.html

Definitely an unexpected behavior (in my book), and very poorly documented. This could unearth a whole new class of bugs.

The instance I personally came across (simplified):

#!/bin/bash

default=10

use_input() {
  return 0
}

read_config() {
  local input
  input=$( < filename )
  echo "Read: $input"
  if use_input ; then
    # Notice the missing $ there
    global_setting=input
  else
    global_setting=$default
  fi

  # Final check
  if [[ $global_setting -gt 0 ]] ; then
    echo "global setting is numeric"
  else
    exit 1
  fi
}
read_config

echo "global setting: $global_setting"

When I put value 5 into filename this outputs:

Read: 5
global setting is numeric
global setting: input

Few other simple examples I can think off:

#!/bin/bash
# Somewhere:
x=5
var=x
# ...
# Later, attempt to make sure that a variable is a positive numeric value
if [[ var -gt 0 ]] ; then
  echo "var is numeric"
  # But in fact it's just x
fi

Or:

#!/bin/bash
internal_val=5
var=$( < file )
# file contains the string internal_val
if [[ var -gt 0 ]] ; then
  # expecting to be processing value from file here but instead working with the internal_val
  x = $((  var * 5 ))
fi

You can even put whole expressions in there as mentioned in the referred mailing list thread:

#!/bin/bash
one=1
two=2
x='one + two'
code='4 > 2 ? x : y'
# This  just evals
echo $(( code ))

The main pain points I see right now are:

  • unintended aliasing / recursive indirection & it's effects
  • incorrect input validation issues

Wouldn't be surprised if this led to a few security incidents as well.

Generally:

  • Can hide an accidental "variable name" string assignment instead of a variable value (missing $).
  • Anything that comes from user input should avoid being used within the (( , $(( and arithmetic part of [[ blocks unless properly validated / pattern matched first.
  • Never use these block for input/value format validation (one exception being [[ containing no arithmetic expression that uses the input).

Not sure how well this could translate into actual shellcheck rules. But kicking off the discussion.

@fedy-cz
Copy link
Author

fedy-cz commented Oct 5, 2024

One more:
The evaluated expressions can even contain assignments to outside variables, so:

#!/bin/bash

secret_password='top secret'
# Example: c is some non-validated user input
c='secret_password=0'
# We are just checking it right?
if (( c > 1 )) ; then
  echo "Do something"
fi
echo "PW: $secret_password"

Outputs:

PW: 0

Yikes! No global variable is safe :(

@brother
Copy link
Collaborator

brother commented Oct 7, 2024

Side note: Interestingly shellcheck complains about b and c being unused here, which in fact is not true :)

That is on the other hand documented as a design decision.
https://github.com/koalaman/shellcheck/wiki/SC2034#exceptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants