[Issue Report]: Unvalidated spell ID from save data causes incorrect indexing in GenerateStaffNameMagical()

[ACavalletto]

Operating System

Linux x86

DevilutionX version

1.5.5

Describe

The item spell ID read from save data is passed directly into GenerateStaffNameMagical()(Source/items.cpp) without any validation. An invalid spell ID can reach the staff-name generation logic and incorrectly index into spell data tables, leading to undefined behavior, a crash, or corrupted item names.

To Reproduce

1. Craft or modify a save file so that an item's spell ID field holds an invalid or out-of-range value.
2. Load the save file in DevilutionX.
3. Allow the game to generate or display the name of a magical staff that references the corrupted spell ID.
4. Observe a crash, garbled item name, or incorrect spell data being read.

Expected Behavior

The spell ID loaded from save data should be validated before use. Any value that does not correspond to known, valid spell should be remapped to SpellID::Null so that the staff-name generation logic handles it safely and predictably.

Additional context

• Vulnerable function: GenerateStaffNameMagical() in Source/items.cpp
• The root cause is a missing validation step after deserializing the spell ID from save data.
• Suggested fix: after loading the spell ID, check whether it falls within the valid SpellID enum range; if not, assign SpellID::Null before passing it to any downstream logic.

[ACavalletto]

@Jacekhacking and @coopons also helped find this bug.

[yuripourre]

@ACavalletto by any chance do you have a savegame file?

[ACavalletto]

We setup a fuzzing harness to mutate over the loadplayer and spawn. So our files are headless and were built around the testing modules. What are you looking for specifically so we can see if we can provide you that? @yuripourre

[yuripourre]

We setup a fuzzing harness to mutate over the loadplayer and spawn. So our files are headless and were built around the testing modules. What are you looking for specifically so we can see if we can provide you that? @yuripourre

@ACavalletto thanks for taking time to explain and report the issue. I was mostly wondering what legit situations could be cause this to happen other than the crafted save game file.

I know DevilutionX don't have validation for all the fields and if they can only break the player that crafted that file, (as a contibutor of this project, not admin/maintainer) is less concerning to me. In any case, it's good to have it documented.

[StephenCWills]

I was mostly wondering what legit situations could be cause this to happen other than the crafted save game file.

Because of the encryption, it's unlikely this would be caused by unintentional data corruption on the file system. That type of issue would be more likely to result in a decoding error, which would cause the game client to skip loading the save file in the character select screen.

There are some legit scenarios for malicious save files.

• The user runs malicious third-party software that targets our save files.
• The user downloads a malicious save file from the internet.
• The user synchronizes their save file with a cloud service, and the cloud service is compromised.
• The user shares save files between themselves and a malicious user.
• If DevilutionX ever manages to implement client/server where save files are stored on the server...
  • A malicious user could inject invalid data into their save file in an attempt to hack the server.
  • A malicious user could first hack the server and then inject invalid data into player save files in an attempt to hack those player's computers.

Currently, I don't think we really need to worry about malicious third-party software targeting our save files, and private servers could potentially use an entirely different save format. Uploading/downloading/sharing save files is a fairly common occurrence so that could be a bit of a concern.

That said, given that we currently don't do much (or any?) validation at all when loading save data, I do wonder if it would be better to track save file validation as an aggregate issue rather than having multiple issues to track each individual field that isn't being validated.