[leong124]

Today my server was crashed by a player who seemed to be sending null commands to the server when they connect my server. Crashdetector reports that there was invalid memory access:

Code:

[2012-12-22 17:15:45] [join] ********* has joined the server (0:***.***.***.***)
[2012-12-22 17:15:47] [debug] Run time error 5: "Invalid memory access"
[2012-12-22 17:15:47] [debug] AMX backtrace:
[2012-12-22 17:15:48] [debug] #0 00000288 in public OnPlayerCommandText (playerid=0, cmdtext[]=@0x0006f098 "") at D:\Program Files (x86)\Rockstar Games\GTA San Andreas\pawno\include\zcmd.inc:92
[2012-12-22 17:15:48] [debug] Run time error 5: "Invalid memory access"
[2012-12-22 17:15:48] [debug] AMX backtrace:
[2012-12-22 17:15:48] [debug] #0 00000378 in public OnPlayerCommandText (playerid=0, cmdtext[]=@0x00061f38 "") at D:\Program Files (x86)\Rockstar Games\GTA San Andreas\pawno\include\zcmd.inc:92
[2012-12-22 17:15:48] [debug] Server crashed while executing scfri.amx
[2012-12-22 17:15:48] [debug] AMX backtrace:
[2012-12-22 17:15:48] [debug] #0 native CallLocalFunction () [00472c00] from samp-server.exe
[2012-12-22 17:15:48] [debug] #1 0000298c in public OnPlayerCommandText (playerid=0, cmdtext[]=@0x003f2e88 "") at D:\Program Files (x86)\Rockstar Games\GTA San Andreas\pawno\include\zcmd.inc:87
[2012-12-22 17:15:48] [debug] System backtrace:
[2012-12-22 17:15:48] [debug] #0 004010b6 in ?? () from D:\Program Files\samp_server\samp-server.exe
[2012-12-22 17:15:48] [debug] #1 00d73c32 in ?? () from D:\Program Files\samp_server\plugins\crashdetect.dll
[2012-12-22 17:15:48] [debug] #2 00d76076 in ?? () from D:\Program Files\samp_server\plugins\crashdetect.dll
[2012-12-22 17:15:48] [debug] #3 00d6c05a in ?? () from D:\Program Files\samp_server\plugins\crashdetect.dll
[2012-12-22 17:15:48] [debug] #4 00d73c5e in ?? () from D:\Program Files\samp_server\plugins\crashdetect.dll
[2012-12-22 17:15:48] [debug] #5 0046d9d0 in ?? () from D:\Program Files\samp_server\samp-server.exe

zcmd:

pawn Code:

public OnPlayerCommandText(playerid, cmdtext[])
{
    state zcmd:y;
    if (zcmd_g_HasOPCS && !CallLocalFunction("OnPlayerCommandReceived","ds",playerid, cmdtext)) return 1;//line 87
    new
        pos = 1,
        funcname[MAX_FUNC_NAME] = "cmd_",
        funcname2[MAX_FUNC_NAME] = "";
    while (pos <= MAX_FUNC_NAME && cmdtext[pos] > ' ') //line 92, I don't know why it crashes here
    {       
        funcname2[pos-1] = tolower(cmdtext[pos]); 
        pos++;
    }
    strcat(funcname, funcname2);
    #if defined FILTERSCRIPT
    if(!zcmd_g_HasOPCS && funcidx(funcname) != -1 && !CallRemoteFunction("OnPlayerCommandReceived", "is", playerid, cmdtext)) return 1;
    #endif
    while (cmdtext[pos] == ' ') pos++;
    if (!cmdtext[pos]) 
    {
        if (zcmd_g_HasOPCE) return zcmd_OnPlayerCommandPerformed(playerid, cmdtext, CallLocalFunction(funcname, "is", playerid, "\1"));
        return CallLocalFunction(funcname, "is", playerid, "\1");
    }   
    if (zcmd_g_HasOPCE) return zcmd_OnPlayerCommandPerformed(playerid, cmdtext, CallLocalFunction(funcname, "is", playerid, cmdtext[pos]));
    return CallLocalFunction(funcname, "is", playerid, cmdtext[pos]);
}

I compiled everything with debug labels but crashdetect still outputs the memory address only.

I'm not reporting about zcmd here. I want to ask why can players send an empty command to the server? I guess it is a vulnerability of the server.

[ev0lution]

CallLocalFunction crashes if passing an empty string, which is what it's trying to do on line 87.

Additionally, line 92 will be trying to check "cmdtext[pos]" where pos = 1, an invalid index of the array since it's an empty string.

I suppose zcmd doesn't handle empty strings in OnPlayerCommandText because it should never receive one - it is indeed a bug in the server. To protect against this you could use something like "if(isnull(cmdtext)) return 0;" at the top of OnPlayerCommandText... and ideally ban anyone who's trying to break your server!

[AndreT]

Thanks for the tip, ev0lution!

[leong124]

The server didn't crash on line 87(CallLocalFunction) but on line 92, although the command text is a null string. But it is impossible because you can access the array of an null string, which actually contains (null) in the string. Thus I doubt it can be an invalid memory access.

Even if the index of the string (i.e. A in a_string[A]) is invalid, it will only return a warning of "array index out of boundaries", but not invalid memory access and crash the server.
I will check it later.

[ev0lution]

Quote:

Originally Posted by leong124 The server didn't crash on line 87(CallLocalFunction) but on line 92, although the command text is a null string. But it is impossible because you can access the array of an null string, which actually contains (null) in the string. Thus I doubt it can be an invalid memory access. Even if the index of the string (i.e. A in a_string[A]) is invalid, it will only return a warning of "array index out of boundaries", but not invalid memory access and crash the server. I will check it later.

According to the logs it was just a runtime error (invalid memory access) on line 92 where it tried to access the out-of-bounds array element, and it crashed later on line 87. It's strange that it crashes on an earlier line but perhaps that's because it doesn't crash when CallLocalFunction is executed, rather when it gets to the receiving function (which presumably takes a few nanoseconds longer).

A "null string" in pawn is exactly the same as a string containing nothing - it's an array with one element, that being zero. Therefore in this case, cmdtext[0] is valid and would contain 0, but cmdtext[1] - which is trying to be accessed - is not.

Indeed it's strange that it's reported as "invalid memory access" rather than out of boundaries but I do believe that's the cause.

[Kar]

I edited my zcmd to fix these issues a long time ago

pawn Code:

public OnPlayerCommandText(playerid, cmdtext[])
{
    if (isnull(cmdtext))
    {
        // You can add other crash detect/warning code here.
        return 0;
    }
    if (!CallLocalFunction("OnPlayerCommandReceived", "is", playerid, cmdtext))
    {
        return 1;
    }
    new pos, funcname[MAX_FUNC_NAME];
    while (cmdtextt[++pos] > ' ')
    {
        if(pos > MAX_FUNC_NAME) break;
        funcname[pos-1] = tolower(cmdtextt[pos]);
    }
    format(funcname, sizeof(funcname), "cmd_%s", funcname);
    while (cmdtextt[pos] == ' ') pos++;
    if (!cmdtextt[pos])
    {
        return CallRemoteFunction("OnPlayerCommandPerformed", "isi", playerid, cmdtextt, CallLocalFunction(funcname, "is", playerid, "\1"));
    }
    return CallRemoteFunction("OnPlayerCommandPerformed", "isi", playerid, cmdtextt, CallLocalFunction(funcname, "is", playerid, cmdtextt[pos]));
}

[Lorenc_]

You should read the ZCMD topic, ****** posted a fix month(s) ago also, I posted my very own fix to commands being over the maximum character limit which also posts a crashdetect msg.

Use the one Kar has provided, it is up-to-date in a way where no crashdetect warnings would be printed.

[leong124]

Ok I'll try to add those code, thanks.
But I still can't understand why it crashes like that.
I made a test some days ago:

pawn Code:

public OnFilterScriptInit()
{
    test("");
    return 1;
}

forward test(aaa[]);
public test(aaa[])
{
    print(aaa[1]);
    return 1;
}

Nothing goes wrong and "(null)" is output. I know strings have size and out of bounds errors can occur, but I guess the size of the string is unknown(dynamic?) here. Usually CallLocalFunction will crash instantly as SA-MP server is single-threaded. The code is ran in sequence that tasks must be done before going for another. If it doesn't crash at that moment it won't crash suddenly later, right?

Anyway, that's another weird problem I faced. I'll try to make the protection anyway, but I just can't understand why it happens. I also do not have any chance to test the code, though I hope no one will try to test it.

[Jupit3r]

it looks like the problem was in the sa-mp internal RPC, not in ZCMD, anyway reading/putting a value out of memory don't make a crash everytimes.

[Lorenc_]

AFAIK, it was introduced in 0.3e

[leong124]

Some help for that from the dev team is appreciated.
I got some crasher like that and the script here blocks it. Some told me that there are some more kinds of such crasher and the isnull check does nothing to the hack, but I can't find one to test.

[ikkentim]

Quote:

Originally Posted by leong124 Ok I'll try to add those code, thanks. But I still can't understand why it crashes like that. I made a test some days ago: pawn Code: public OnFilterScriptInit() {     test("");     return 1; } forward test(aaa[]); public test(aaa[]) {     print(aaa[1]);     return 1; } Nothing goes wrong and "(null)" is output. I know strings have size and out of bounds errors can occur, but I guess the size of the string is unknown(dynamic?) here. Usually CallLocalFunction will crash instantly as SA-MP server is single-threaded. The code is ran in sequence that tasks must be done before going for another. If it doesn't crash at that moment it won't crash suddenly later, right? Anyway, that's another weird problem I faced. I'll try to make the protection anyway, but I just can't understand why it happens. I also do not have any chance to test the code, though I hope no one will try to test it.

That doesn't couse a crash no. This does:

Code:

public OnFilterScriptInit()
{
    new empty[16];//not assigning a string, then it is null. If formatting it as "" it will add the /0 character to the first byte

    CallLocalFunction(test,"s",empty);//afaik print doesn't crash with empty strings, calllocalfunction does
    return 1;
}

forward test(aaa[]);
public test(aaa[])
{
    print(aaa);
    return 1;
}

[leong124]

I'm not talking about the crash on CallLocalFunction.
I'm trying to prove that accessing some array index, which is larger than the length of the string contained, will not crash the server. That's why I send an empty string into a function of unknown size and try to display the second cell of it, which only shows a null element but not crashing.

[mastermax7777]

im using zcmd.. how do i know if i have this same problem? (in other words.. how do i crash my server?? )

[leong124]

Maybe you try to search with google, but those crappy hacks are blocked by the isnull check in the previous page.

[Karlip]

The problem seems to have been fixed in the first RC build of 0.3x.

Quote:

- The potential for a player to send a 0-length command has been addressed.

For the blind: https://sampforum.blast.hk/showthread.php?tid=405580

[leong124]

I'll test that when I have time.

[kurta999]

This not fixed.

pawn Code:

public OnFilterScriptInit()
{
    new empty[16];//not assigning a string, then it is null. If formatting it as "" it will add the /0 character to the first byte

    CallLocalFunction("test","s",empty);//afaik print doesn't crash with empty strings, calllocalfunction does
    return 1;
}

forward test(aaa[]);
public test(aaa[])
{
    print(aaa);
    return 1;
}

[leong124]

Quote:

Originally Posted by kurta999 This not fixed. pawn Code: public OnFilterScriptInit() {     new empty[16];//not assigning a string, then it is null. If formatting it as "" it will add the /0 character to the first byte     CallLocalFunction("test","s",empty);//afaik print doesn't crash with empty strings, calllocalfunction does     return 1; } forward test(aaa[]); public test(aaa[]) {     print(aaa);     return 1; }

That has been a problem for a very long time. It can be easily fixed by adding an isnull check before CallLocal/RemoteFunction. What I talk about here is that cheaters can send 0-length command (even without the slash) to the server, causing weird crashes.

As far as we discussed here, such attack can be easily blocked by insull check before processing your commands. I even made it to ban the cheater if the command checked is null. I tested my script with similar empty command hacks that can be easily found, and it successfully blocks the attack.

It is good if it is fixed in 0.3x, but I think we can't detect if anybody try to attack the server.