Harden proxy ping to avoid shell injection - #1110
Conversation
Refactor ping command to use subprocess for better error handling and readability.
| ['ping', '-c', '1', '-W', '1', '--', hostname], | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL | ||
| ).returncode |
There was a problem hiding this comment.
returncode was used to get the exitcode to follow up with the next conditions. is there anything i have to fix? or you want me to intialize it like this?
var = subprocess.run(['ping', '-c', '1', '-W', '1', '--', hostname],
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL)
response = var.returncode
if it returns 0 that means it finished sucessfully running it, if its 1 or any value other than 0 then it means it didnt execute properly. --> (ping)
or i can try doing it without returncode in the first place like this:-
( this one completely removes the if condition and response since try and except directly handles it )
if hostname:
non_blank_host_name_counter += 1
try:
subprocess.run(
['ping', '-c', '1', '-W', '1', '--', hostname],
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
check=True,
)
except (OSError, subprocess.CalledProcessError):
self._proxy_inline_alerts[schema].show()
response_to_return = False
tried my best to give indentation and spacing for better understanding for you.
let me know which one you prefer.
There was a problem hiding this comment.
honestly i prefer this one:
if hostname:
non_blank_host_name_counter += 1
try:
subprocess.run(
['ping', '-c', '1', '-W', '1', '--', hostname],
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
check=True,
)
except (OSError, subprocess.CalledProcessError):
self._proxy_inline_alerts[schema].show()
response_to_return = False
There was a problem hiding this comment.
Updated the file to use check=True instead of return code for error handling.
|
You have a commit message "flask8 fixes", I'm assuming you meant flake8 so correct the commit message. |
|
yea i will correct the commit message as soon as possible and let you know. |
|
I don't mind if this is fixed, but be realistic - the Terminal activity is a much easier way to get a shell, and we can recommend that instead of this unexpected feature. 😀 |
|
thats true @quozl 🤣 |
but the "users" might get mislead by any youtube channel on running any command which can cause damage. |
|
yes, but we can't ban youtube. another way is to provide a web server that offers a zip file with content type application/vnd.olpc-sugar which browse will unpack and install. getting someone to click on a link is much easier than getting them to waste time on youtube. |
ur right there are easier ways like the one u mentioned. my intension for this PR to get rid of this avoidable shell injection by using subprocess.run. i will update the PR based on the comments ( usiing check=True instead of relying on returncode) and commit new changes. |
|
a doubt like we can add user confirmations before installing anything right? (the malicious installation u told me abt) i actually went and checked the code, and rn theres literally no confirmation for a fresh install. journalactivity.py listens for new journal entries and just calls handle_bundle_installation() straight away, which calls registry.install() with no prompt at all. only time it asks anything is if a downgrade is needed (theres already a ConfirmationAlert used for that in misc.py). so my idea is to reuse that same alert pattern for first time installs too, not just downgrades, so the user atleast sees a "do u want to install this activity?" popup before it happens. would like to work on this as a follow up PR if thats cool, lmk what u guys think. |
|
other operating systems have resolved this with code signing. |
that makes sense code signing would def be the stronger fix long term compared to just a popup. honestly im down to look into that too if yall think its worth it just lmk the direction u want ( confirmation dialog as a smaller step first or straight up trying to figure out signing ) and ill start looking into it |
|
hello @chimosky whnever u get a chance can u take a look again? i have updated the code using check = True rather than relying on .returncode lmk if anything else needs changing. |
|
it would make the activity library smaller again. it was made much smaller when we ported to python 3. if we went for signing of activities, it would have to start from zero again. it's not the digital signatures that are a problem, it's the maintenance and release of activities that is not happening much. without that happening it's hard to see how signing would help. also, it's not the sort of problem that would be solved by ci/cd, tempting though that may seem. we lack maintainers, and the security assurance that they bring to development. |
then i guess the confirmation dialog is probably the more realistic thing to actually ship now? |
|
it would be a step in the right direction, as far as a security policy is concerned. i'm unfamiliar with your work, so don't know if you're aware of the extent of the challenge to just get one line of code in the browse activity to change, so here's a brief summary;
also, while i've assumed that a confirmation dialog in browse will handle the situation, you may have been thinking of a dialog in the sugar shell, jarabe. if so, the other test cases to consider are filesystem access by the embedded journal activity, and file sharing via collaboration stack. good luck. |
|
hey quozl u mentioned (embedded journal access + collab stack), and also found another gap with Activities being watched live while sugar's running (the terminal case). think i found exactly where each one needs to change to close them properly. does this scope seem right before i start building it out as a real follow up PR? |
|
sorry, i don't understand what you mean by the other gap. |
|
@quozl to clarify what i meant by the "other gap" i was talking about a potential security bypass in the terminal activity. right now, a user can just use wget to download an untrusted file directly into the sugar environment and it happens completely silently without any UI confirmation or warning. (i attached a screenshot showing a .xo file being downloaded this way) also, on a separate note regarding GTK4 since i know there is an ongoing effort to port the environment who is the best person for me to connect with i just want to make sure my current work doesn't cause major merge conflicts or create extra headaches for whoever is handling the migration later on. |
|
|
oh, okay, that kind of change is best implemented in bash or the operating system, not in terminal. or remove the terminal activity. check the sugar-toolkit-gtk4 repository for activity on gtk4. jukebox has the most recent pull requests i've seen. |
|
@quozl a quick doubt should the popup only occur when the user have downloaded anything from the browser or like only.xo files? |
|
don't know, sorry. it is a GET of the content-type application/vnd.olpc-sugar that is the critical link in the social engineering vulnerability. |
|
@quozl while actually editing and testing right i found this on the shell where i was not able to access the terminal. i went and checked on terminal-activity as well but i was not able to find where it was to raise a PR over there. so the thing is that it uses readfp at this line which breaks the code since fedora uses python 3.14 at the moment. (like the newer fedora linux) this exact place:- and this is the error i got like it showed me also the logs:- after using read_file instead of readfp it actually worked. |
|
is this wantedly done? or u can actually do like this right? |
|
the version also i checked and it was 47 |
|
The terminal-activity lives in our GH org, where did you check that you couldn't raise a PR? |
i went to terminal.py over there and still i cant find that exact line thats what i meant by checked. like the same function which shows theres a error:- def _configure_vt doesn't exists inside terminal.py. |
The change you're referring to has already been made in 451fd6d, but a release hasn't been made yet. |
|
apart from please don't use screenshots when you can copy and paste text, and that i agree with @chimosky, i've nothing else to add. thanks for sharing. |
Oh ok Alright I understood. Thanks. |
Ok I will copy and paste texts from now. |
|
hey @quozl, wanted to check in with a few open questions before i clean this up for review.
|
|
I don't think dropping a folder into ~/Activities should be refused, and I do think deleting data is an inappropriate response. Journal activity can show an external storage device, and it is one way a teacher is instructed to load specific activities without a network. Where signing is to be adopted, that path must also be considered. |
3 participants
The proxy configuration flow concatenates user-provided hostnames directly into an os.system call. This creates a local shell injection vector.
This replaces os.system with subprocess.run and an argument list to bypass the shell entirely, eliminating the injection risk. It also adds '--' before the hostname argument to prevent flag injection in case a malformed hostname starts with a hyphen.
Manual Test Plan:
Verified locally by entering '127.0.0.1; touch ~/ping_test' in the proxy settings. On the old code, the file is created. With this patch, the ping safely fails to resolve the malformed host, bypassing the shell, and no file is created.
Took inspiration from:- sugarlabs/sugar-ai#136