Page MenuHomeWildfire Games

Add an option to allow players to circumvent TLS bugs
ClosedPublic

Authored by elexis on Nov 24 2018, 11:14 AM.

Details

Summary
  • Add the ability to disable TLS encryption (typically in order to circumvent #5349).
  • Add the ability to enable TLS cert verification (in order to have full TLS security whenever users get a patched gloox from their system packages)
  • Remove security claims from the Privacy Policy.
Test Plan

@bb can reproduce #5349 and will be able to test this option removes the crash.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6469
Build 10710: Vulcan BuildJenkins
Build 10709: arc lint + arc unit

Event Timeline

Itms created this revision.Nov 24 2018, 11:14 AM
Vulcan added a subscriber: Vulcan.Nov 24 2018, 11:16 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/796/

Itms marked an inline comment as done.Nov 24 2018, 11:16 AM
Itms added inline comments.
binaries/data/mods/public/gui/prelobby/common/terms/Privacy_Policy.txt
42

Alternatively, one could replace is by can be, which would create extra translation work for little benefit, and would be dishonest considering TLS encryption is not fully implemented as long as certificate verification is broken.

As mentioned in https://trac.wildfiregames.com/ticket/5349#comment:4 the question is whether gloox::TLSOptional should not rather become gloox::TLSDisabled in XmppClient.cpp if it crashes when trying the handshake.

I installed Fedora 29 on virtualbox yesterday, but I didn't succeed to compile yet (out of memory errors and bla).

(It seems gloox/TLS is so broken that I wonder if it's not better to disable TLS entirely and only have the interested people play with it.
But then again we wouldn't get feedback like the one from Fedora 29, so maybe it's a better compromise to add this option.)

binaries/data/mods/public/gui/options/options.json
382

-Enable (all booleans options enable)
-Experimental (side effects can be mentioned in the tooltip)

388

-Enable Experimental

389

As certificate verification is broken in gloox, it doesn't work on any platform currently, so not sure if this adds value other than the hypothetical use case where gloox is fixed upstream.

(For #5344 eventually, it should probably be message boxes asking for confirmation whether to proceed with an illegal or not certificate if there isn't a certificate provided, rather than a config file, but I guess that's irrelevant for now)

(entirely secure -> security seems binary, either it is or isn't)
(Perhaps as the second sentence: This ensures that the lobby certificate stems from the lobby server, not an attacker and that the certificate is active and not expired yet.)

binaries/data/mods/public/gui/prelobby/common/terms/Privacy_Policy.txt
42

ack

bb added a comment.Nov 24 2018, 3:08 PM

Testing on fedora 28: patch doesn't solve the segfault, with or without the patch both segfault on lobby join. If required I could get access to a fedora 29 machine for testing.

Stan added a subscriber: Stan.Nov 24 2018, 4:45 PM

@bb Got a stacktrace for the lobby crash ? To see if it has anything in common with Mac ?

elexis requested changes to this revision.Nov 24 2018, 7:41 PM

Stacktrace is on the trac ticket.

Tested on Fedora 29, with gloox::TLSDisabled it doesn't crash, with TLSOptional it does.

If this is going to be changed to encompass the TLSDisabled option, perhaps lobby.require_tls should be renamed (a three way choice would also be an option, or disabling this crap altogether).
Better would be a fix, investigating.

This revision now requires changes to proceed.Nov 24 2018, 7:41 PM

Fedora 29 uses GnuTLS 3.6.4 and gloox 1.0.14.
I've compiled with that GnuTLS version and the most recent gloox windows on Ubuntu 18.04 but can't reproduce there.
I suppose the checkbox is even relevant if the Fedora thing is fixed, because the next best platform might crash randomly as well.

elexis commandeered this revision.Nov 25 2018, 11:08 AM
elexis edited reviewers, added: Itms; removed: elexis.
elexis updated this revision to Diff 7007.EditedNov 25 2018, 11:09 AM

Use TLSDisabled, rephrase, nuke cert verification option until we have at least one machine where it works.

Confirmed with wireshark that it works.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/802/

Itms added a comment.Nov 25 2018, 1:56 PM

Looks good to me! Please commit it if this allows Fedora users to work around the crash.

elexis marked an inline comment as done.Nov 25 2018, 2:29 PM
elexis added inline comments.
binaries/data/mods/public/gui/prelobby/common/terms/Privacy_Policy.txt
42

Actually we can keep it as it's still enbled by default, if someone posts his password in the chat or disables encryption, not the fault of the service provider. (The sentence is more conflicting with the by default disabled certificate verification actually. Will be hard for someone to claim personal damages due to this fact, so whatever)

Better would be a fix, investigating.

Compiling the entire GnuTLS / gloox / 0ad stack cleanly everytime in a VM takes 30min and makes my system unusuable or even freezes it, so it ended up being deleted...
especially because the next best platform can have broken gloox as well,
considering how many gloox/gnutls bugs are already reported,
and considering that I hardly know every detail about gloox, gnutls and fedora for every version.

While typing the commit message, one always stumbles upon thoughts one didn't have in the revision proposal:

I wonder if the options dialog is the right place.
Users don't know that they need to visit the options dialog and change an option to solve the crash. They will report it regardless as a bug and then we can inform them what to do.
The only difference that this patch introduces is that they don't need to edit a file but can do it in the GUI.
But if the "TLS Encryption" option checkbox were in the login/register dialog, the user might guess to disable that checkbox, without us having to inform him of the existence and receommendation of that option.
Incidentally it would also be closer to what we want in #5344.
Let's see whether the lobby/login/register rewrite made that easier...

lyv added a subscriber: lyv.Nov 26 2018, 10:35 AM

Some kind of a warning or a note under places which could end up crashing.


It's getting more crowded. The terms buttons and feedback thing are uglily too large (but shrinking them would look differently awkward.
The new checkbox looks okay I think, but it still seems getting more confusing to the average user.

I guess it's better if the option wouldn't exist and if there was only a message box popping up, asking the user if he wants to proceed if the connection is unsafe, otherwise presume TLS.
But that would require a bit more rework - first attempting to login with TLS enabled and then asking if that failed - which is also not an option to circumvent any TLS handshake crashes...

(We could add an icon on the left and the buttons on the right, perhaps something like we find when doing a websearch for "gdpr icon".
Notice Article 12 https://gdpr-info.eu/art-12-gdpr/ speaks about "standardised icons", but I've never seen them.)

elexis updated this revision to Diff 7008.Nov 26 2018, 11:24 AM

Add the checkbox in to the login/register page instead of options dialog.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/prelobby/common/encryption/encryption.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/prelobby/common/encryption/encryption.js
|  10|  10| 
|  11|  11| function updateEncryption(enabled)
|  12|  12| {
|  13|    |-	saveSettingAndWriteToUserConfig("lobby.tls", String(enabled))
|    |  13|+	saveSettingAndWriteToUserConfig("lobby.tls", String(enabled));
|  14|  14| }

binaries/data/mods/public/gui/prelobby/common/encryption/encryption.js
|  13| »   saveSettingAndWriteToUserConfig("lobby.tls",·String(enabled))
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

Link to build: https://jenkins.wildfiregames.com/job/differential/803/

lyv added a comment.Nov 26 2018, 11:30 AM

The problem of crashes still persists. The average user would not know (wont even try probably) the cause of the crash. And there would be a lot of “disable TLS” replies to reports. Pop-up or a tooltip would do maybe.

Arguably the previous diff was better, because it didn't confuse the uneducated player further in the login dialog. Arguably this diff is better, because it empowers the educated player.
As a third alternative one could commit only the cpp + default.cfg diff, as linux people are usually smart enough to edit a config file, and the other platforms, macOS and windows, seem tested by us.
Meh.

In D1679#66711, @smiley wrote:

The problem of crashes still persists. The average user would not know (wont even try probably) the cause of the crash. And there would be a lot of “disable TLS” replies to reports. Pop-up or a tooltip would do maybe.

Yes, but since when do we inform users of bugs ingame to prevent duplicate bugreports?I think the reply to duplicate bugreports would just be "disable TLS until #5349 is fixed", no? And there don't seem to be too many Fedora / affected linux users, no?

There could be a random library crash everywhere, developers have to be able to rely on library functions as defined in the specs.
So using TLS by default and a message box if its unavailable otherwise should be possible - just not for all platforms in 2018.
0ad+gloox+gnutls is too buggy currently to implement that, so we gotta chose one of the three proposals, or a fourth one if you have a specific one.

lyv added a comment.Nov 26 2018, 11:44 AM

I guess it’s low priority if it only affect a subset of a platform. Same goes for the glsl crash too.

Stan added a comment.Nov 26 2018, 11:47 AM

We could add some text under the TLS encryption saying its broken, on WinXP + OsX < 10.12 + Fedora ?

Stan added a comment.Nov 26 2018, 12:13 PM

A better idea.

Not sure it's possible though. Can we rely on the HWdetect to get the platform os version to disable (grey out) the enable tls (with a nice a message, such as not supported on your platform)

lyv added a comment.EditedNov 26 2018, 1:44 PM

What about people on those platforms who have fixed it themselves. This is more of a library bug rather than the OS.

Itms added a comment.Nov 26 2018, 2:08 PM

First of all, I can reproduce the crash on Fedora, and the current patch does fix it when the TLS box is unticked. So the C++ change is good ?

Secondarily, I do not have a strong opinion on Options vs. Login. I do not find the new login dialog ugly, I find however the XML/JS code a bit cluttered by the change (with respect to adding a single entry in the options JSON). I am not sure the login checkbox is that much more explicit for educated users than a config option. I think very few will risk unchecking that without someone suggesting them to do so.

Finally, adding detection of specific platforms via the hardware detection is equivalent to hardcoding the list of broken platforms in the game, so I think it's not a good idea. Just as smiley says, it's like GLSL: if someone says "my lobby crashes", the immediate proposal we can make will be "you can try disabling TLS". It is a new feature of A23b and it's unfortunately not our fault if gloox is broken in some places.
(Ah sorry I'm just paraphrasing your D1679#66713 comment elexis, I agree with you).

Stan added a comment.Nov 26 2018, 2:23 PM

I understand your point. My concern was mainly about people who don't report bugs because it's not a common thing to do :)

Itms added a comment.Nov 26 2018, 6:06 PM
In D1679#66719, @Stan wrote:

I understand your point. My concern was mainly about people who don't report bugs because it's not a common thing to do :)

Indeed. But we don't really need tons of bugreports here, we know what are the issues. We just have to follow the upstream work on gloox (for instance we could make it work on OSX 10.9 by making gloox support darwinssl), but that is more long-term.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 27 2018, 4:12 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Nov 27 2018, 4:12 PM