Skip to content

fix crash in profile loading#272

Merged
isage merged 1 commit intonxengine:masterfrom
FridayOrtiz:master
Jul 18, 2022
Merged

fix crash in profile loading#272
isage merged 1 commit intonxengine:masterfrom
FridayOrtiz:master

Conversation

@FridayOrtiz
Copy link
Contributor

There are two crashes/segfaults in profile_load which could be turned into arbitrary writes. First, the fgetl and fgeti functions don't check if you've reached the end of the file, which causes a too-small profile.dat to fill the Profile with whatever garbage stack values and leads to a crash, if you call profile_load directly.

00000000: 446f 3034 3132 3230                      Do041220

In the next crash we set the item type to some large value, like 0x5C000000.

00000000: 446f 3034 3132 3230 0d00 0000 0800 0000  Do041220........
00000010: 2de6 0100 20e0 0000 0200 0000 0300 0000  -... ...........
00000020: 0300 0000 0000 0000 0000 0000 0000 0000  ................
00000030: 0000 0000 0000 0000 0000 005c 5c5c 5c5c  ...........\\\\\
00000040: 5c5c 5c5c 5c5c 5c5c 5c5c 5c5c            \\\\\\\\\\\\

When we reach the following code block, we'll attempt to write to some large offset and most likely segfault.

    int level   = fgetl(fp);
    int xp      = fgetl(fp);
    int maxammo = fgetl(fp);
    int ammo    = fgetl(fp);

    file->weapons[type].hasWeapon = true;
    file->weapons[type].level     = (level - 1);
    file->weapons[type].xp        = xp;
    file->weapons[type].ammo      = ammo;
    file->weapons[type].maxammo   = maxammo;

This patch fixes the two crashes by adding a check to the fgeti and fgetl functions and making sure the type is within the maximum number of weapons.

Note: I have no idea if this will break existing saves in some way, but I don't think it will. I have a similar PR open in the NXEngine repo: EXL/NXEngine#9.

@isage isage merged commit d4bd21e into nxengine:master Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants