dolphin-emu
Add Approved Patch Allowlist for Achievements
Prototype of a system to whitelist known game patches that are allowed to be used while RetroAchievements Hardcore mode is active. ApprovedInis.txt contains known hashes for the ini files as they appear in the repo, and can be compared to the local versions of these files to ensure they have not been edited locally by the player. ApprovedInis.txt is hashed and verified similarly first, with its hash residing as a const string within AchievementManager.h, ensuring ApprovedInis and the hashes within cannot be modified without editing Dolphin's source code and recompiling completely.
While the concept is reasonable, hashing the entire INI seems very restrictive.
If I'm reading the current version of the code correctly, if the user has edited the Sys or the User INI for any reason (e.g. to automatically load a specific controller profile upon loading the game, to apply custom graphics settings, to automatically enable/disable Dual Core or other settings), it will get blocked if Hardcore mode is enabled, even if such edits doesn't touch any settings restricted by it.
Ideally, only the [OnFrame] section (maybe [OnFrameEnabled] too) should be hashed/validated (perhaps parsing them and hashing the resulting object in memory, that way comments or other formatting differences can be ignored too), to still allow enforcing which patches can be loaded in Hardcore mode without completely blocking any other edit the user may have made to the INI.
Speaking of the ApprovedInis.txt, maybe I'm reading too much into it but having the only safety against tampering being a hash comparison with a known hash embedded in the source code doesn't seem exactly foolproof either. Yes, Dolphin would have to be recompiled in order to accept a tampered ApprovedInis.txt, but I feel like something external (read as "not as simple as just recompiling the code") should be enforcing the hash integrity too...
While I think it's fine to reject edited Sys INIs, as the user isn't supposed to edit those anyway, I do agree that hashing the patches rather than the whole INI would be cleaner.
To ensure nobody updates the INIs and forgets to update the hashes, how about adding a unit test that checks that all hashes match? I suppose this would require you to list file names in ApprovedInis.txt.
Speaking of the
ApprovedInis.txt, maybe I'm reading too much into it but having the only safety against tampering being a hash comparison with a known hash embedded in the source code doesn't seem exactly foolproof either. Yes, Dolphin would have to be recompiled in order to accept a tamperedApprovedInis.txt, but I feel like something external (read as "not as simple as just recompiling the code") should be enforcing the hash integrity too...
If the user recompiles Dolphin, the security model is screwed anyway. You could easily do something like telling the server hardcode mode is enabled when it isn't.
Good feedback, I'm glad this is a good first step. I'll look into how to apply it to the patches more directly. I got someone asking if there was a way to allow specific AR or Gecko codes so while that isn't critical at this time I'll try to consider it.
Speaking of the
ApprovedInis.txt, maybe I'm reading too much into it but having the only safety against tampering being a hash comparison with a known hash embedded in the source code doesn't seem exactly foolproof either. Yes, Dolphin would have to be recompiled in order to accept a tamperedApprovedInis.txt, but I feel like something external (read as "not as simple as just recompiling the code") should be enforcing the hash integrity too...
My exact personal cutoff for security is "if someone can cause problems without editing source and recompiling, block it". Honestly RetroAchievements' server is only so secure and it wouldn't be that difficult for someone to [REDACTED] if they know how to do relatively simple coding concepts, even writing a program completely from scratch. So I figure this is a reasonable endpoint to catch everyone who can't code. At that point, it's probably just less effort to play the damn game and earn it the normal way.
A more general question... is there any need for Gecko or AR codes in this system at all? I'm pretty sure all of our 'fix game' patches use our built-in patch format (can you find a counterexample?), and I'm looking at the code for AR codes here and I'm pretty sure the way you're handling these is very naive -- I haven't actually tested this, but I suspect I could enter a code while a game is running and it would not pass through the approval check, and for that matter it would probably end up deleting all my non-approved codes from my user ini if I modified the code list in hardcore mode. So to reduce complexity here, can we reduce this down to just Patches?
Also I just realized that you're re-parsing and re-hashing the entire approved code list for every single code you check. Please just do that once on AchievementManager init.
Also I just realized that you're re-parsing and re-hashing the entire approved code list for every single code you check. Please just do that once on AchievementManager init.
I suppose I could keep the list in memory, then - otherwise I'd have no real way to guarantee that the player didn't mess with it immediately before booting a game, which was my mindset for the current way.
A more general question... is there any need for Gecko or AR codes in this system at all? I'm pretty sure all of our 'fix game' patches use our built-in patch format (can you find a counterexample?), and I'm looking at the code for AR codes here and I'm pretty sure the way you're handling these is very naive -- I haven't actually tested this, but I suspect I could enter a code while a game is running and it would not pass through the approval check, and for that matter it would probably end up deleting all my non-approved codes from my user ini if I modified the code list in hardcore mode. So to reduce complexity here, can we reduce this down to just Patches?
I can for the time being but I know there's definitely people who have been bugging me about their precious 60fps widescreen hacks and I was under the impression that that's where those hacks were applied.
Sure, but is that essential to launching this feature? I mean we can do it, but I just hope you're aware that auditing three systems for any potential issues is a lot more work than just one.
I suppose I could keep the list in memory, then
Yes, obviously...? What else would you even suggest?
I suppose I could keep the list in memory, then
Yes, obviously...? What else would you even suggest?
Simply that this runs once upon startup of each game. I initially thought it would be preferable to allow that process to take a small amount of extra time for the benefit of not consuming that patch of unused memory when games need a lot of memory available. Apologies for being wrong.
Oh, running once per game is not a bad idea, if you can find a good time to actually unload it. But I wouldn't worry too much about it, this will take a few kilobytes at most.
And sorry if that came across as mean, that wasn't my intention, I just genuinely couldn't think of any other way to interpret my request.