WIP: Hardening: guest accesses masked into the guest window; file system paths kept inside their mounts #880
Draft
hckrman101 wants to merge 3 commits from
hckrman101/harden-mask-paths into main
pull from: hckrman101/harden-mask-paths
merge into: MagicBeanDev:main
MagicBeanDev:main
MagicBeanDev:github/pr/2893
MagicBeanDev:github/pr/2890
MagicBeanDev:github/pr/2887
MagicBeanDev:github/pr/2878
MagicBeanDev:github/pr/2875
MagicBeanDev:github/pr/2874
MagicBeanDev:github/pr/2873
MagicBeanDev:github/pr/2871
MagicBeanDev:github/pr/2756
MagicBeanDev:github/pr/2897
MagicBeanDev:github/pr/2896
MagicBeanDev:github/pr/2718
MagicBeanDev:github/pr/2842
MagicBeanDev:github/pr/2691
MagicBeanDev:github/pr/2895
MagicBeanDev:github/pr/2743
MagicBeanDev:github/pr/2765
MagicBeanDev:github/pr/2708
MagicBeanDev:github/pr/2732
MagicBeanDev:github/pr/2755
MagicBeanDev:github/pr/2817
MagicBeanDev:github/pr/2876
MagicBeanDev:release-please--branches--main
MagicBeanDev:release-please--branches--main--release-notes
MagicBeanDev:github/pr/2778
MagicBeanDev:github/pr/2888
MagicBeanDev:github/pr/2866
MagicBeanDev:github/pr/2877
MagicBeanDev:github/pr/2800
MagicBeanDev:github/pr/2829
MagicBeanDev:github/pr/2850
MagicBeanDev:github/pr/2889
MagicBeanDev:github/pr/2882
MagicBeanDev:github/pr/2860
MagicBeanDev:github/pr/2849
MagicBeanDev:github/pr/2886
MagicBeanDev:github/pr/2794
MagicBeanDev:github/pr/2840
MagicBeanDev:github/pr/2830
MagicBeanDev:github/pr/2834
MagicBeanDev:github/pr/2719
MagicBeanDev:github/pr/2742
MagicBeanDev:github/pr/2741
MagicBeanDev:github/pr/2820
MagicBeanDev:github/pr/2771
MagicBeanDev:github/pr/2859
MagicBeanDev:github/pr/2858
MagicBeanDev:github/pr/2823
MagicBeanDev:github/pr/2757
MagicBeanDev:github/pr/2769
MagicBeanDev:github/pr/2844
MagicBeanDev:github/pr/2843
MagicBeanDev:github/pr/2673
MagicBeanDev:github/pr/2855
MagicBeanDev:github/pr/2739
MagicBeanDev:github/pr/2807
MagicBeanDev:github/pr/2851
MagicBeanDev:github/pr/2789
MagicBeanDev:github/pr/2814
MagicBeanDev:github/pr/2818
MagicBeanDev:github/pr/2774
MagicBeanDev:github/pr/2772
MagicBeanDev:github/pr/2750
MagicBeanDev:github/pr/2786
MagicBeanDev:github/pr/2785
MagicBeanDev:github/pr/2782
MagicBeanDev:github/pr/2779
MagicBeanDev:github/pr/2775
MagicBeanDev:github/pr/2706
MagicBeanDev:github/pr/2766
MagicBeanDev:github/pr/2747
MagicBeanDev:github/pr/2759
MagicBeanDev:github/pr/2745
MagicBeanDev:github/pr/2740
MagicBeanDev:github/pr/2733
MagicBeanDev:github/pr/2725
MagicBeanDev:github/pr/2727
MagicBeanDev:github/pr/2728
MagicBeanDev:github/pr/2729
MagicBeanDev:github/pr/2721
MagicBeanDev:github/pr/2722
MagicBeanDev:github/pr/2715
MagicBeanDev:github/pr/2714
MagicBeanDev:github/pr/2707
MagicBeanDev:github/pr/2703
MagicBeanDev:github/pr/2705
MagicBeanDev:github/pr/2697
MagicBeanDev:github/pr/2679
MagicBeanDev:github/pr/2676
MagicBeanDev:github/pr/2669
MagicBeanDev:docs/what-hookshot-connects-to
MagicBeanDev:fix/hdr-encode-test
MagicBeanDev:fix/main-tests
MagicBeanDev:fix/play-cache-files
MagicBeanDev:perf/game-barrier-order
MagicBeanDev:feat/heap-lazy
MagicBeanDev:feat/texture-detail
MagicBeanDev:perf/astc-opaque-bc1
MagicBeanDev:fix/fs-dir-lock
MagicBeanDev:fix/framegen-native-kept
MagicBeanDev:fix/update-no-downgrade
MagicBeanDev:port/k10-gcn-f16-mirrors
MagicBeanDev:fix/framegen-mult-cap
MagicBeanDev:chore/release-as-2.1.1
MagicBeanDev:perf/pcache-load-switch
MagicBeanDev:fix/prepare-progress-log
MagicBeanDev:fix/update-closes-stale-game
MagicBeanDev:fix/hdr-scaled-sampled
MagicBeanDev:perf/first-start-preset
MagicBeanDev:perf/zero-upload
MagicBeanDev:fix/vcore-escape-2s
MagicBeanDev:test/fake-gh-atomic
MagicBeanDev:perf/batch-idle-flush
MagicBeanDev:test/finish-stub-a
MagicBeanDev:chore/record-2.1.0
MagicBeanDev:fix/main-red-tests
MagicBeanDev:fix/test-display-startup-driver-cache
MagicBeanDev:feat/mesh-shader-mode
MagicBeanDev:fix/menu-probes-at-startup
MagicBeanDev:fix/fps-counter-classic
MagicBeanDev:fix/test-log-batch-flake
MagicBeanDev:fix/narrow-screen-hud
MagicBeanDev:ci/prereleases
MagicBeanDev:release/v2.0.15
MagicBeanDev:hotfix/v2.0.15
MagicBeanDev:fix/present-ahead-replay
MagicBeanDev:release/v2.0.13
MagicBeanDev:fix/bracket-paths-2013
MagicBeanDev:fix/hdr-overlays-2013
MagicBeanDev:fix/fps-counter-scale-2013
MagicBeanDev:fix/stick-seen-at-rest
MagicBeanDev:hotfix/menu-fits-screen
MagicBeanDev:fix/f1-always-opens-menu
MagicBeanDev:fix/ini-keep-lines
MagicBeanDev:claude/cranky-pike-7cf247
MagicBeanDev:perf/texture-answer-flat
MagicBeanDev:fix/stamp-test-modules
MagicBeanDev:fix/appimage-fonts
MagicBeanDev:fix/display-awake
MagicBeanDev:release/v2.0.12
MagicBeanDev:fix/discord-windows
MagicBeanDev:rc/v2.0.12-tests
MagicBeanDev:fix/dlss-keep-feature
MagicBeanDev:rc/v2.0.12
MagicBeanDev:button-icons
No reviewers
Labels
Clear labels
accessibility
Barrier affecting people with disabilities
autorelease: pending
autorelease: tagged
bug
Something isn't working
dependencies
Pull requests that update a dependency file
documentation
Improvements or additions to documentation
duplicate
This issue or pull request already exists
enhancement
New feature or request
github_actions
Pull requests that update GitHub Actions code
good first issue
Good for newcomers
help wanted
Extra attention is needed
invalid
This doesn't seem right
question
Further information is requested
testing wanted
wontfix
This will not be worked on
No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Hookshot!880
Loading…
Reference in a new issue
No description provided.
Delete branch "hckrman101/harden-mask-paths"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
A save file from someone else is data the game's own code parses. Should that code ever be led astray by a crafted file, the worst it should be able to do is crash the game. Two places let it do more:
1. Guest memory: every access masked into the 512 GB window
Lifted code and the host's
GP()turned a 64-bit guest address into a host pointer unchecked (A64_MEM + address), so an address the game's code computed from bad data could reach any host memory.A64_PTR(nx2/rtgen.py) masks the address to the window's 39 bits: an address past the window wraps into guest memory, never outside it.GP()isA64_PTR, so the host's own guest-pointer accesses are masked too.recomp_<op>_khelpers, generated by rtgen.py). Maskingbase + offsetas a whole cost an add and a mask per access; the split form costs one mask per base.The game's own addresses (image at 0x80000000, heap from 64 GB, regions down from 512 GB) all lie inside the window and are unchanged. Indirect branches were already limited to lifted function entries.
Cost, Linux, RTX 3090, Kokiri Forest (ft-fast, 45 s frame log, three runs each under the timing lock):
The run-to-run spread at this spot (36-52 fps for the same build) is larger than any difference between the builds; no slowdown shows.
A64_NO_WINDOW_MASK(measurement) andA64_NO_ADDR_SPLIT=1(lift time) turn the two parts off.2. File system: guest paths stay inside their mounts, no symbolic links
Paths reached the host as
<mount root>/<rest>unchecked, and the game builds some of them from what it reads.save:/../../xleft the save folder, and a save folder unpacked from an archive could hold symbolic links that the game would read and write through (DeleteDirectoryRecursively followed them too).host_pathrefuses a part inside the mount that is not plain names:.., a name of only dots and spaces (Windows trims...to..), a backslash, a colon (drive or stream).O_NOFOLLOWon POSIX (FILE_FLAG_OPEN_REPARSE_POINTin the POSIX CreateFileA).Normal paths behave as before.
tests/test_fs_paths.pybuilds the real nn_fs.c with the POSIX layer and checks normal saves, traversal, absolute parts, separators, colons and links (it fails on the old nn_fs.c).Tested
tests/run_all.py --strict(the three failures are the same on main: zstandard not installed, test_cpp_preflight).VirtualAllocMEM_RESERVE before and after the window) and the reparse-point check (FindFirstFileA'sdwReserved0).Thank you for this! It changes how the game runs at a level that needs a maintainer design decision first, so I'm closing it to keep the queue clear: needs maintainer design decision, reopen after discussion. (Masking every guest access touches all lifted code: performance and correctness need a design discussion first.)
Tested on Linux with an RTX 4070 Ti, merged locally onto current
main(9a2e80eplus #1422 and the others), using the complete v2.0.0+update dump.1. The build breaks after rebasing, because of the address split. The lift stops with:
nx2/oot_patches.pymatches literal lifted C, for example{ uint64_t _b=c->x[21]; int64_t _o=20; recomp_store64(c,(_b+_o),c->x[8]); }, andaddr_split.pyrewrites exactly that form. These patches (output size, text speed, dynamic resolution, mouse camera) landed after this branch's base. WithA64_NO_ADDR_SPLIT=1the same merge builds, so the split is the only cause. The patterns need the_kforms, or matching that accepts both.2.
host/nn_fs.cconflicts with main's own hardening.guest_path_ok(./..,\,:, control characters, and on Windows trailing dot/space and device names) plus the single snapshot copy of the guest path already coverrel_ok. What this PR adds on top islink_below/is_link/O_NOFOLLOWand the name-surrogate test indelete_tree. I resolved the merge as main'sguest_path_okand snapshot copy, then this PR'slink_belowafter the lookup, then this PR'sis_linkindelete_tree, which keeps OneDrive placeholders usable.3. Two test fixtures fail to build after the merge (fixture builds, not behaviour):
test_fs_paths.pyhas noHOS_SPACE_ENDstub for main'sgstr, and main'stest_fs_links.pydoesn't extractis_link, whichdelete_treenow calls.Runtime (the merge built with
A64_NO_ADDR_SPLIT=1, so masking on, same options file as main's build, DLSS Quality at 4K output, Hyrule Castle road, no mods):No slowdown that I can measure. In-game saving through System -> Save writes
slot_03/progress.sav,caption.sav,option.savandaccount.sav, with nofs: refusedlines. My test user folder is itself a symbolic link above the save mount, whichlink_belowcorrectly allows. I haven't tested the split form at runtime, because it doesn't build on main until point 1 is fixed.Updated onto
main(merge6e92a01; no force push).host/nn_fs.c:mainnow has its own path hardening, so its checks are kept as they are (guest_path_ok, the single copy of the guest path, links deleted rather than followed). The oldrel_okis gone. On top ofmain's checks, this PR still adds:link_belowon the resolved path, so a symbolic link or junction below a mount is refused (mod files exempt, as before);O_NOFOLLOWfor game file opens on POSIX;refused()log, now also for pathsguest_path_okrejects;delete_treetreats only name-surrogate reparse points (symbolic links, junctions) as links, so OneDrive placeholder folders are emptied, not skipped.Address split (Joey-Magnus's point 1): rewriting lifted C inside
emit_functionran beforemain'soot_patches.pyand page-mark grouping, which match the lift's exact text, so the lift failed.addr_splitis now a last pass inchunks.edit_c, after the patches and grouping. In the real lift everybase + constantaccess became a_kform, and the required patch check passed. Newtests/test_addr_split.pycovers the rewritten forms and the pass order, and compiles and runs every_khelper against the whole-address one, including wrap past the window.Masking coverage: every lifted access,
GP(), the exclusive/CAS helpers (includingmain's newercas_n) andA64_MARKstill go throughA64_PTR. The GPU back end's ownguest_base() + addruses stay out of scope.Tested:
tests/run_all.pyshows the same failures asmain.account.savwritten, nofs: refused, no crash.Not tested yet:
main.