TheGrandmother / MUD

The worlds lames mud :/
1 stars 1 forks source link

Message injection is possible! #20

Closed Chilinot closed 8 years ago

Chilinot commented 8 years ago

It is possible to inject messages inside messages. We need to escape all input from the user before we construct the messages! An example for this can be seen in commit e212d9876b2e4837c14ee96cf9cf81e27bf092b1

Chilinot commented 8 years ago

A possible fix for this might be to URL-encode all input from the user before adding it to the message.

Example: server;urlencode(username);HANDSHAKE;null;345678;\n

This should remove possible injections if im not mistaken.

TheGrandmother commented 8 years ago

Indeed!

Or we could just o regular escaping. so if the user wants to have a ; in say something he says to another player the ui adds an escape character like \ to the outgoing message and resolves escapes when handling incoming messages.

This could also be use to solve the problem with non ascii characters. Like å,ä,ö

TheGrandmother commented 8 years ago

As it is now im unable to send a message with an argument hi :) which makes chatting kinda pointless :)

Chilinot commented 8 years ago

you can have spaces in the arguments. But since the use of regex is so strict i need to add support in the regex for the two characters : and ) :)

Chilinot commented 8 years ago

Base64 is also an alternative. Example: input: tjenare!! hallågåsse!;;,1@£:() base64 output: dGplbmFyZSEhIGhhbGzDpWfDpXNzZSE7OywxQMKjOigp

Chilinot commented 8 years ago

base64 also has the added bonus of only producing output that can be parsed by the "\w" character in regex.

TheGrandmother commented 8 years ago

:+1:

It would be trivial to re factor the front end. I'll just need to add 'atob()' and 'btoa()' to the sender and the receiver function and it would work!

Chilinot commented 8 years ago

So lets go with base64. How much should we encode? Should we only encode fields the user can effect or every single field including type, action, and timestamp?

Example for only user manipulated input: b64(receiver);b64(sender);type;action;timestamp;b64(arg1);...b64(argN);

TheGrandmother commented 8 years ago

Lets just do the user fields.

Otherwise debugging may become painful

Chilinot commented 8 years ago

This issue should be fixed in the commit i just pushed. But i will not merge the base64 branch until your javascript interface can properly handle the new messages.

TheGrandmother commented 8 years ago

My branch can handle them now!

Although ÅÄÖ are still not working. They get encoded but they still show up as ???.

TheGrandmother commented 8 years ago

The reason why we got those NullPointerExceptions was actually that the server ran Java-7 and java.util.Base64 was added in Java-8.

But since WebSocketServer.WebSocketWorker overrode the default UncaughtExceptionHandler it din't print the stack trace and it lost the actual NoClassDefFoundError exception. I had to do some changes in the source of the WebsocketsLibrary to actually find the bug :p

Chilinot commented 8 years ago

Nice work :) The åäö problem might be the locale issue we had earlier. It is one of the earlier closed issues.