Fix unintended command execution by "_quote_readline_by_ref"

[akinomyoga]

_quote_readline_by_ref in bash_completion has a problem similar to the code injection vulnerability. This PR solves the problem.

Edit (2021-03-16): There was already a related issue #189, so let me mark as [ Fix #189 ].

Repeat-By

Here I describe a simple reproducer. With the following operation, a command touch file is unintendedly executed inside the completion function.

$ bash --rcfile bash_completion.sh
$ echo '$(touch file)[TAB]

In the above code block, [TAB] represents pressing TAB key. When TAB is pressed, the command touch file is executed, which can be confirmed by an unintended new file file.

For a more practical example (in which I noticed this problem), a file named 0.0 is created by the following operation:

awk '$1 > 0.0[TAB]

Investigation

This is caused by the implementation of the function _quote_readline_by_ref:

_quote_readline_by_ref()
{
    if [[ $1 == \'* ]]; then
        # Leave out first character
        printf -v $2 %s "${1:1}"
    else
        printf -v $2 %q "$1"
    fi

    # If result becomes quoted like this: $'string', re-evaluate in order to
    # drop the additional quoting.  See also:
    # https://www.mail-archive.com/[email protected]/msg01942.html
    [[ ${!2} == \$* ]] && eval $2=${!2}
} # _quote_readline_by_ref()

When the completed word has the form '$..., a command cur=$... is executed. First, inside the completion function, _quote_readline_by_ref is called in the following ways:

_quote_readline_by_ref "'\$(touch file)" cur
_quote_readline_by_ref "'\$1 > 0.0" cur

Then, after the first printf %s in _quote_readline_by_ref, the variable cur has the value such like $(touch file) or $1 > 0.0. Finally, in the last line of _quote_readline_by_ref, cur=$(touch file) or cur=$1 > 0.0 is executed.

Fix

As described in the code comment, the purpose of the last line [[ ${!2} == \$* ]] && eval $2=${!2} seems to be the removal of the additional quote of the form $'string' which would be only resulted from printf %q. Here, I suggest moving the last line inside the if statement so that the additional quote removal is only performed for the result of printf %q.

Thank you!

[akinomyoga]

Today I found that #189 is actually the same issue as the one that this PR aims to solve.

@scop May I ask why this pull request (PR) isn't reviewed for a long time even though all the other PRs after this have already received comments or have been merged? If something necessary to be reviewed is missing, could you please let me know that? (Note that the CI errors in this PR also happen in the master branch and all the other PRs so are not caused by this PR.) Thank you.

[scop]

@scop May I ask why this pull request (PR) isn't reviewed for a long time

Mainly because the code this PR touches is something a) I wish didn't exist in the first place, and b) I haven't ever dug into it to actually understand it in depth, and have trouble finding motivation for, kind of due to a). So I'm not in a place where I would be comfortable with reviewing this, and nobody else has stepped up.

