Some details about the nightwatch bug allowing lobby connection injection:
I think the infrastructure deserve some freshening. If you don't, keep reading anyway. I'm currently reading a lot of code because I want to understand what exactly all this ZK code is doing and how it interacts with lobby clients, lobby server and various bots. In particular, I'm looking at authentication.
Here are the tools I used:
- grep
- less
Sometimes I use better tools too. But that's when I want to find a needle in a haystack and not the opposite.
MSDN documentation is also required.
Often, I come across code that doesn't look right when reading code from ZK repository.
Here is an example:
https://github.com/ZeroK-RTS/Zero-K-Infrastructure/blob/master/NightWatch/AuthService.cs#L481Got it? It doesn't
look right. This thing is concatenating strings together, but its not checking whether the strings actually contain spaces (or other suspicious chars) and it is not escaping them. If you don't want to dig TasClient (instance client) code... I can assure you that SendRaw does what its name suggest: it will send anything, raw.
Now, if you have a playful mind... you may wonder what would happen if you could slip spaces in either login or hashedPassword. Even nastier... can you imagine what you could do if you could slip newlines into that command?
Imagine, if you could manage to reach line 481 with the following values:
-login is a multiline string:
"bla bla
SAYEX sy is dreaming of electric sheep
PING"
-hashedPassword: anything will do... lets say we use "BAAAA"
This would get sent over the lobby channel as:
1234 TESTLOGIN bla bla
SAYEX sy is dreaming of electric sheep
PING BAAAA
Brillant!
(maybe at this point you think it would be wise to send a valid login/pass pair instead of bla/bla... but don't do it, it would probably create a mess in zk's db)
But can we manage to slip this multiline string into login?
The number #1 security advice is: don't trust user inputs. So normally, this should not be possible... it would be non-sense to allow a user to pick a multiline username anyway. But remember about the needlestack and the haystack. When you see suspicious code like that, its not always obvious that it may be exploitable or how to control the values of the variables... but in this case, the variables are named login and hashedPassword.
So now, we need to find what is calling this function.
grep can do wonders. Sure, it sucks when compared to tools that have some knowledge of the structure of the code you are analyzing. But its a needlestack, we need gloves more than a radar.
https://github.com/ZeroK-RTS/Zero-K-Infrastructure/blob/master/Zero-K.info/AppCode/AuthServiceClient.cs#L54AuthServiceClient.VerifyAccountHashed() is calling NightWatch.Auth.VerifyAccount().
Before that, it makes a few checks. No problem with my multiline login... it has multiple lines, so it is not empty :)
Also, there is no account in database with such a stupid login name, so of course, VerifyAccount() gets called.
Lets move up... what do we need to do to reach AuthServiceClient.VerifyAccountHashed() ?
https://github.com/ZeroK-RTS/Zero-K-Infrastructure/blob/master/Zero-K.info/Global.asax.cs#L120MvcApplication_PostAuthenticateRequest()
Ok... if you want the gory details regarding when exactly this handler gets called, have fun with the MSDN documentation.
All I cared is that it gets called at some point. And that it calls AuthServiceClient.VerifyAccountHashed() without any extra check.
Read this line, then read it again:
if (acc == null) if (Request[GlobalConst.LoginCookieName] != null) acc = AuthServiceClient.VerifyAccountHashed(Request[GlobalConst.LoginCookieName], Request[GlobalConst.PasswordHashCookieName]);
I spent some time wondering how to fit a multiline string into a cookie. I have not found a way to do it, but maybe there is.
You may think its a dead end then? You read that code line many times, right? You have seen how the code gets 2 cookies whose names are in GlobalConst.LoginCookieName and GlobalConst.PasswordHashCookieName. You agree with that?
Actually... that's what is expected by the code author(s). (Note: all this authentication system is horrible, but that was the subject of another issue so I won't cover it).
The unintended benefits of using a tool such as grep rather than a high-level tool is that sometimes your regex matches unexpected stuff. (For that reason, I suggest that grep should be renamed to serendipity.). I noticed something odd while looking for code accessing cookies in ZK's repository. Like every piece of code in ZK, there is no consistency in the methods... there is code duplication and just random stuff. But in the end, I wondered about the Request[something] notation. I mean... its probably not a dumb array of only cookie values with such a generic name. And indeed:
http://msdn.microsoft.com/en-us/library/System.Web.HttpRequest%28v=vs.110%29.aspx"To access data from the QueryString, Form, Cookies, or ServerVariables collections, you can write Request["key"], as shown in the example for the QueryString property."
You must be kidding right? That is a really stupid way to design an API (hi microsoft). I can not think of any reason why someone would design such a stupid function and I can not think of any reason why it is used here... but maybe that opens new doors:
- QueryString... why not? maybe later
- Cookies... I know that... how do you think I arrived here?
- ServerVariables... I do not control that! well, maybe, but that is beyond the point... :)
- Form... ewwww... a form, like one of those that can include textarea fields... and so, are multiline?
Game over.
I'm not posting more details or the electric sheepsploit code. But I gave working proof... ;)
My rant about how fucked up the infra is will come later...