Im talking about the service which handles run conditions and the rest of the Android specific logic which is not tied to the UI itself.
It’s essentially intertwined with the ui code, as run conditions and the ui is in the same android code, not syncthing/go code.
That’s what i’m talking about. Creating an Android library repo governed by the syncthing project with said code. The benefit would be that all platform related issues(run conditions, starting the binary, powersaving quirks etc) can be handled in one place and the apps just use it and slap their UI on it.
Currently all such issues are handled in one place - the app/wrapper/GUI, which is one and the same thing. You want us to break away the GUI, keeping the rest in a library that others can “slap a GUI on”. Why? The hard engineering that would need to go into the library is the same as today, and the limitations in terms of contributions and releases the same. Is there an advantage in having random people slap GUIs on our work, getting a proliferation of different looking syncthing apps all providing identical functionality? I don’t see the problem this solves, at all.
4 Likes
So long story short, Syncthing-Fork for Android is just a hobby project with a different strategy what gets in and how quality assurance is done. It will never mine bitcoins, if you’re in doubt, use the Fdroid release.
“Mining bitcoins” must not be taken verbatim, you surely are aware of that - but maybe we distictively could and should take it verbatim?
Synthing is about valuable data, privacy, security and stuff. Now imagine some innocent (not malicious!!) code change that rips open a major security hole in the app undetected. This has happened way too often in many, many other projects, open and closed source. So this will open the doors wide for some malicious attacker that finds out before the good guys do and fix it, a classic zero day. And despite fdroid confirming the app contains no evil code they of course won’t be able to confirm the code is secure and cannot be easily penetrated.
The only thing that we could do to keep the risk for security breaches low(!) is to do proper reviews and thus IMHO we should not slaken on those reviews. And in fact this also includes seemingly minor issues like the consistent, self-expaining and typo-free naming of variables. Even this adds (or subtracts) to the quality of the code base. Imagine, projects like KDE even have rules that dictate the usage of tabs vs. spaces and that do ban trailing whitespace. And they have reason to do so. Sloppy handling of whitespace can negatively impact readability and could be a first faint hint on some missing painstakingness of a developer also in other aspects.
I know what I’m talking about, I’m in professional C++ development for over 25 years and this made me see lots of crappy and inferior code. Doing reviews is one of my (many) daily businesses, you get used to it and demanding improvements is not molesting the developers, it’s about commitment to the code base and quality. Reviews have a proven track of increasing the code quality, we should not let this chance pass up without reason.
Don’t get me wrong. I can understand your position, “it’s just a hobby fork made mainly for my personal needs and therefor it suffices” very well. Nevertheless I strongly second doing proper code reviews when giving an app to the public.
1 Like
If someone likes to apply to the fork repository and take the security reviewer role I’d appreciate that. As I’ve nothing to do with the core code that sets the level of security as it handles the syncing and networking stuff, I suspect the wrapper’s responsibility is a bit lower. It has some attacker doors open for example regarding receivers and intents, I’m no expert on this but also found those security relevant glitches in official and the fork may also have some. Just trying to do my best at being cautious.
2 Likes
Hey, just saw https has been disabled in the official app since 0.10.18. (https://github.com/syncthing/syncthing-android/commit/1f82ec0fc6096d6694ae75ccaca5afcf09088581 ) Come on, instead of arguing about reviews merge Syncthing Forks solution which keeps https REST connection between the app and native if security is important.
1 Like
Yeah, that just seems like a horrible decision, one that actually introduces a security vulnerability. Now any app can just start listening on port 8384 and wait for the Syncthing app to start to get the API key. Then it can let Syncthing start, and use the API key to make any changes it wants, like add new devices, which would let it read and write data to all Syncthing folders.
For reference, syncthing-fork disables TLS on old Android versions, but keeps it enabled on supported versions. That is not ideal because it still leaves some devices vulnerable, but much better than disabling TLS completely.
Sorry @AudriusButkevicius I was giving you the benefit of the doubt, but now I really feel like the app would be in better hands with @Catfriend1 as maintainer.
Edit: The problem is not mainly about making a change to disable TLS. The problem is with arguing that code reviews are important for security, and then quietly merging and releasing something that obviously makes security worse, without any review at all.
How exactly was TLS preventing an app starting on the same port and getting the API key before?
There was no certificate validation, so this change does not make the security any worse.
If we want to rave and scream just for the sake to make me look bad, without actually understanding implications of the change, go ahead I couldn’t care less.
I have no interest in maintaining the app, nor did I want to spend my evening removing TLS and fixing builds.
Yes, it is hipocrate of me to merge my own changes and then demand reviews of others, but as we all agreed there are no maintainers, so I wouldn’t expect reviews.
I don’t want to spend any more than the 40mins I had on this (ideally I would not spend any time on this at all).
If you are so fussed about it, let me revert it and let’s just tell people to fuck off.
Also, my intentions to remove TLS were loud and clear in the ticket some time before the actual, so you could have objected that earlier. Now it seems you just came around when the change was done just for the sake of pouring dirt.
@Nutomic: I’ve just looked at the SyncthingTrustManager class which I hadn’t to fiddle before. Is that class there to validate Syncthings local https certificate?
I’ve reverted my change, and in progress of pushing out beta2. I did fuck up here, which probably means I should stay away from android all together and just let shit rot.
Hey guys, I was here to just thank you all for all your hard work and this great community, but as I see there are a lot of masculine discussions in the background o_O I hope you all get together and keep working on this great community which you have build. I’m using the syncthing on my android phone and pc and suggested it to all my family and friends ^^
You are creating a beautiful, easy to use and secure possiblity to synchronise everything for all the world.
Thank you all for your hard work.
Best wishes from germany (excuse my bad english, please 
) Christina.
8 Likes
Good that we agree here. Can we please make @Catfriend1 maintainer for the Android app then?
@Catfriend1 Yes, it verifies that the certificate sent by the Syncthing API matches the certificate file in the config folder. Precisely to prevent the attack of another app binding on port 8384 and getting the API key.
I’ve sent invitations / tweaked permissions. Let’s look at the new permanent structure later.
1 Like
Who is in charge of the GooglePlay account?
Thanks for the invitation. Before starting the work on the official I’d like to write up a suggestion how we could do merge and release management. I’ve already had interesting chats about this with @capi and @imsodin, got contacted yesterday by @Zillode. And of course, I’d like to do better in working together with Audrius during reviews. What I would intend to bring up first is the fix for the TLS problem. (Thanks @Nutomic for shedding light on this) From my research, is it okay to force https on all Android OS except 4.4 and 7.0? Another dev told me that those Android’s miss some elliptic curves to do tls 1.2 with strong ciphers.
From my research, is it okay to force https on all Android OS except 4.4 and 7.0? Another dev told me that those Android’s miss some elliptic curves to do tls 1.2 with strong ciphers.
Just my 50 cents on this, may be invalid since I don’t know the context of this, but: TLS 1.2 has a lot of secure cipher suites which do not require any elliptic curves. In the web ECDH/ECDSA is common, but standard Diffie-Hellman with RSA has a very broad compatiblity too. I’ve no idea if you use elliptic curves in the certificates or so, but not using TLS 1.2 on an Android 7 platform seems like a really bad idea.
1 Like