Those two mentioned things combined plus the fact that this PR does not contain any tests to reproduce the problem or to demonstrate the fix, and that we have close to zero existing coverage at least for this particular function in the test suite (it could be tested by some other tests, but I can't tell that offhand) makes it all too easy for this to slip under my radar. The thorough description is appreciated, but implementing that as tests for the test suite instead would be more that. Even more would be a PR that removes this stuff (ditto everything else that uses eval), but I digress.

[akinomyoga]

@scop Sorry for the late reply. I was a bit busy recently. I have updated the PR. Could you now review the PR?

• a28fb29 I changed to an alternative implementation for decoding the escape string of the form $'...'. I have switched from eval to printf. Now we are free from eval in this function.
• ad128e3 I added test cases which failed before the fix. You can checkout commit ad128e3 and see they actually fail. These problems are fixed in commit a28fb29.

  $ git checkout ad128e32
  $ cd test/t
  $ pytest-3 unit/test_unit_quote_readline.py
  
  [...]
  
  unit/test_unit_quote_readline.py ..FFFFF
  
  [...]
  
  $ git checkout a28fb29d
  $ pytest-3 unit/test_unit_quote_readline.py
  
  [...]
  
  unit/test_unit_quote_readline.py .......

[scop]

Thanks for your work, much happier with this now. Added some test code related wishes.

[akinomyoga]

I actually have another question. If we want to perform tests in a specific directory, or if we want to perform the tests under a certain shell setting, what is the proper way to do that?

• (a) If we just change the shell settings, the environment change would be detected and the test would fail.

• (b) In this PR, I have done the tests in subshells, but in this way, we cannot detect other unexpected changes to the environment.

• (c) We can save the current setting, then do the tests, and finally restore the settings. This can be cumbersome depending on the type of the shell setting. For example, with this approach, test_github_issue_492_4 would become

  with tempfile.TemporaryDirectory() as tmpdir:
      assert_bash_exec(
          bash,
          (
              "_test_unit_OLDPWD=$OLDPWD;"
              "_test_unit_SHOPT=$(shopt -p failglob);"
              "cd %s;"
              "shopt -s failglob;"
              "quote_readline $'a\\\\\\tb*' >/dev/null;"
              "eval -- \"$_test_unit_SHOPT\";"
              "cd - 2>/dev/null"
              "OLDPWD=$_test_unit_OLDPWD;"
              "unset -v _test_unit_OLDPWD _test_unit_SHOPT"
          ) % self.quote_bash_word(tmpdir))

  But this still doesn't work because bash.expect blocks when the command line doesn't fit in a line. The terminal size is set to (24, 160)---

  bash-completion/test/t/conftest.py

  Line 211 in 23382c0

  dimensions=(24, 160),

  ---so the maximum length of the tested command needs to be about less than 160. Finally, we need to do the tests in the following way:

  with tempfile.TemporaryDirectory() as tmpdir:
      # Fake test to set up the shell state
      assert_bash_exec(
          bash,
          ("_test_unit_OLDPWD=$OLDPWD;"
           "_test_unit_SHOPT=$(shopt -p failglob);"
           "cd %s;"
           "shopt -s failglob;") % self.quote_bash_word(tmpdir)
      )
  
      # Real test
      assert_bash_exec(bash, "quote_readline $'a\\\\\\tb*' >/dev/null;")
  
      # Fake test to restore the shell state
      assert_bash_exec(
          bash,
          "eval -- \"$_test_unit_SHOPT\";"
          "cd - >/dev/null;"
          "OLDPWD=$_test_unit_OLDPWD;"
          "unset -v _test_unit_OLDPWD _test_unit_SHOPT"
      )

---

By the way, why is the environment variable OLDPWD allowed to be changed in the tests? If completers change OLDPWD, users cannot use cd - reliably to go back to the previous directory.

bash-completion/test/t/conftest.py

Lines 391 to 392 in 23382c0

	# Ignore variables expected to change:
	and not re.search("^[-+](_|PPID|BASH_REMATCH|OLDPWD)=", x)

[scop]

Re working dir, the cwd arg to pytest.mark.complete might help.

For shell settings, env changes etc, pre_cmds (and post_cmds to clean up) might help.

But then again, the above might not help much after all, depends :)

Re temp dir usage, I've had the intention to look into using pytest's tmp_path fixture, but haven't got around to it. We shouldn't expect to be able to write to our source/test dirs. https://docs.pytest.org/en/stable/tmpdir.html

Re terminal size, I'm afraid I don't remember the details.

Changing OLDPWD isn't "allowed", changes in its value are just expected as tests may cd around. We could remove the special treatment for it if someone wants to land work to make that happen. pushd and popd could be an alternative.

[akinomyoga]

Re working dir, the cwd arg to pytest.mark.complete might help.

For shell settings, env changes etc, pre_cmds (and post_cmds to clean up) might help.

But then again, the above might not help much after all, depends :)

Re temp dir usage, I've had the intention to look into using pytest's tmp_path fixture, but haven't got around to it. We shouldn't expect to be able to write to our source/test dirs. https://docs.pytest.org/en/stable/tmpdir.html

I have updated the tests and rebased. I decided to perform the entire tests in unit/test_unit_quote_readline.py in a temporary directory by specifying arguments to @pytest.mark.bashcomp() decorator for class TestUnitQuoteReadline. I've looked at tmp_path, but it seems to change the Python's current working directory for each test function. Instead, what we want is to change the current working directory of the Bash process, so I just changed the arguments to bash.spawn by providing new arguments of @pytest.mark.bashcomp().

