Buildozer silently ignores local recipes – and this is bad

[leycec]

tl;dr

Relative dirnames in p4a.local_recipes and --local_recipes have been broken for a year or two. Until this issue is resolved, please replace the ./ prefixing these dirnames with ../../../../ instead: e.g.,

# Instead of this...
p4a.local_recipes = ./p4a-recipes/

# ...just do this!
p4a.local_recipes = ../../../../p4a-recipes/

Rejoice as local recipes suddenly work. If you too are now facepalming, this emoji's for you. :facepalm:

Versions or It Didn't Happen

• Python: Any
• OS: Gentoo Linux, of course!
• Buildozer: 1.3.0

Exegesis or It Didn't Happen

Buildozer silently ignores relative dirnames for both the p4a.local_recipes setting in buildozer.spec files and the --local_recipes option accepted by most p4a subcommands. Since the only sane means of specifying a local recipe directory is with a relative dirname, local recipes have effectively been broken for at least a year or two... which is bad.

Clearly, local recipes used to work. As this issue will show, they no longer do. The breakage probably happened when Buildozer was refactored to change the current working directory (CWD) to the deeply nested .buildozer/android/platform/python-for-android/ subdirectory before running p4a. Previously, Buildozer must not have done that; it must have preserved the CWD as the current top-level project directory containing the root buildozer.spec file. Now, Buildozer changes the CWD to a deeply nested subdirectory breaking relative dirnames in buildozer.spec files.

Let's dissect this. :scissors:

buildozer.spec

Let's use this spec file to build the Blap app, Buildozer!

[app]
p4a.local_recipes = ./p4a-recipes/
title = Blap
package.name = blap
source.dir = .
version = 0.0.1
requirements = python3>=3.9.0, kivy>=2.1.0, bleak>=0.14.0
android.accept_sdk_license = True
android.archs = arm64-v8a, armeabi-v7a

[buildozer]
log_level = 2

Let's define a trivial recipe for the bleak requirement that mostly does nothing except exist!

$ mkdir -p p4a-recipes/bleak
$ cat << EOF > p4a-recipes/bleak/__init__.py
from pythonforandroid.recipe import PythonRecipe

class BleakRecipe(PythonRecipe):
    version = '0.14.0'
    url = 'https://pypi.python.org/packages/source/b/bleak/bleak-{version}.tar.gz'
    name = 'bleak'

recipe = BleakRecipe()
EOF

Lastly, let's build the Android Blap app, Buildozer!

$ buildozer android debug

We are now ready to break you, Buildozer.

Logs

There's little point in pasting the full output, so let's reduce this to the only line that matters:

$ buildozer android debug
...
[INFO]:    	blap: min API 21, includes recipes (hostpython3, libffi, openssl, sdl2_image, sdl2_mixer, sdl2_ttf, sqlite3, python3, sdl2, setuptools, six, pyjnius, android), built for archs (arm64-v8a, armeabi-v7a)

Note the conspicuous absence of bleak above. So, Buildozer has failed to find our local p4a-recipes/bleak recipe. B-b-but whatever could the matter be...? Just kidding! I figured everything out already. It's the pythonforandroid.recipe.Recipe.recipe_dirs() classmethod, which is painfully broken with respect to Buildozer's CWD behaviour. </sigh>

It's Raining Solutions

Ah-ha. Bet you weren't expecting that one, were you, @misl6? Nobody ever submits solutions. They only complain endlessly. Right? I know. I'm usually one of those people, because I am lazy. But complaining mostly doesn't work, as evidenced by the 199 currently open issues on this tracker.

So... I did a deep dive on this for the community. There are multiple intersecting issues here, each of which should be resolved before this issue is eventually closed. They all involve the aforementioned pythonforandroid.recipe.Recipe.recipe_dirs() classmethod, whose implementation is so naive it makes me just want to stop doing anything and play Persona 5 Royal for the 10th time. Specifically, that classmethod:

