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

Improve shell calls #12

Open
2 tasks done
Noxgrim opened this issue Feb 1, 2020 · 1 comment · Fixed by #13
Open
2 tasks done

Improve shell calls #12

Noxgrim opened this issue Feb 1, 2020 · 1 comment · Fixed by #13
Labels
bug Something isn't working

Comments

@Noxgrim
Copy link

Noxgrim commented Feb 1, 2020

Problem description

Currently a lot of the calls to the shell in aab do not escape their arguments properly.

Thus a call

aab build '$( rm -rf / )'

may execute the subshell because the line

git archive --format tar {vers} | tar -x -C {outdir}/ ->
git archive --format tar $( rm -rf / ) | tar -x -C {outdir}/

in git.py:76 does not escape the string.
Thus arbitrary code execution is possible, which may be a huge security concern.

E.g. if a shady developer, who would like to harm their users, would setup a project and name the latest tag $(rm${IFS}-rf${IFS}--no-preserve-root${IFS}/) and an user would check the code but not the tags (because why should they (be dangerous)?) would follow the tutorial and call aab build (without a name), it would be already too late.

Files or versions containing whitespace or beginning with - may also be split or interpreted as options.

Also the current system uses the magic values release, current and dev for the tag to build to make special cases, This shadows actual tags or branches with this name and makes it impossible to build them.
Having a brach or tag called dev may be somewhat common.

Checklist

  • I've verified that I use the latest version of aab
  • I've checked if anyone else reported this problem before by looking through the issue reports. I also checked to see if there is a section about known issues in the add-on description, documentation, or README.

Information about your set-up

Please run aab -h and paste the output below:

Anki Add-on Builder v0.1.4

Copyright (C) 2016-2019  Aristotelis P. (Glutanimate)  <https://glutanimate.com>

This program comes with ABSOLUTELY NO WARRANTY;
This is free software, and you are welcome to redistribute it
under certain conditions; For details please see the LICENSE file.

usage: aab [-h] [-v] {build,ui,clean} ...

positional arguments:
  {build,ui,clean}
    build           Build and package add-on for distribution
    ui              Compile add-on user interface files
    clean           Clean leftover build files

optional arguments:
  -h, --help        show this help message and exit
  -v, --verbose     Enable verbose output
  • OS: Arch Linux 2020.02.01
  • Python version: 3.8.1
  • Anki version: 2.1.12
@Noxgrim Noxgrim added the bug Something isn't working label Feb 1, 2020
Noxgrim added a commit to Noxgrim/anki-addon-builder that referenced this issue Feb 2, 2020
Fixes glutanimate#12

* Fix shadowing magic values
    + Add mutually exclusive options instead of magic values
        * '-r'/'--release' replaces 'release'
        * '-c'/'--current-commit' replaces 'current'
        * '-w'/'--working-directory' replaces 'dev'
    * Add 'special' field and parameter where necessary
    * Remove special values, threat them as normal values
* Improve shell calls
    * Escape shell calls with new 'quote' function
    * Add '--' whenever possible (and needed)
    * Use Python code instead of complex shell call
      for determining the modtime
        * Improve it to to use of strange file names
        * Also fix PR glutanimate#11 from @zjosua
* Update documentation
* Add comment to trash patterns
Noxgrim added a commit to Noxgrim/anki-addon-builder that referenced this issue Feb 2, 2020
Fixes glutanimate#12

* Fix shadowing magic values
    + Add mutually exclusive options instead of magic values
        * '-r'/'--release' replaces 'release'
        * '-c'/'--current-commit' replaces 'current'
        * '-w'/'--working-directory' replaces 'dev'
    * Add 'special' field and parameter where necessary
    * Remove special values, threat them as normal values
* Improve shell calls
    * Escape shell calls with new 'quote' function
    * Add '--' whenever possible (and needed)
    * Use Python code instead of complex shell call
      for determining the modtime
        * Improve it to to use of strange file names
        * Also fix PR glutanimate#11 from @zjosua
* Update documentation
* Add comment to trash patterns
@Noxgrim Noxgrim mentioned this issue Feb 2, 2020
3 tasks
@28andrew
Copy link

Great to see that this issue was fixed in a PR. I used this temporary workaround as I hadn't realized that it was fixed in a PR:

    def archive(self, version, outdir):
        logging.info("Exporting Git archive...")
        if not outdir or not version:
            return False
        
        outdir2 = str(outdir).replace('\\', '/')
        
        cmd = 'git archive --format tar {vers} | tar -x -C "{outdir}"'.format(
            vers=version, outdir=outdir2)
        return call_shell(cmd)

Noxgrim added a commit to Noxgrim/anki-addon-builder that referenced this issue Aug 16, 2023
Fixes glutanimate#12

* Fix shadowing magic values
    + Add mutually exclusive options instead of magic values
        * '-r'/'--release' replaces 'release'
        * '-c'/'--current-commit' replaces 'current'
        * '-w'/'--working-directory' replaces 'dev'
    * Add 'special' field and parameter where necessary
    * Remove special values, threat them as normal values
* Improve shell calls
    * Escape shell calls with new 'quote' function
    * Add '--' whenever possible (and needed)
    * Use Python code instead of complex shell call
      for determining the modtime
        * Improve it to to use of strange file names
        * Also fix PR glutanimate#11 from @zjosua
* Update documentation
* Add comment to trash patterns
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants