Issue Information
-
#004982
-
5 - Critical
-
Fixed
Issue Confirmations
-
Yes (0)No (0)
0
Originally posted by theultramage
http://www.eathena.w...er&showbug=4982
There is a copy-paste typo in function quest_add() that trashes the in-memory quest index of all already finished quests. If the index is used without re-logging, it will cause an out of bounds read, possibly resulting in a server crash.
The questlog array is split into two parts - active quests, followed by completed quests. When adding a new quest, all completed quests are shifted one array position to the right using memmove(). The same thing is then done to the quest_index array, which is paired with the questlog array.
The typo was introduced 2 years ago in r14039 as a followup to r14038. The reason why it wasn't discovered earlier is, that the index corruption only affects already completed entries, and that the index is actually used only in 2 places, one of which only processes active entries. And even the remaining one only executes vulnerable code as a special case.
To actually execute this, you need to 1) start a hunting quest, 2) have it marked as completed, but not delete it, 3) start another quest without relogging, 4) execute checkquest(HUNTING) on the already completed quest id. These are very special circumstances - however, that's exactly what the renewal novice ground script does.
PS: here's my reproduction script (might not actually crash, I used a debugger to verify the issue).
PS: Crash was reported by Gepard in #athena.
This post has been edited by theultramage: Jun 26 2011, 10:02 AM
http://www.eathena.w...er&showbug=4982
There is a copy-paste typo in function quest_add() that trashes the in-memory quest index of all already finished quests. If the index is used without re-logging, it will cause an out of bounds read, possibly resulting in a server crash.
QUOTE
memmove(&sd->quest_log[i+1], &sd->quest_log[i], sizeof(struct quest)*(sd->num_quests-sd->avail_quests));
memmove(sd->quest_index+i+1, sd->quest_log+i, sizeof(int)*(sd->num_quests-sd->avail_quests));
memmove(sd->quest_index+i+1, sd->quest_log+i, sizeof(int)*(sd->num_quests-sd->avail_quests));
The questlog array is split into two parts - active quests, followed by completed quests. When adding a new quest, all completed quests are shifted one array position to the right using memmove(). The same thing is then done to the quest_index array, which is paired with the questlog array.
The typo was introduced 2 years ago in r14039 as a followup to r14038. The reason why it wasn't discovered earlier is, that the index corruption only affects already completed entries, and that the index is actually used only in 2 places, one of which only processes active entries. And even the remaining one only executes vulnerable code as a special case.
CODE
int quest_check(TBL_PC * sd, int quest_id, quest_check_type type)
{...
case HUNTING:
ARR_FIND(0, MAX_QUEST_OBJECTIVES, j, sd->quest_log[i].count[j] < quest_db[sd->quest_index[i]].count[j]);
{...
case HUNTING:
ARR_FIND(0, MAX_QUEST_OBJECTIVES, j, sd->quest_log[i].count[j] < quest_db[sd->quest_index[i]].count[j]);
To actually execute this, you need to 1) start a hunting quest, 2) have it marked as completed, but not delete it, 3) start another quest without relogging, 4) execute checkquest(HUNTING) on the already completed quest id. These are very special circumstances - however, that's exactly what the renewal novice ground script does.
PS: here's my reproduction script (might not actually crash, I used a debugger to verify the issue).
CODE
prontera.gat,145,175,4 script Test Poring 909,{
// cleanup
erasequest 1001;
erasequest 60107; // "Hunting Fabres"
// setup
setquest 60107;
completequest 60107;
// corrupt
setquest 1001;
// crash
checkquest(60107,HUNTING);
}
// cleanup
erasequest 1001;
erasequest 60107; // "Hunting Fabres"
// setup
setquest 60107;
completequest 60107;
// corrupt
setquest 1001;
// crash
checkquest(60107,HUNTING);
}
PS: Crash was reported by Gepard in #athena.
This post has been edited by theultramage: Jun 26 2011, 10:02 AM