1. Isn't tested by anything. Tests shouldn't pass, because p4a.local_recipes and --local_recipes are both utterly broken – and have been that way for a few years now. But tests pass. Ergo, someone who is not me really needs to write working unit and integration tests ensuring this never happens again. Of course, test coverage is at a shocking low of 39%, so... what you gonna do? It doesn't help that both Buildozer and python-for-android need additional tests:
   • Buildozer needs integration tests ensuring that python-for-android is actually respecting the Buildozer-specific p4a.local_recipes setting. It currently isn't.
   • python-for-android needs unit tests ensuring that the Recipe.recipe_dirs() classmethod:
     • Raises exceptions when the local recipe directory doesn't actually exist.
     • Registers the local recipe directory when that directory does exist.
2. Fails to log anything. Seriously. This classmethod needs to start logging something.
3. Fails to raise any exceptions. If the user explicitly specifies a local recipes directory (via either p4a.local_recipes or --local_recipes) that does not exist, this classmethod needs to raise a human-readable exception. Currently, this classmethod silently ignores this edge case – which is actually all cases, because this classmethod erroneously canonicalizes most relative dirnames to non-existent absolute dirnames. Cue more facepalm emojis. :man_facepalming: :woman_facepalming:
4. Fails to correctly canonicalize relative dirnames. When the user specifies a local recipes directory like p4a.local_recipes = ./p4a-recipes/ or --local_recipes=./p4a-recipes/, they expect the ./ prefixing that dirname to refer to the top-level project directory containing the root buildozer.spec file. Instead, Buildozer unexpectedly:
   1. Changes the current working directory (CWD) to a private Buildozer-isolated python-for-android subdirectory:

      # Cwd /home/itsmeleycec/blap/.buildozer/android/platform/python-for-android
      

   2. Eventually attempts to canonicalize that local recipes directory against that private Buildozer-isolated python-for-android subdirectory rather than against the top-level project directory. Why does this classmethod do that? Because this classmethod is naive:

          @classmethod
          def recipe_dirs(cls, ctx):
              recipe_dirs = []
              if ctx.local_recipes is not None:
                  recipe_dirs.append(realpath(ctx.local_recipes))  # <-- THIS IS BALLS
              ...
      

See the line conspicuously marked # <-- THIS IS BALLS? Yeah. That's the problem. When that line is interpreted, ctx.local_recipes == './p4a-recipes/'. Since Buildozer changes the CWD beforehand to the .buildozer/android/platform/python-for-android/ subdirectory, passing that relative dirname to the os.path.realpath() function incorrectly expands that dirname to .buildozer/android/platform/python-for-android/p4a-recipes/.

Of course, that directory doesn't actually exist – but recipe_dirs() doesn't care. recipe_dirs() is naive. It crosses its fingers and just silently appends that non-existent directory to the returned list of recipe dirnames. Cue badness. :crossed_fingers:

Here's what the Recipe.recipe_dirs() classmethod needs to look like instead:

