A few days ago I posted an update to my websocket chat demo that talked about associating a CFC with the web socket to perform server side operations. While testing the chat, a user (hope he reads this and chimes in to take credit) noted another security issue with the code. I had blogged on this topic already, specifically how my chat handler was escaping HTML but could be bypassed easily enough. The user found another hole though. Let's examine it, and then I'll demonstrate the fix.
When the chat button is pressed, the following code is run:
$("#sendmessagebutton").click(function() { var txt = $.trim($("#newmessage").val()); if(txt == "") return; msg = { type: "chat", username: username, chat: txt }; chatWS.publish("chat",msg); $("#newmessage").val(""); });
I've removed my HTML escaping code since the server handles it. But pay attention to the message payload. It contains a type, a username, and a chat. The username value is set after you sign in. It's a simple global JavaScript variable. It's also trivial to modify. Just create your own structure and pass it to the web socket object:
chatWS.publish("chat", {type:"chat",username:"Bob Newhart", chat:"Howdy"});
The server will gladly accept that and pass it along to others. Not good. Luckily there is a simple enough fix for this. My first change was to remove the username from the packet completely.
$("#sendmessagebutton").click(function() { var txt = $.trim($("#newmessage").val()); if(txt == "") return; msg = { type: "chat", chat: txt }; chatWS.publish("chat",msg); $("#newmessage").val(""); });
If you remember, we had a CFC associated with our web socket that was handling a variety of tasks. One of them supported stripping HTML. Here is the original method:
public any function beforeSendMessage(any message, Struct subscriberInfo) { if(structKeyExists(message, "type") && message.type == "chat") message.chat=rereplace(message.chat, "<.*?>","","all"); return message; }
Notice the second argument we didn't use? This a structure of data associated with the client. We modified this a bit on our initial subscription to include our username. That means we can make use of it again:
message["username"]=arguments.subscriberInfo.userinfo.username;
This will now get returned in our packet. Check it our yourself below. I've included a zip of the code. (And this is my last chat demo. Honest.)
OOPS!
Turns out I have a critical mistake in my fix, but it's one of those awesome screwups that lead to learning. As soon as I posted my demo, a user noted that his chats were being marked as coming from me. I had no idea why. I then modified my CFC to do a bit of logging:
var myfile = fileOpen(expandPath("./log.txt"),"append"); fileWriteLine(myfile,serializejson(message) & "----" & serializejson(subscriberInfo));
I saw this in the log file:
{"chat":"TestAlphaOne","type":"chat"}----{"userinfo":{"username":"chk"},"connectioninfo":{"connectiontime":"March, 02 2012 09:57:49","clientid":542107549,"authenticated":false},"channelname":"chat"}{"chat":"TestAlphaOne","type":"chat"}----{"userinfo":{"username":"Ray"},"connectioninfo":{"connectiontime":"March, 02 2012 09:56:18","clientid":1511146919,"authenticated":false},"channelname":"chat"}
At the time of this test, there were two users. My one message was sent out 2 times. So this is interesting. To me, I thought beforeSendMessage was called once, but it's actually called N times, one for each listener. That's kind of cool. It means you could - possibly - stop a message from going to one user. Of course, it totally breaks my code.
One possible fix would be to simply see if USERNAME existed in the message packet. But if I did that, an enterprising hacker would simply supply it.
When I figure this out, I'll post again.
SECOND EDIT
Woot! I figured it out. Turns out I should have been using beforePublish. It makes total sense once you remember it exists. It also makes more sense to have my HTML "clean" there too. Things got a bit complex though.
beforePublish is sent a structure too, but it only contains the clientinfo packet. It does not contain the custom information added by the front-end code. I'm thinking this is for security. But, we have the clientid value and we have a server-side function, wsGetSubscribers. If we combine the two, we can create a way to get the proper username:
if(structKeyExists(message, "type") && message.type == "chat") { //gets the user list, this is an array of names only var users = getUserList(); var myclientid = publisherInfo.connectioninfo.clientid; var me = users[arrayFind(wsGetSubscribers('chat'), function(i) { return (i.clientid == myclientid); })]; message.chat=rereplace(message.chat, "<.*?>","","all"); message["username"]=me; } return message; }public any function beforePublish(any message, Struct publisherInfo) {
Does that logic make sense? Basically we are just comparing clientids. I've restored the demo so have at it!
