Skip to content
This repository has been archived by the owner. It is now read-only.

Properly escape commands in pmb.chroot.user() #1316

Merged
merged 1 commit into from Mar 10, 2018

Conversation

@ollieparanoid
Copy link
Member

commented Mar 10, 2018

Introduction

In #1302 we noticed that pmb.chroot.user() does not escape commands
properly: When passing one string with spaces, it would pass them as
two strings to the chroot. The use case is passing a description with
a space inside to newapkbuild with pmboostrap newapkbuild.

This is not a security issue, as we don't pass strings from untrusted
input to this function.

Functions for running commands in pmbootstrap

To put the rest of the description in context: We have four high level
functions that run commands:

  • pmb.helpers.run.user()
  • pmb.helpers.run.root()
  • pmb.chroot.root()
  • pmb.chroot.user()

In addition, one low level function that the others invoke:

  • pmb.helpers.run.core()

Flawed test case

The issue described above did not get detected for so long, because we
have a test case in place since day one, which verifies that all of the
functions above escape everything properly:

  • test/test_shell_escape.py

So the test case ran a given command through all these functions, and
compared the result each time. However, pmb.chroot.root()
modified the command variable (passed by reference) and did the
escaping already, which means pmb.chroot.user() running directly
afterwards only returns the right output when not doing any escaping.

Without questioning the accuracy of the test case, I've escaped
commands and environment variables with shlex.quote() before
passing them to pmb.chroot.user(). In retrospective this does not
make sense at all and is reverted with this commit.

Environment variables

By coincidence, we have only passed custom environment variables to
pmb.chroot.user(), never to the other high level functions. This only
worked, because we did not do any escaping and the passed line gets
executed as shell command:

$ MYENV=test echo test2
test 2

If it was properly escaped as one shell command:

$ 'MYENV=test echo test2'
sh: MYENV=test echo test2: not found

So doing that clearly doesn't work anymore. I have added a new env
parameter to pmb.chroot.user() (and to all other high level functions
for consistency), where environment variables can be passed as a
dictionary. Then the function knows what to do and we end up with
properly escaped commands and environment variables.

Details

  • Add new env parameter to all high level command execution functions
  • New pmb.helpers.run.flat_cmd() function, that takes a command as
    list and environment variables as dict, and creates a properly escaped
    flat string from the input.
  • Use that function for proper escaping in all high level exec funcs
  • Don't escape commands before passing them to pmb.chroot.user()
  • Describe parameters of the command execution functions
  • pmbootstrap -v writes the exact command to the log that was
    executed (in addition to the simplified form we always write down for
    readability)
  • test_shell_escape.py: verify that the command passed by reference
    has not been modified, add a new test for strings with spaces, add
    tests for new function pmb.helpers.run.flat_cmd()
  • Remove obsolete commend in pmb.chroot.distccd about environment
    variables, because we don't use any there anymore
  • Add TERM=xterm to default environment variables in the chroot,
    so running ncurses applications like menuconfig and nano works out of the box
Properly escape commands in pmb.chroot.user()
.## Introduction
In #1302 we noticed that `pmb.chroot.user()` does not escape commands
properly: When passing one string with spaces, it would pass them as
two strings to the chroot. The use case is passing a description with
a space inside to `newapkbuild` with `pmboostrap newapkbuild`.

This is not a security issue, as we don't pass strings from untrusted
input to this function.

.## Functions for running commands in pmbootstrap
To put the rest of the description in context: We have four high level
functions that run commands:
* `pmb.helpers.run.user()`
* `pmb.helpers.run.root()`
* `pmb.chroot.root()`
* `pmb.chroot.user()`

In addition, one low level function that the others invoke:
* `pmb.helpers.run.core()`

.## Flawed test case
The issue described above did not get detected for so long, because we
have a test case in place since day one, which verifies that all of the
functions above escape everything properly:
* `test/test_shell_escape.py`

So the test case ran a given command through all these functions, and
compared the result each time. However, `pmb.chroot.root()`
modified the command variable (passed by reference) and did the
escaping already, which means `pmb.chroot.user()` running directly
afterwards only returns the right output when *not* doing any escaping.

Without questioning the accuracy of the test case, I've escaped
commands and environment variables with `shlex.quote()` *before*
passing them to `pmb.chroot.user()`. In retrospective this does not
make sense at all and is reverted with this commit.

.## Environment variables
By coincidence, we have only passed custom environment variables to
`pmb.chroot.user()`, never to the other high level functions. This only
worked, because we did not do any escaping and the passed line gets
executed as shell command:
```
$ MYENV=test echo test2
test 2
```
If it was properly escaped as one shell command:
```
$ 'MYENV=test echo test2'
sh: MYENV=test echo test2: not found
```
So doing that clearly doesn't work anymore. I have added a new `env`
parameter to `pmb.chroot.user()` (and to all other high level functions
for consistency), where environment variables can be passed as a
dictionary. Then the function knows what to do and we end up with
properly escaped commands and environment variables.

.## Details
* Add new `env` parameter to all high level command execution functions
* New `pmb.helpers.run.flat_cmd()` function, that takes a command as
  list and environment variables as dict, and creates a properly escaped
  flat string from the input.
