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

Fix sending wrongly encoded commands into the unix subshell (fixes #1198) #1199

Merged
merged 8 commits into from Jul 27, 2018

Conversation

Projects
None yet
2 participants
@Catfriend1
Copy link
Contributor

Catfriend1 commented Jul 27, 2018

EDIT:
Purpose
This PR provides a fix for issue #1198 . The problem is that the "canWriteToFolder" check fails when the folder path to test contains cyrillic characters. Doing it from java would make the test inaccurate, so no help by implementing more code. Root cause of the problem: Wrongly encoded commands get sent into the unix subshell. Resolved by using BufferedWriter as it's already doing the proper encoding work for the subshell correctly and is a built-In java class.

Testing
Verified working on device lg-h815 running Android 7.1.2 at commit 7541fa9 on base of release tag 0.10.12

Screenshots
a) Russian folder name gets accepted by the shell and folder permission check in "canWriteToFolder" return the correct result (in this case on my phone read-writeable).
image
b) Log output
image

@Catfriend1 Catfriend1 added the bug label Jul 27, 2018

@Catfriend1 Catfriend1 added this to the 0.10.13 milestone Jul 27, 2018

@Catfriend1 Catfriend1 self-assigned this Jul 27, 2018

@Catfriend1 Catfriend1 requested a review from AudriusButkevicius Jul 27, 2018

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 27, 2018

This is a poor solution. There are plenty of other encodings that will have the same problem, Chinese, Japanese, Arabic, Greek etc.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 27, 2018

( This is a poor solution. ) It is. Please merge it to help users out first ( to prevent user loss ) and I'll keep that on my todo list as we keep the original issue open. I'll do more research on it and propose a better solution as soon as I found one. Thanks for your understanding.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 27, 2018

No, sorry, but we either fix it properly or we don't, as we'll soon have another poor solution for a different encoding.

If you want to make a "hotfix" for this, then disable the checking all together until there is a better way.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 27, 2018

I'll commit a change disabling this check in the meanwhile, so I could make a release.
Feel free to open a PR fixing this in a more sensible way.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 27, 2018

Yes, I'm on it.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 27, 2018

@AudriusButkevicius
I'd like to continue improving this PR from workaround to proper fix. It's my work and I don't need to do it a second time. > Reopened, explanation on the forum.

@Catfriend1 Catfriend1 reopened this Jul 27, 2018

@Catfriend1 Catfriend1 changed the title Workaround to fail permission detection gracefully on paths with cyrillic characters WIP - Fix sending wrongly encoded commands into the unix subshell (fixes #1198) Jul 27, 2018

@Catfriend1 Catfriend1 removed the request for review from AudriusButkevicius Jul 27, 2018

@Catfriend1 Catfriend1 modified the milestones: 0.10.13, 0.10.14 Jul 27, 2018

Catfriend1 added some commits Jul 27, 2018

Revert "Remove unused import"
This reverts commit 7541fa9.

@Catfriend1 Catfriend1 changed the title WIP - Fix sending wrongly encoded commands into the unix subshell (fixes #1198) Fix sending wrongly encoded commands into the unix subshell (fixes #1198) Jul 27, 2018

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 27, 2018

This PR should probably revert my changes.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 27, 2018

To be honest, we should revert this whole subshell nonsense, as there is no reasonable escaping.
User can create a folder with name foo; rm -rf /; echo which will then be interpreted as:
rm foo; rm -rf /; echo /.stwritetest

@Catfriend1 Catfriend1 merged commit 5426e75 into syncthing:master Jul 27, 2018

2 checks passed

Build (Syncthing Android) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Catfriend1 Catfriend1 deleted the Catfriend1:fixIssue1198-Encoding branch Jul 27, 2018

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 27, 2018

As I said, I think this whole "user input into a subshell" should be backed out.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 27, 2018

@AudriusButkevicius I disagree. User cannot enter paths manually causing this as the canWrite... function is only called with parameter content that came from android's folder picker returning VALID paths.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 27, 2018

And what's preventing me from creating a directory with that name?

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 28, 2018

You can do so, but that is not suspect to this PR as it's expecting to have a "security team" that regulates this. It's totally clear to me I can offer you a text filter, e.g. RegEx to prevent this one case, but then someone could make up another, probably weird non-daily use case, security infringement use. Anyway that's not the main topic we must care about NOW. The user can also open a terminal emulator and enter the bad command by hand. He doesn't need the syncthing folder creation to do this as both have the same unix permissions on android.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 28, 2018

Sure, but a user can be tricked into accepting a folder offered by a remote peer that has the folder name with shell commands in it. I've posted a link on the forum for a utility function which should help work around this.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 28, 2018

Agree to your forum post. Disagree with the folder share scenario, we hand in the path from the picker not the announced folder label.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

AudriusButkevicius commented Jul 28, 2018

Sure. Syncthing prepopulates the path, perhaps android doesn't, hence this is not directly a vulnerability, but it's still an attack vector. There is nothing worse than to have a remote execution attack vector in a piece of software that claims to be secure, encrypted and all about privacy.

@Catfriend1

This comment has been minimized.

Copy link
Contributor Author

Catfriend1 commented Jul 28, 2018

What about the license of StringEscapeUtils.java, its under apache license. Is this "compatible" and allowed? I think it's now your turn to escape the following I found out that would makes sense:
shellEscape(absoluteFolderPath) and then verifies it doesn't break anything else with the encoding that just got fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment