Discussion LoginPacket - Critical security bug (all chronicles)

[eressea]

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)

[Nightw0lf]

amaging mistake/awsome fix thank you

[xxdem]

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

[Elfo]

a

Edited October 1, 2021 by Elfocrash

[xxdem]

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

[Elfo]

a

Edited October 1, 2021 by Elfocrash

[eressea]

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 July 30, 2018 by eressea

[Pulentolento]

Thank you Eressea for you amazing yob!!

[Plawed]

Thanks again Eressea. Your contributions towards the l2off development community are always amazing.

[xxdem]

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 :)

[xxdem]

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 July 31, 2018 by xxdem

[eressea]

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.

[xxdem]

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)

[eressea]

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.

[xxdem]

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