Page MenuHomeWildfire Games

Forces SSL for the UserReport
AbandonedPublic

Authored by vladislavbelov on Jul 29 2018, 11:20 PM.

Details

Reviewers
elexis
Summary

Uses SSL for the UserReport.

Test Plan
  1. Change the userreport.url to the HTTP one.
  2. Check that a connection is failed.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6308
Build 10464: Vulcan BuildJenkins

Event Timeline

vladislavbelov created this revision.
vladislavbelov added inline comments.
binaries/data/config/default.cfg
477

The v1/ suffix was removed, because it's duplicated by the version in the UserReport.

Vulcan added a subscriber: Vulcan.Jul 29 2018, 11:25 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/695/display/redirect

Adds the Mod.IO support.

Updates the year.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/696/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/differential/697/display/redirect

elexis added a comment.Aug 1 2018, 8:58 PM

Thanks for writing the patch, it's much better with SSL for the UserReporter.

I could not test whether the patch works as advertized if someone passed HTTP instead of HTTPS for mod.io, because the curl error are not relayed and it only displays "Could not parse JSON".

So I can't full verify the correctness unless we have some cheap LOGWARNING or whatever to display the error code.

I can't imagine it not working, but I find myself surprised sometimes with library functions. So you can edit the error code or commit without my verification if you are dead serious about the correctness.

(also some less relevant / verbose comments in irc at date of first publication)

We should also have it for the lobby, I guess me or Jan should generate a certificate.

binaries/data/config/default.cfg
477

Ok.

This could be a backend version (v1) while the version that the UserReporter sends could be considered the client version (11).

Perhaps the idea is that old clients can still report at v1 and newer at v2.
But for that one could just add a gateway, so the version seems unneeded (not to mention that its ugly to have the version in the path of the URL).

source/ps/UserReport.cpp
98

nuke (as agreed on irc since one may want to have the verbose output option without adding possible but unlikely security risks)

121

Checked all options at https://curl.haxx.se/libcurl/c/curl_easy_setopt.html, couldn't find anything else that would be useful, maybe dunno probably not CURLOPT_FAILONERROR

elexis added inline comments.Aug 26 2018, 1:12 PM
binaries/data/config/default.cfg
477

HTTP Test negative: When I apply this patch and keep the "http" in default.cfg, the UserReporter returns the http status code 301. But this seems wrong, because the SSL/TLS negotiation should have been rejected before the HTTP status code is received, no?

HTTPS Test positive: The current certificate is invalid (cert for wrong host), and the UserReporter shows "SSL" as an error reason - which is correct and sufficiently informative.

The backend is not ready to use this patch:

  • feedback.wildfiregames.com still resolves to Philips IP.
  • Philips server does not support SSL.
  • Our server currently passes the certificate from svn.wildfiregames.com, i.e. is invalid.
  • Since gloox rejects the lobby certificate with SChannel on windows and since curl uses SChannel too, the UserReporter might reject the proper Lets Encrypt certificate too.
elexis abandoned this revision.Oct 4 2018, 4:32 PM

So I can't full verify the correctness unless we have some cheap LOGWARNING or whatever to display the error code.

Proper © error display was implemented in rP21892, so the patch can be tested since.

I can't imagine it not working, but I find myself surprised sometimes with library functions.

And that's why we require to test things before committing them.

Patch doesn't actually work for me and still allows upload with http. I could see with Wireshark that the up and download was done using HTTP, though it seems zlib encrypted (and an absence of any TLS initiation within the http cleartext data, also that would be a protocol new to me).

Actually it seems this only uses SSL/TLS for FTP and other protocols, but not HTTP!
https://curl.haxx.se/libcurl/c/CURLOPT_USE_SSL.html
https://curl.haxx.se/libcurl/c/libcurl-errors.html#CURLEUSESSLFAILED

So if it should be enforced in C++, it'd have to be a rejection if the URL starts with http:// Im afraid.
But we wanted to protect from malicious JS mods changing the URL anyway by adding protected config entries for such sensitive data.

Btw the curl UserAgent exposes possibly sensitive data
0ad libcurl/7.47.0 GnuTLS/3.4.10 zlib/1.2.8 libidn/1.32 librtmp/2.3 (http://play0ad.com/)

In D1600#65204, @elexis wrote:

I can't imagine it not working, but I find myself surprised sometimes with library functions.

And that's why we require to test things before committing them.
Patch doesn't actually work for me and still allows upload with http. I could see with Wireshark that the up and download was done using HTTP, though it seems zlib encrypted (and an absence of any TLS initiation within the http cleartext data, also that would be a protocol new to me).
Actually it seems this only uses SSL/TLS for FTP and other protocols, but not HTTP!
https://curl.haxx.se/libcurl/c/CURLOPT_USE_SSL.html
https://curl.haxx.se/libcurl/c/libcurl-errors.html#CURLEUSESSLFAILED
So if it should be enforced in C++, it'd have to be a rejection if the URL starts with http:// Im afraid.
But we wanted to protect from malicious JS mods changing the URL anyway by adding protected config entries for such sensitive data.
Btw the curl UserAgent exposes possibly sensitive data
0ad libcurl/7.47.0 GnuTLS/3.4.10 zlib/1.2.8 libidn/1.32 librtmp/2.3 (http://play0ad.com/)

But the url still can be changed anyway, no?

Stan added a subscriber: Stan.Oct 30 2018, 9:50 AM
Stan added inline comments.
source/ps/ModIo.cpp
211

Typo.

source/ps/UserReport.cpp
121

Typo in the comment.

But the url still can be changed anyway, no?

The frontend and the backend should be kept in sync:
https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/tools/webservices/userreport/

In D1600#65632, @elexis wrote:

But the url still can be changed anyway, no?

The frontend and the backend should be kept in sync:
https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/tools/webservices/userreport/

Don't we use our new feedback server? Also http > https doesn't matter for backend.

In D1600#65632, @elexis wrote:

Don't we use our new feedback server?

The repository should still be kept in sync with what we installed on the server.

Also http > https doesn't matter for backend.

The frontend rejects servers that don't support https (keeping the promise of the terms and conditions).

Stan added a comment.Oct 30 2018, 8:45 PM

Maybe it would be nice to have a separate repository ? So you can pull to keep it in sync automatically.