Change to conftest.py No. 1 (cc55970): ensure to call the cleanup codes

Before modifying def bash for new arguments of @pytest.mark.bashcomp(), I changed def bash so that it ensures to call the cleanup codes. Previously, cleanup codes for bash and logfile might be skipped when there are assertion failures (though they might be finally cleaned up when the destructors are called on later garbage collection). I wanted to make this change before I add the cleanup code for the temporary directory. In this commit, indentation has been changed by the introduction of the try ... finally ... construct, but the essential change of this commit (git diff -w cc559704~ cc559704) is as follows:

diff --git a/test/t/conftest.py b/test/t/conftest.py
index 3386b9ff..551f08a7 100644
--- a/test/t/conftest.py
+++ b/test/t/conftest.py
@@ -195,6 +195,8 @@ def bash(request) -> pexpect.spawn:
         )
     )

+    bash = None
+    try:
         fixturesdir = os.path.join(testdir, "fixtures")
         os.chdir(fixturesdir)

@@ -237,6 +239,7 @@ def bash(request) -> pexpect.spawn:
                     pass
                 else:
                     bash.close()
+                    bash = None
                     pytest.skip(skipif)
             xfail = marker.kwargs.get("xfail")
             if xfail:
@@ -273,7 +276,9 @@ def bash(request) -> pexpect.spawn:
             for post_cmd in marker.kwargs.get("post_cmds", []):
                 assert_bash_exec(bash, post_cmd, want_output=None)

+    finally:
         # Clean up
+        if bash:
             bash.close()
         if logfile and logfile != sys.stdout:
             logfile.close()

Change to conftest.py No. 2 (426f855): Class decorator @pytest.mark.bashcomp now accepts arguments cwd and temp_cwd

Because the lifetime of the temporary directory needs to be managed along with that of the Bash process, I decided to add the code of temporary directories in def bash. I also added support for the argument cwd to specify an existing directory. When @pytest.mark.bashcomp(cwd="path-to-directory") is specified, Bash is started with that directory being the current working directory. Otherwise, when @pytest.mark.bashcomp(temp_cwd=True) is specified, Bash is started in a newly-created temporary directory. Otherwise, the current directory will be test/fixtures as in the previous behavior. Note that @pytest.mark.bashcomp(cwd=tempfile.TemporaryDirectory().name) doesn't work because the lifetime of the decorator arguments is different from the duration of the function call.

Refactor unit/test_unit_quote_readline.py (a08b0c7)

Using @pytest.mark.bashcomp(pre_cmds=(...,)) as well as the new argument temp_cwd, I updated the tests. Now it is much simpler and clearer than before.

---

If we should separate the PR for the change to conftest.py, please let me know

[akinomyoga]

Re terminal size, I'm afraid I don't remember the details.

When the tested command (i.e., the string sent to the Bash stdin) is longer than the terminal width, Bash will output

echo Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tem
por incididunt ut labore et dolore magna aliqua.

while pexpect expects unwrapped text

echo Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

Because pexpect never finds the "expected" unrapped text, the test blocks. I think the simplest way to solve this issue is just to increase the terminal width sufficiently large such as 9999.

Changing OLDPWD isn't "allowed", changes in its value are just expected as tests may cd around. We could remove the special treatment for it if someone wants to land work to make that happen. pushd and popd could be an alternative.

OK. If it is not too hard, maybe I'll do that after this PR is settled.

Thank you for your all comments so far.

[scop]

Awesome work, thanks for that, and your patience!

[scop]

I think the simplest way to solve this issue is just to increase the terminal width sufficiently large such as 9999.

I have a nagging feeling that overly wide terminal might cause issues related to receiving output from some commands. But I'm up to trying that out, guess we'd witness those pretty much instantly. PR's welcome :)

[akinomyoga]

Thank you! I have a comment on a lint fix.

[akinomyoga]

I have a nagging feeling that overly wide terminal might cause issues related to receiving output from some commands. But I'm up to trying that out, guess we'd witness those pretty much instantly. PR's welcome :)

I have run the entire test by make check after changing the width to 9999, but there seems to be no problem. But actually, we don't have such long test commands currently, so I think it is fine to keep it to be 160 for now.