* Use that function for proper escaping in all high level exec funcs
* Don't escape commands *before* passing them to `pmb.chroot.user()`
* Describe parameters of the command execution functions
* `pmbootstrap -v` writes the exact command to the log that was
  executed (in addition to the simplified form we always write down for
  readability)
* `test_shell_escape.py`: verify that the command passed by reference
  has not been modified, add a new test for strings with spaces, add
  tests for new function `pmb.helpers.run.flat_cmd()`
* Remove obsolete commend in `pmb.chroot.distccd` about environment
  variables, because we don't use any there anymore
@NotKit

NotKit approved these changes Mar 10, 2018

Copy link
Collaborator

left a comment

The code changes look fine to me. I run basic pmbootstrap functions (indexing, package compilation and building of rootfs) with this, no evident breakage with this.

Thanks to @ollieparanoid!

"HISTFILE": "~/.ash_history",
"PATH": pmb.config.chroot_path,
"SHELL": "/bin/ash",
"TERM": "xterm"}

This comment has been minimized.

Copy link
@NotKit

NotKit Mar 10, 2018

Collaborator

Is it ok to hardcode it to "xterm"? Konsole seems to have this set to "xterm-256color" by default, but probably this makes sense only in interactive session like "pmboostrap chroot".

This comment has been minimized.

Copy link
@MartijnBraam

MartijnBraam Mar 10, 2018

Member

I think xterm is a pretty safe default, I don't think we need any special terminal features.

This comment has been minimized.

Copy link
@ollieparanoid

ollieparanoid Mar 10, 2018

Author Member

It is what we've used so far for menuconfig. I don't mind changing it when using another value gives us an advantage. Alpine ships xterm-256color as well.

I'm merging this version (which does not change the value, only move it to export it by default), but if you know of any advantage we could easily make a follow-up PR that changes the value.

@ollieparanoid

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2018

@NotKit: thanks a lot for testing and reviewing, highly appreciated! 🎉

@ollieparanoid ollieparanoid merged commit 3666388 into master Mar 10, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 76.372%
Details

@ollieparanoid ollieparanoid deleted the fix/pmb-chroot-user-escaping branch Mar 10, 2018

postmarketOS-Wiki pushed a commit to postmarketOS/wiki that referenced this pull request Mar 11, 2018

fooforever added a commit to fooforever/pmbootstrap that referenced this pull request Mar 14, 2018

Properly escape commands in pmb.chroot.user() (postmarketOS#1316)
## Introduction
In postmarketOS#1302 we noticed that `pmb.chroot.user()` does not escape commands
properly: When passing one string with spaces, it would pass them as
two strings to the chroot. The use case is passing a description with
a space inside to `newapkbuild` with `pmboostrap newapkbuild`.

This is not a security issue, as we don't pass strings from untrusted
input to this function.

## Functions for running commands in pmbootstrap
To put the rest of the description in context: We have four high level
functions that run commands:
* `pmb.helpers.run.user()`
* `pmb.helpers.run.root()`
* `pmb.chroot.root()`
* `pmb.chroot.user()`

In addition, one low level function that the others invoke:
* `pmb.helpers.run.core()`

## Flawed test case
The issue described above did not get detected for so long, because we
have a test case in place since day one, which verifies that all of the
functions above escape everything properly:
* `test/test_shell_escape.py`

So the test case ran a given command through all these functions, and
compared the result each time. However, `pmb.chroot.root()`
modified the command variable (passed by reference) and did the
escaping already, which means `pmb.chroot.user()` running directly
afterwards only returns the right output when *not* doing any escaping.

Without questioning the accuracy of the test case, I've escaped
commands and environment variables with `shlex.quote()` *before*
passing them to `pmb.chroot.user()`. In retrospective this does not
make sense at all and is reverted with this commit.

## Environment variables
By coincidence, we have only passed custom environment variables to
`pmb.chroot.user()`, never to the other high level functions. This only
worked, because we did not do any escaping and the passed line gets
executed as shell command:
```
$ MYENV=test echo test2
test 2
```
If it was properly escaped as one shell command:
```
$ 'MYENV=test echo test2'
sh: MYENV=test echo test2: not found
```
So doing that clearly doesn't work anymore. I have added a new `env`
parameter to `pmb.chroot.user()` (and to all other high level functions
for consistency), where environment variables can be passed as a
dictionary. Then the function knows what to do and we end up with
properly escaped commands and environment variables.

## Details
* Add new `env` parameter to all high level command execution functions
* New `pmb.helpers.run.flat_cmd()` function, that takes a command as
  list and environment variables as dict, and creates a properly escaped
  flat string from the input.
* Use that function for proper escaping in all high level exec funcs
* Don't escape commands *before* passing them to `pmb.chroot.user()`
* Describe parameters of the command execution functions
* `pmbootstrap -v` writes the exact command to the log that was
  executed (in addition to the simplified form we always write down for
  readability)
* `test_shell_escape.py`: verify that the command passed by reference
  has not been modified, add a new test for strings with spaces, add
  tests for new function `pmb.helpers.run.flat_cmd()`
* Remove obsolete commend in `pmb.chroot.distccd` about environment
  variables, because we don't use any there anymore
* Add `TERM=xterm` to default environment variables in the chroot,
  so running ncurses applications like `menuconfig` and `nano` works out of
  the box
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.