search

d0k3 / GodMode9

GodMode9 Explorer - A full access file browser for the Nintendo 3DS console :godmode:
GNU General Public License v3.0
2.12k stars 191 forks source link

Preserve RSA keys in trimmed NTR dumps #722

Closed lifehackerhansol closed 3 years ago

lifehackerhansol commented 3 years ago

Fixes #721, implemented exactly as described in the issue.

This was tested on a Mario Kart DS (Korea) cartridge, while being played on nds-bootstrap and an original R4, where cloneboot requires the RSA key. It is useful for real hardware NDS playback as a whole.

I unfortunately do not have a TWL cart to test the fix there. If anyone else is willing to test, or have any suggestions about what to add to this PR, feel free to let me know or directly commit.

lifehackerhansol commented 3 years ago

Force push was to change the author email. Nothing else changed.

lifehackerhansol commented 3 years ago

There have been some points brought up mid-testing with some members in the community as to whether it is actually worthwhile to keep the appended 0x88 in TWL carts. There isn't a known TWL cart (as in DSi Enhanced or DSi Exclusive) that utilizes cloneboot at all, so it may just be unneeded extra data (and may not actually contain valid data) which goes against the purpose of trim dumps.

In GodMode9i, where this was first implemented (mentioned in the related issue), I don't think there is a particular check for NTR or TWL dumps. In my opinion if it'll be done on one cart it should be done for all, but I think it's better for someone else to decide this.

I've left this ability intact to dump the 0x88 in TWL dumps. If this is to be removed, let me know, or feel free to directly commit before merge.

SNBeast commented 3 years ago

There is a check for NTR vs. TWL dumps in GodMode9i, and it's in the code snippet in the linked issue above: ndsCardHeader.unitCode != 0. The unit code is 0 for NTR, 2 for TWL with NTR support, and 3 for TWL only.

lifehackerhansol commented 3 years ago

I see, so the trim+0x88 only applies when the unit code is 0, then? I may have misunderstood the code the first time I saw.

SNBeast commented 3 years ago

Yes. + has a higher operator precedence than ? and :, so the choices read "ndsCardHeader.twlRomSize if true, ndsCardHeader.romSize + 0x88 if false."

lifehackerhansol commented 3 years ago

Yeah not sure how I messed that up. It's gone now.

lifehackerhansol commented 3 years ago

At the moment, this commit freezes GodMode9, though it is implemented as described in the issue. I'll try again.

d0k3 commented 3 years ago

Should not freeze GM9. Make sure you've got the latest master as base. Also take note that appending + 0x88 to that exact location is what I did earlier. According to @SNBeast this is not correct.

lifehackerhansol commented 3 years ago

Should not freeze GM9. Make sure you've got the latest master as base. Also take note that appending + 0x88 to that exact location is what I did earlier. According to @SNBeast this is not correct.

Apparently, it doesn't work when reading from C: GAMECART. It does however, preserve the key on an existing ROM on the SD.

So the trim size operation is not working straight off the cart.

The crash issue I've found was just a weird build quirk. After a make clean it didn't crash.

d0k3 commented 3 years ago

Alright, I forgot about that one actually, that's a different place in the code. Maybe your initial fix wasn't so bad. You need to make sure, tough, that the data size never exceeds the cart size.

d0k3 commented 3 years ago

Don't want to bother, but do you by any chance have a new build that can be tested? Sorry about the earlier confusion, your initial fix was actually the correct way, it just needs an additional sanity check build in. If you need more info, just ask.

lifehackerhansol commented 3 years ago

I haven't had time to do more commits. It's been on my mind for quite a while though, I might revert the changes again.

I have absolutely no idea where I'd place the checks, though.

d0k3 commented 3 years ago

It belongs right below where you made the data_size change. data_size + 32 may, in some rare cases, exceed cart_size, causing a lot of trouble. You just make sure data_size is never more than cart_size.

lifehackerhansol commented 3 years ago

Isn't that check already done in https://github.com/d0k3/GodMode9/blob/4dc96d37e84e341cf8e0aecc1a26c1e0f84b4004/arm9/source/gamecart/gamecart.c#L274 ? Or do you want more safety checks?

I ask because that line is exactly the safety check I was going to write, and then I saw that.

lifehackerhansol commented 3 years ago

Kind of off-topic, but I noticed local builds seem to produce smaller binaries... but I assume the end result would be the same, so.

The check is removed, and yeah, I did a poor job of it first time. Here goes.

d0k3 commented 3 years ago

Merged, thank you!