Welcome to MaxCheaters.com

Register now to gain access to all of our features. Once registered and logged in, you will be able to create topics, post replies to existing threads

LoginPacket - Critical security bug (all chronicles)


Recommended Posts

Hello everyone,

 

I'd like to inform you about critical security bug that's present in all widely known NCsoft l2server.exe binaries. If you're using Vanganth, you're definitely vulnerable to this one. If you're using MyExt64, update your server immediately (it's fixed in commit c52b36e).

 

Quote

 

"Someone has been using my builder account!"

-- Papa Bear, NCsoft and the Three Bears

 

 

When a player tries to log in, client sends LoginPacket with account ID and session ID. L2server should search for given account ID in std::map<int, int>, compare result with session ID and decide whether it's the correct account and session. However, some programmer at NCsoft made a terrible mistake. Instead of searching in the map like

std::map<int, int>::const_iterator i = map.find(accountId);
if (i == map.end() || i->second != sessionId) return true; // disconnect user

they ended up searching the map this way:

if (map[accountId] != sessionId) return true; // disconnect user

As you can see, if you supply arbitrary accountId and sessionId of 0, l2server will let you in (because it will add std::pair<int, int>(accountId, 0) to the map and then return 0). In reality you can't use any account ID as it's also searched in another std::map, so it works only for accounts that have been logged in since server start - but this is the only limitation.

 

This bug is really critical, if a player is able to guess account ID of some character with builder that has been logged in since server start, nasty things are going to happen...

 

I suggest everyone to fix it as soon as possible - you can see the fix in this commit https://bitbucket.org/l2shrine/extender-public/commits/c52b36e8aad518a094774aca49f2b78da7da390b (for Gracia Final, for other chronicles you'll have to find correct addresses)

  • Like 1
  • Thanks 1
  • Upvote 2

Share this post


Link to post
Share on other sites
  • Replies 33
  • Created
  • Last Reply

Top Posters In This Topic

Top Posters In This Topic

Popular Posts

Hello everyone,   I'd like to inform you about critical security bug that's present in all widely known NCsoft l2server.exe binaries. If you're using Vanganth, you're definitely vulnerable to this one. If you're using MyExt64, update your server immediately (it's fixed in commit c52b36e).     When a player tries to log in, client sends LoginPacket with account ID and session ID. L2server should search for given account ID in std::map<int, int>, compare result wit

If they wrote it in C, there would be many more problems than this one (much more code to write).   As for map [] operator - all good C++ programmers KNOW that it inserts element if it's not already present. Operator overloading is good feature - if used sanely - I definitely like writing s == "abc" more than writing s.isEqual("abc") or strcmp(s, "abc") == 0.   Generally speaking, I see C++ bit safer, but you have to use it well. You can write bad code in any language.

25 minutes ago, xxdem said:

A good reason on why they should stick with C on such case, this can't happen in C

Love how your first response is "It's the language's fault". Yeah, blame the tools not the idiot who wrote this.

 

Ontopic: Amazing catch good job.

Share this post


Link to post
Share on other sites

You don't know if the guy who wrote this was idiot, probably he is better than all of us since he was given such a critical task, the fact is, that IF this bug exists is either a backdoor or something that slipped away from them for all those reasons that make C++ shit

Share this post


Link to post
Share on other sites
5 minutes ago, xxdem said:

You don't know if the guy who wrote this was idiot, probably he is better than all of us since he was given such a critical task, the fact is, that IF this bug exists is either a backdoor or something that slipped away from them for all those reasons that make C++ shit

Really poor comment

Share this post


Link to post
Share on other sites
1 hour ago, xxdem said:

A good reason on why they should stick with C on such case, this can't happen in C

 

If they wrote it in C, there would be many more problems than this one (much more code to write).

 

As for map [] operator - all good C++ programmers KNOW that it inserts element if it's not already present. Operator overloading is good feature - if used sanely - I definitely like writing s == "abc" more than writing s.isEqual("abc") or strcmp(s, "abc") == 0.

 

Generally speaking, I see C++ bit safer, but you have to use it well. You can write bad code in any language.

Edited by eressea
  • Like 1

Share this post


Link to post
Share on other sites
21 hours ago, eressea said:

 

If they wrote it in C, there would be many more problems than this one (much more code to write).

 

As for map [] operator - all good C++ programmers KNOW that it inserts element if it's not already present. Operator overloading is good feature - if used sanely - I definitely like writing s == "abc" more than writing s.isEqual("abc") or strcmp(s, "abc") == 0.

 

Generally speaking, I see C++ bit safer, but you have to use it well. You can write bad code in any language.

 

That's not quite true about C, it would just be done on a more plain way, you don't really need a complex data structure to do this job, imho.

 

I love that you immediately noticed that on this specific case I was talking about operator overloading ([] more specifically) I didn't become too specific because there are lots of noobs here that wouldn't understand my point but you are smart enough to understand of what exactly I didn't like about the code :)

Share this post


Link to post
Share on other sites

PS: with all the respect I have towards you, you get a big fuck you for overloading == into strcmp, the word to describe what it feals like is "sacrilege" in my country :p

 

Makes low level programming feel like frontend script code, and you also loose lots of functionality like inability to know and compare by address

Edited by xxdem

Share this post


Link to post
Share on other sites
4 hours ago, xxdem said:

That's not quite true about C, it would just be done on a more plain way, you don't really need a complex data structure to do this job, imho.

 

Writing red-black tree or hashmap in C is really pain in the ass unless you use macros (which I’m trying to avoid/minimize).

It can be done in asm, it can be done in C and it also can be done in C++ - with no or really little performance impact. I find maintaing clean C++ codebase easier than in C or asm - but it’s up to you to pick language you prefer. If written correctly, the result will be almost identical. But it will probably take twice time in asm vs C and twice time in C than in C++. All languages have pros and cons.

Share this post


Link to post
Share on other sites
1 minute ago, eressea said:

 

Writing red-black tree or hashmap in C is really pain in the ass unless you use macros (which I’m trying to avoid/minimize).

It can be done in asm, it can be done in C and it also can be done in C++ - with no or really little performance impact. I find maintaing clean C++ codebase easier than in C or asm - but it’s up to you to pick language you prefer. If written correctly, the result will be almost identical. But it will probably take twice time in asm vs C and twice time in C than in C++. All languages have pros and cons.

 

My point was that it can be done without a hash map, I didn't meant "implement a C hashmap" that would be stupid, hash maps have a shitload of features you don't need on this case, (one of these features made this bug)

Share this post


Link to post
Share on other sites
2 hours ago, xxdem said:

 

My point was that it can be done without a hash map, I didn't meant "implement a C hashmap" that would be stupid, hash maps have a shitload of features you don't need on this case, (one of these features made this bug)

 

Well, you need to map one ID to another - and as those IDs are sparse, you can't just allocate few megabyte array and store it there. You need some associative container - either red-black map or hashmap... Of course it still can be some sorted (or unsorted) array or something like that, but it will be harder to manage, especially in case there won't be enough allocated memory to store all elements; you would have to reallocate and copy; also insert and delete function would be far from O(log n), however search operation would be O(log n) via binary search if it's sorted.

 

I don't say C is bad - but std::map is really much more convenient and when used well, it's perfectly safe.

Share this post


Link to post
Share on other sites

imho I would just go O(n) with an array of struct{int accountId, int sessionId} and perform a search or something similar, it doesn't seem that the n will ever be huge on this case, I could be wrong

Share this post


Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.