##########################################################################################

       LOC-13: Message payloads from SYSTEM should be encrypted

##########################################################################################


Issue Type: Improvement 
-----------------------------------------------------------------------------------------

Issue Information
====================

Priority: Major					Status:      Closed
Resolution:  Done (2018-05-19 11:04:10)
Project: LocalChat (LOC)


Reported By: btasker					
Assigned To: btasker

Components: 
	- Client 
	- Server 
	- Encryption 


Targeted for fix in version: 
	- V0.0.2


Time Estimate: 0 minutes
Time Logged:   0 minutes


-----------------------------------------------------------------------------------------

Issue Description
==================

Currently there's a try statement in the client to detect whether a message payload is
pure JSON (i.e. a message from SYSTEM) or is an encrypted blob (a standard message).

The problem with this is it's possible for an arbitrary client to push a cleartext message
(it won't display as being from SYSTEM but is still a concern).

So, SYSTEM should really use some form of E2E encryption.

The problem with this is the server does not (and should not) know the room  key, so we
cannot encrypt with the same key as standard messages use. Even if we could, we probably
shouldn't.

Needs some thought, but my initial suggestion is that at room creation, the server
generate a key for SYSTEM to use and store it in the rooms table.

When a user joins the room, they should be passed the SYSTEM key.

When returning messages in pollMsg there should be a flag to indicate whether the message
is a system message or not. If the flag is 1, then the SYSTEM key should be used, if not
the standard room key should be used.

This offers limited protection if someone takes a memory dump on the server - the key will
also be in memory, though they won't immediately get a plaintext dump of SYSTEM messages.
It's aim is purely to remove a potential loophole/vulnerability in the client.

Client's will, of course, have access to the SYSTEM key, but should be entirely unable to
affect the flag returned in pollMsg to trigger it's use when decrypting their message. As
a precaution though, the flag should *only* ever be used to select the decryption key, and
not to identify whether it's a SYSTEM message or not.

-----------------------------------------------------------------------------------------

Activity
==========

	
-----------------------------------------------------------------------------------------
2018-05-19 10:46:37              git
-----------------------------------------------------------------------------------------


-- BEGIN QUOTE --

Repo: LocalChat
Host:Rimmer

commit 1a62526289a0e8b426f8080b7b20ee9fcbf862cb
Author: B Tasker <github@<Domain Hidden>>
Date:   Sat May 19 10:44:02 2018 +0100

Commit Message: LOC-13 - Encrypt system message payloads.
    
    This is done for two main reasons - firstly to minimise the amount of obvious
plaintext in memory (though it is still base64 encoded, so limited effect there).
    
    Secondly, and more importantly, it allows us to adjust the client so that it will
never honour a plaintext JSON message payload.

 client/LocalChatClient.py |   24 ++++++++++++++----------
 server/LocalChat.py       |   39 +++++++++++++++++++++++++++++++++++----
 2 files changed, 49 insertions(+), 14 deletions(-)

-- END QUOTE --

https://repos.bentasker.co.uk/projects/LocalChat.git/commits/1a62526289a0e8b426f8080b7b20ee9fcbf862cb
|
https://repos.bentasker.co.uk/projects/LocalChat.git/commitdiff/1a62526289a0e8b426f8080b7b20ee9fcbf862cb

	
-----------------------------------------------------------------------------------------
2018-05-19 11:04:02              btasker
-----------------------------------------------------------------------------------------

This has now been implemented.

The key used is system wide rather than per-room. I had originally started implementing
per-room (as otherwise, access to a single room allows you to decrypt system messages from
any room - assuming you can get those messages in the first place).

I changed my mind though. The syskey doesn't really offer much, if any, protection against
someone taking a memory dump (and it's not designed to) as the key needed to decrypt those
messages is also in memory. So it's probably not worth the processing overhead of having
to store (and lookup) per-room encryption keys.

An attacker with access to room 'room1' still shouldn't be able to fetch messages from
'room2' (I await the discovery of a horrible bug which renders this untrue....), so the
only protection the key might offer at the server's end is against someone who can gain
access to memory on the server.

The primary aim (now accomplished) was to be able to tell the client that it should never
accept a plain JSON message payload from the server. Although I can't immediately think of
any specific attacks which could be achieved, it does seem wise to prevent the client from
attempting to de-serialise arbitrary strings (the payload is provided by the sending user,
so they _could_ conceivably put something nasty in there, Whereas the rest of the JSON
response to pollMsg is generated by the server, so should be fine outside of server
compromise).

Having a system wide key may, of course, be something we later come to regret, but
currently on balance it seems like the best option.
	
-----------------------------------------------------------------------------------------
2018-05-19 11:04:10              
-----------------------------------------------------------------------------------------

btasker changed status from 'Open' to 'Resolved'
	
-----------------------------------------------------------------------------------------
2018-05-19 11:04:10              
-----------------------------------------------------------------------------------------

btasker added 'Done' to resolution
	
-----------------------------------------------------------------------------------------
2018-05-19 11:04:13              
-----------------------------------------------------------------------------------------

btasker changed status from 'Resolved' to 'Closed'