```python
from os.path import (
    basename, dirname, exists, isabs, isdir, isfile, join, realpath, split)
...

class Recipe(with_metaclass(RecipeMeta)):
    ...

    @classmethod
    def recipe_dirs(cls, ctx):
        recipe_dirs = []
        if ctx.local_recipes is not None:
            # If the local recipes directory is absolute, accept this directory
            # as is; else, canonicalize this relative directory against this
            # project's directory (rather than the current working directory).
            local_recipes_dir = realpath(
                ctx.local_recipes
                if isabs(ctx.local_recipes) else
                join(ctx.project_dir, ctx.local_recipes)
            )

            if not isdir(local_recipes_dir):
                raise ValueError(
                    f'Local recipes directory "{local_recipes_dir}" not found.')

            recipe_dirs.append(local_recipes_dir)
        ...

Crucially, we now:

1. Correctly canonicalize that directory against the project directory.
2. Raise an exception when that directory is missing.

Wunderbar. So, why haven't I already submitted a PR conveniently solving this for everyone? Because there is no ctx.project_dir. I just made that up, you see. The pythonforandroid.build.Context class doesn't seem to provide the project directory, which leaves that prospective PR with nowhere good to go.

Internal documentation in the python-for-android codebase is so scarce that I can't be sure, though. The project directory must be accessibly stored somewhere, right? That's the most important directory. Everything else is just a footnote to the project directory. It'd be kinda sad (but ultimately unsurprising) if the build context fails to capture the most important build context. The feeling of vertigo is now overwhelming. :face_with_spiral_eyes:

I now beseech someone with greater wisdom than me (...so, basically anyone) to do this for me.

Everything, It Now Makes Sense

There's actually a related open issue. Ironically, that issue was submitted by @dlech – the active maintainer of Bleak, whose recipe prompted both of these issues. Double-ironically, @dlech also identified the wacky short-term workaround for this insane long-standing issue: just replace the ./ substring prefixing a p4a.local_recipes dirname with ../../../../. Since the .buildozer/android/platform/python-for-android subdirectory is nested four levels deep, four levels of "de-nesting" are required to break out of the Buildozer-isolated p4a directory and return back to the top-level project directory. Note the judicious use of adjectives like "wacky" and "insane."

There's also a fascinating Reddit thread on the /r/kivy subreddit debating this very topic. Numerous Reddit users self-report the same issue. Of course, no one knew how to resolve the issue or that there even was an issue. Instead, everyone uselessly flailed around and (presumably) eventually abandoned Kivy because nothing was working. I'll admit I would have done the same in their position.

The fact that p4a.local_recipes and --local_recipes broken also breaks Buildozer's official documentation on the subject as well as downstream projects previously leveraging these things.

Sad cats unite. :joy_cat: :crying_cat_face: :scream_cat:

[misl6]

Thank you for the really detailed report! Will try to give it a deeper look ASAP. Meanwhile, I've checked one of my buildozer.spec file, which I use to package an app at least once a week, which have a p4a.local_recipes = ./p4a-recipes/in it, and seems that "Just Works ™️", so there's probably a condition where a patch is not needed. 🧐

[leycec]

Meanwhile, I've checked one of my buildozer.spec file, which I use to package an app at least once a week, which have a p4a.local_recipes = ./p4a-recipes/ in it, and seems that "Just Works :tm: "...

Excellent. Thanks so much for sinking your incisors into this! I'm both delighted and appalled everything works on your end, though. Deterministic issues are so much simpler to resolve. Could there possibly be a caching issue here where Buildozer previously cached a correctly resolved relative p4a.local_recipes dirname for you that it's now simply reusing? I've noted that it's really hard to get Buildozer to recognize any change to the p4a.local_recipes setting after the initial build. Then again, I've also noted that I have no idea what I'm doing. That should be taken into account.

Relatedly, I just noticed the existing Recipe.get_recipe_dir() method and pythonforandroid.graph.get_recipe_order_and_bootstrap() function. Those probably pertain to all this, too. I'm fascinated by mind-breaking puzzles that destroy your soul. So, I'll do a deeper dive on this from my end and see if I can't fake a working solution that actually works. :smiling_face_with_tear:

[leycec]

Stranger in a strange land... is exactly how I feel. Because you're sorta right, @misl6; after exhaustively injecting info() logging statements throughout both the graph and recipe submodules, I can happily confirm that Buildozer is correctly resolving relative p4a.local_recipes dirnames like ./p4a-recipes/ during the initial build. That's good.

However, I can unhappily confirm that Buildozer is still silently ignoring all of the local recipes in that directory. That's bad. This issue is thus still a thing. I am left scratching my head, which is now openly suppurating with oozing wounds. Mind-breaking puzzle: you continue to haunt me. :ghost:

[leycec]

ZOMG. Buildozer is silently ignoring all of the local recipes in p4a.local_recipes because Buildozer ignores the requirements list in buildozer.spec when constructing its recipe list. Ad-hoc logging I've added to the get_recipe_order_and_bootstrap() function shows that Buildozer is just resolving the default recipe set of sdl2, android, and python3:

[INFO]:    [ZOMG] in get_recipe_order_and_bootstrap()...
[INFO]:    [ZOMG] names (initial): ['python3']
[INFO]:    [ZOMG] names @ bs.recipe_depends: {'sdl2', 'android', 'python3'}
[INFO]:    [ZOMG] names @ fix_deplist: [('sdl2',), ('android',), ('python3',)]
[INFO]:    [ZOMG] names @ blacklist: [('sdl2',), ('android',), ('python3',)]

Instead, Buildozer should be appending recipes for bleak, numpy, scipy, and sympy to that set. I'm confusion incarnate.

Let's debug up the call chain and see why the toolchain.build_dist_from_args() function isn't passing the correct set of initial recipe names to graph.get_recipe_order_and_bootstrap(). Looks like dist.recipes == ['python3'] in build_dist_from_args(). That's... not right. :shrug:

[leycec]

Ugh. Looks like the eventual culprit is the toolchain.dist_from_args() function, which appears to be ignoring the requirements list in buildozer.spec. Specifically, this line:

def dist_from_args(ctx, args):
    """Parses out any distribution-related arguments, and uses them to
    obtain a Distribution class instance for the build.
    """
    return Distribution.get_distribution(
        ctx,
        name=args.dist_name,
        recipes=split_argument_list(args.requirements),  # <-- THIS SO BROKY
        archs=args.arch,
        ndk_api=args.ndk_api,
        force_build=args.force_build,
        require_perfect_match=args.require_perfect_match,
        allow_replace_dist=args.allow_replace_dist)

Ugh x 2. Looks like the real eventual culprit is that args.requirements == ['python3']. So, the requirements list in buildozer.spec is indeed being ignored. B-b-but why would you do this to me, Buildozer? And all I ever wanted to do was actually use you. :face_exhaling:

[leycec]

ZOMGZOMGZOMG. Much to my complete dismay and astonishment, I believe I finally uncovered the ultimate culprit: it's the ToolchainCL.__init__() constructor, which is totally ignoring the requirements list in buildozer.spec. Welp, that was not at all as expected. Since the requirements list in buildozer.spec is the only authoritative source of Buildozer-specific requirements, I'm dead certain beyond a doubt that the ToolchainCL.__init__() constructor absolutely should base its initial recipe list from the requirements list in buildozer.spec.

Instead, that constructor appears to derive its list of initial recipes from two divergent sources:

1. The extremely hacky pythonforandroid.pythonpackage.get_dep_names_of_package() function, which silently attempts (but utterly fails, in our case) to parse an initial recipe list from either:
   • The setuptools-specific dependencies listed in a setup.py file.
   • The PEP 517-compliant dependencies listed in a pyproject.toml file.
2. The less hacky (but mostly useless for our purposes) --requirements option explicitly passed at the command line.

In our case, our proprietary project has a standard setup.py file – but proprietary, so I can't exactly repost its contents here without violating intellectual property clauses. But nothing of that matters, because...

THIS IS ALL WRONG, BRO

This is all wrong, @misl6. I :heart_eyes: everything else you've done with the Kivy ecosystem, but this is... not right. The pythonforandroid.pythonpackage submodule itself admits this is not right:

    Yes, this module doesn't fit well into python-for-android, but this
    functionality isn't available ANYWHERE ELSE, and upstream (pip, ...)
    currently has no interest in taking this over, so it has no other place
    to go.
    (Unless someone reading this puts it into yet another packaging lib)

It's not just that the pythonforandroid.pythonpackage submodule "doesn't fit well" into python-for-android. It's that ad-hoc (and clearly broken) parsing of packaging file formats is not in Buildozer's wheelhouse. That isn't something Buildozer should ever attempt to do, because it's infeasible to do that correctly in practice, because Buildozer is not pip. Right?

Even setuptools itself has abandoned all pretence of doing that. You can no longer use setuptools to install anything directly, because not even setuptools trusts itself to install anything directly. Notably, directly running a setup.py script now results in a deprecation warning resembling:

SetuptoolsDeprecationWarning: setup.py install is deprecated. Use build and pip and other standards-based tools.

That's how hard packaging in Python is. Python packaging is hollowed ground. You don't trespass on hollowed ground without good reason. Buildozer doesn't have that reason, because Buildozer already has the requirements list in buildozer.spec. That's exactly what Buildozer should be passing to python-for-android – possibly via the aforementioned --requirements option.

Let Me Help You

I package for Gentoo Linux. I package for conda-forge. I package for pip. I'm conversant in PEP 517. In short, I package. I package many things. Let me help you fix this. I'm "happy" (...for certain unspecified definitions of that adjective) to do the grunt work here, but I'll need more than a little architectural assistance first. In my humble opinion...

Let's Stop the Parsing Madness

Buildozer should never attempt to parse setup.py or pyproject.toml files. The entire pythonforandroid.pythonpackage submodule should go away and never come back. :axe: :drop_of_blood:

I note the use of an upstream pep517 dependency here that's probably performing most of this parsing for you. That's great, except that dependency isn't actively maintained. It's last commit was six months ago. There are also a sprawling number of open issues, including test failures and deprecation warnings under Python 3.11, which is slated for rapid release in just a few months.

If you'd really like to continue using package dependencies (which you shouldn't), Buildozer should instead defer to the core importlib.metadata API standardized by PEP 566. It's available under Python ≥ 3.8, it works everywhere and well, it doesn't parse setup.py or pyproject.toml (which is why it works everywhere and well), and there's an official backport to Python 3.7. Python 3.6 has long since hit its End-of-Life (EOL) and is thus a security risk, so nobody cares about Python < 3.7 anymore, right?

The only snag there is that importlib.metadata requires the package you're inspecting to be installed first – but that shouldn't be a concern here. Buildozer already installs packages into a virtualenv it manages.

In short: pythonforandroid.pythonpackage → importlib.metadata is the way, if you really must.

Let's Start Using buildozer.spec

buildozer.spec exists. The requirements list in buildozer.spec is the authoritative list of Buildozer-specific dependencies, so let's start using that list.

[leycec]

ZOMGZOMGZOMG. I can't believe it, but the real, real issue here is that Buildozer silently ignores requirements containing the >= operator. I... I don't even. Specifically:

# If I replace this line in "buildozer.spec"...
requirements = python3>=3.9.0,kivy>=2.1.0,kivymd>=0.104.2,beartype>=0.10.0,bleak>=0.14.0,numpy>=1.22.0,scipy>=1.7.0,sympy>=1.9.0

# ...with this line substituting the ">=" for "==" operator.
requirements = python3==3.9.0,kivy==2.1.0,kivymd==0.104.2,beartype==0.10.0,bleak==0.14.0,numpy==1.22.0,scipy==1.7.0,sympy==1.9.0

...then Buildozer suddenly begins accepting and using those requirements as recipes.

So, let's just ignore everything above about setup.py, pyproject.toml, and PEP 517... because Buildozer clearly has larger problems than that.

This Is Really Not Right

During app development and testing, I definitely do not want to pin exact dependency versions. During app release and version bumps? Sure. I suppose, but probably not even then. We intend our app to portably work with a reasonable range of dependency versions, because the alternative is unmaintainable madness. It helps that our dependencies avoid breaking backward compatibility – or at least explicitly warn us with obvious deprecations when they intend to do so in a future release.

The only time I've ever really wanted to pin exact dependencies was in the web dev space, because web dev is fundamentally broken by design. That's the nature of the beast. So you limit the breakage as best you can with pinning, semantic versioning (SemVer), and the ^ operator everywhere. But most Python projects don't SemVer – and even if they did, Buildozer doesn't grok ^, so we'd still be in the same unpleasant conundrum here.

This Can Be Made Right

In short, I'd like to:

1. Add support for at least the >= operator in requirements lists to Buildozer and/or python-for-android.
2. Explicitly raise exceptions for malformed requirements lists. Buildozer should never silently ignore a requirements list that it considers to be syntactically invalid. Obviously, a parsing error of some sort should be raised instead.

Does this erratic gameplan seem reasonable, @misl6? If so, I'll submit a series of well-tested PRs – first addressing 2. (because the user should always be informed when Buildozer disagrees with something) and then addressing 1.. Let's do this, Team Kivy! :fist_raised:

[leycec]

Fascinating... and horrifying! Buildozer is correctly sending the --requirements=python3>=3.9.0,kivy>=2.1.0,kivymd>=0.104.2,beartype>=0.10.0,bleak>=0.14.0,numpy>=1.22.0,scipy>=1.7.0,sympy>=1.9.0 list to python-for-android. It's actually python-for-android that's silently reducing that list to just --requirements=python3.

That simplifies matters a lot. Good news descends from the heavens like mana. We'll only need to submit PRs against python-for-android; Buildozer itself is doing the Right Thing :tm: . In hindsight, that means this issue should have been opened up against python-for-android rather than Buildozer. Wut you gonna do?

[leycec]

Oh... oh by Gods. Could it be? N-n-no. It couldn't possibly... But it could.

> is the Bourne shell output stream redirection operator. If Buildozer is simply naively shipping an unquoted option value like --requirements=python3>=3.9.0,kivy>=2.1.0,kivymd>=0.104.2,beartype>=0.10.0,bleak>=0.14.0,numpy>=1.22.0,scipy>=1.7.0,sympy>=1.9.0 to python-for-android, that would explain why python-for-android only sees the prefix --requirements=python3 of that list. The shell strips off the rest of that option and then silently redirects everything else into useless files with nonsensical basenames like =3.90,kivy and =1.7.0,sympy in the .buildozer/.android/platform/python-for-android/ subdirectory.

OH MY LIVING GODS BELOW. That's exactly what Buildozer is doing:

$ ls .buildozer/android/platform/python-for-android/pythonforandroid
 .         pythonforandroid    '=0.14.0,kivy'   '=3.9.0,bleak'      Dockerfile                Makefile
 ..        testapps            '=0.14.0,numpy'  '=3.9.0,kivy'       .dockerignore             MANIFEST.in
 ci        tests               '=1.22.0,scipy'   CHANGELOG.md       .env                      .projectile
 doc      '=0.10.0,bleak'      '=1.7.0,sympy'    .coveragerc        .gitignore                README.md
 .git     '=0.10.0,numpy'      '=1.9.0'          .deepsource.toml   ionyou-debug-0.0.1-.apk   setup.py
 .github  '=0.104.2,beartype'  '=2.1.0,kivymd'   distribute.sh      LICENSE                   tox.ini

I'm sitting stunned here – mouth agape like Soyboy Wojak. I cannot believe my cheating eyes. How is this possible, @misl6? How is it possible that a mission-critical cross-compiling toolchain with over 300 financial backers and 20 corporate supporters in 2022 is naively running shell commands without quote-protecting shell arguments?

The Sky Is Officially Falling

This is a serious security risk, bro. No. Seriously. Full-stop. This is injection attack territory.

Buildozer is executing arbitrary strings in buildozer.spec files as shell code with user-level privileges. Let's play a dangerous game of hypotheticals. What happens when a malicious blackhat embeds the following requirements list at the very bottom of an otherwise innocuous buildozer.spec file and then seductively entices one or more other hapless victims into executing Buildozer against that file?

# *BOOM.* Your root partition just imploded.
requirements=python3;rm -rf /

# *BOOM.* Your data center just DoSed itself as well as exhausting all disk space.
requirements=python3;a='/*/*/*/*/../../../../*/*/*/*/../../../../*/*/*/*' sh -c ': ${a=foo}' >/uhoh

Buildozer is now in imminent danger of having a CVE posted against it. The severity of this issue cannot be overstated. If I'm reading the codebase right (...I'm probably not), the buildozer.targets.android.TargetAndroid.compile_platform() method is the red-headed Irish stepchild here:

    def compile_platform(self):
        ...
        p4a_create = "create --dist_name={} --bootstrap={} --requirements={} ".format(dist_name, self._p4a_bootstrap, requirements)

:facepalm: :man_facepalming: :woman_facepalming:

Never interpolate raw arbitrary strings directly into shell commands. Please. Gods both above and below. Please stop doing that before the Internet fully implodes into itself. Strings deriving from external sources (including buildozer.spec files) must be assumed to be insecure. Insecure strings must be quote-protected. There are two ways to effect this in Python. There's the good way... and then there's the bad way, which is what Buildozer is going to have to do, because we're on the worst historical timeline:

• Good way. The good way is to pass subprocess functions the shell=False keyword argument along with a list of command arguments. This is good, because (A) it's inherently portable and (B) it's inherently safe, because Python implicitly quote-protects all of those command arguments for you. Unfortunately, you can't do this here. Why? Because the buildozer.__init__.Buildozer.cmd() method explicitly passes shell=True. This is why I sigh repeatedly to myself.
• Bad way. The bad way is to manually quote-protect every single command argument everywhere in the Buildozer codebase with the standard shlex.quote() function before you interpolate those arguments into a string to be subsequently run. This is a brutal undertaking, but Buildozer's pinned itself into a corner by setting shell=True and running interpolated string commands rather than running lists of uninterpolated strings: e.g.,

from shlex import quote
...

    def compile_platform(self):
        ...
        p4a_create = "create --dist_name={} --bootstrap={} --requirements={} ".format(
            quote(dist_name),
            quote(self._p4a_bootstrap),
            quote(requirements),
        )

The road to Hell is paved with manual shell command interpolation.

tl;dr

Buildozer is fundamentally insecure against shell injection attacks. Resolving CVEs is way outside my wheelhouse as a volunteer minion, so I'm just going to quietly show myself the door while devoutly praying to every northern Canadian mosquito swamp deity I know that someone who is not me fixes everything before the corporate donors justifiably storm the gates.

[tshirtman]

First, thanks a lot for the investigation. Indeed security was certainly not a major concern during the design of buildozer, as it’s usually ran manually by the user wishing to build their application, while that’s certainly no excuse, as seemingly unexploitable bugs can always find their use in chains later as people rely on software and building more complex solutions on it without understanding their security characteristics.

Also, it’s a stupid and dangerous bug no matter how you see it, and it should be solved, no argument.

I don’t remember from the top of my head why we pass shell=True, but if possible at all, we should indeed close that gap first. Other than that, yeah, quoting every parameters would be the way, although it’s indeed harder to make sure you didn’t let any slip through.

This should get priority in solving.

[misl6]

Even if I'm not too concerned about the security issue found during the discussion, I'm taking this occasion to refresh some code.

The weekend plan was to work on a python-for-android WIP PR, but his one has the priority ATM, so something will land (hopefully) soon.

Why I'm not concerned:

There are plenty of ways (even simpler ones than the one discussed here) of injecting malicious shell commands into the developer shell (which I'm not going to discuss here, for obvious reasons) when using buildozer (and is not Kivy's fault). A developer should trust the author of e.g. a buildozer.spec example, before using it, or at least should read the most essential parts, unless the code is running on a sandboxed environment.

Why even if I'm not concerned I'm working on it:

Also, it’s a stupid and dangerous bug no matter how you see it, and it should be solved, no argument.

Agree, and that's an excuse to refresh some code.

What we should do in the future:

Set up a Security Policy so we can talk about this kind of issues privately (and later share info with the community about security bugs when are already fixed (or at least triaged), and not exploitable anymore). [ Was on my low-priority task list ]