Avoid the usage of shell=True in Popen, which could lead to potential security risks.
#1435
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tshirtman
previously approved these changes
Apr 30, 2022
buildozer/targets/android.py
Outdated
| @@ -569,7 +564,7 @@ def _android_update_sdk(self, *sdkmanager_commands): | |||
| sdkmanager_path = self.sdkmanager_path | |||
| sdk_root = f"--sdk_root={android_sdk_dir}" | |||
| command = f"{yes_command} | {sdkmanager_path} {sdk_root} --licenses" | |||
| self.buildozer.cmd(command, cwd=self.android_sdk_dir) | |||
| self.buildozer.cmd(command, cwd=self.android_sdk_dir, shell=True) | |||
There was a problem hiding this comment.
i believe this one could be replaced using pipes to capture stdout and inject it in stdin of sdkmanager, but indeed our abstraction probably doesn't make that convenient.
There was a problem hiding this comment.
Yeah, that was a quick fix in order to see the CI running. I expect to remove it before the merge.
We have another shell=True on iOS, that needs some attention, but doesn't make use of anything that is injectable by the user via buildozer.spec, so is low-prio.
There was a problem hiding this comment.
Changed this one, but kept the one on iOS.
misl6
changed the title
shell=True in Popen, which could lead to potential security risks.
|
🏁 I think is ready for a review. |
misl6
force-pushed
the
fix/security-issue
branch
from
0a31e74 to
4a14335
Compare
misl6
force-pushed
the
fix/security-issue
branch
from
4a14335 to
63909c8
Compare
misl6
force-pushed
the
fix/security-issue
branch
from
63909c8 to
f85a901
Compare
|
@tshirtman since you started a review on it, do you see anything that could lock us to merge it? |
tshirtman
approved these changes
Jul 24, 2022
Craulback
added a commit
to Craulback/buildozer
that referenced
this pull request
Sep 27, 2022
* Add support --res_xml option in p4a * res_xml paths are relative to buildozer.spec * Changes for NDK23 (kivy#1427) The arch no longer used in url for NDK23+ * Our self-hosted Apple Silicon runner now has been migrated to actions/runner v2.292.0 which now supports arm64 natively (kivy#1438) * use p4a --add-source instead of manual copy (kivy#1450) Currently, android.add_src does not work anymore. Using --add-source from p4a make it work again. * fix aar build (kivy#1444) * fix aar build * update default.spec to include 'debug_artifact' * Updates default buildozer.spec NDK from 19b to 23b (kivy#1462) * Update CHANGELOG and prepare release 1.4.0 (kivy#1463) * Bump version to 1.4.1.dev0 (kivy#1464) * p4a prerequisites install should be done in non-interactive mode during CI builds. (kivy#1465) * Avoid the usage of `shell=True` in Popen, which could lead to potential security risks. (kivy#1435) * Removes (where possible) the usage of shell=True in Popen * Fixes (needs check on runtime) adb + add a solution for p4a.extra_args * Change the logic to auto accept the SDK licenses, to avoid the shell=True usage * Removes six dependency in tests (kivy#1475) * Fixes some E275 - assert is a keyword. (kivy#1495) * Fix presplash color (kivy#1497) Buildozer wont build if a presplash color is set, this should fix it as descrived in kivy#1487 * Show output during aab support check, as p4a may require the user input (kivy#1494) * Update installation.rst (kivy#1500) * Update installation.rst Update jdk and Ubuntu versions. Update WSL usage instructions. * Update installation.rst * Update docs/source/installation.rst Co-authored-by: Mirko Galimberti <[email protected]> * Update docs/source/installation.rst Co-authored-by: Mirko Galimberti <[email protected]> Co-authored-by: Mirko Galimberti <[email protected]> Co-authored-by: Eero af Heurlin <[email protected]> Co-authored-by: HyTurtle <[email protected]> Co-authored-by: Mirko Galimberti <[email protected]> Co-authored-by: Mathieu Virbel <[email protected]> Co-authored-by: Mikhail Zakharov <[email protected]> Co-authored-by: Akshay Arora <[email protected]> Co-authored-by: Apacelus <[email protected]> Co-authored-by: RobertF <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See full story here:
#1433 (comment)
#1433 (comment)
#1433 (comment)