Skip to content

fs/binfmt: close symlink TOCTOU and harden setuid/setgid exec hygiene - #19585

Open
Abhishekmishra2808 wants to merge 2 commits into
apache:masterfrom
Abhishekmishra2808:fs/exec-security-toctou-hygiene
Open

fs/binfmt: close symlink TOCTOU and harden setuid/setgid exec hygiene#19585
Abhishekmishra2808 wants to merge 2 commits into
apache:masterfrom
Abhishekmishra2808:fs/exec-security-toctou-hygiene

Conversation

@Abhishekmishra2808

Copy link
Copy Markdown
Contributor

Summary

This change closes a symlink TOCTOU race in the pseudo-filesystem and hardens setuid/setgid exec identity handling. Parent-directory permission checks are performed inside inode_reserve() and inode_remove() while the inode tree lock is held, and that lock is kept across pseudorename, so a symlink cannot be swapped between the check and the mutation. On setuid/setgid exec, saved set-IDs are updated, the task group is marked secure, unsafe environment variables are sanitized, and debug/dumpable state is cleared. Support is also added for issetugid(), secure_getenv(), and PR_SET_DUMPABLE / PR_GET_DUMPABLE, gated on CONFIG_SCHED_USER_IDENTITY.

Impact

Configs that enable CONFIG_FS_PERMISSION and CONFIG_PSEUDOFS_SOFTLINKS now enforce directory permissions under the inode lock, which closes the previous check-then-act window around symlink targets. Configs with CONFIG_SCHED_USER_IDENTITY get stricter setuid/setgid exec behavior, including saved-ID updates, environment sanitization, and dumpable/secure flag handling. When those options are disabled there is no intended behavior change, and stock sim:nsh still builds. Applications that previously relied on inheriting variables such as PATH or LD_* across a setuid exec may see those cleared; that is intentional.

Testing

multiuser_test completed with 0 failures, covering UID/GID switching, saved-set child semantics, pseudoFS and tmpfs permission enforcement, and passwd lookup after credential drop. Full ostest exited with status 0.

user_main: multi-user test
multiuser_test: start
multiuser: effective UID/GID switching
  PASS: initial uid == 0
  PASS: initial euid == 0
  PASS: initial gid == 0
  PASS: initial egid == 0
  PASS: root seteuid(1000)
  PASS: euid after seteuid(1000) == 1000
  PASS: root seteuid(0) restore
  PASS: euid restored to 0 == 0
  PASS: root setegid(2000)
  PASS: egid after setegid(2000) == 2000
  PASS: root setegid(0) restore
  PASS: egid restored to 0 == 0
multiuser: saved set-UID/GID semantics (child task)
multiuser_suid_child: saved set-UID/GID semantics
  PASS: setuid(1000) as root
  PASS: uid after setuid(1000) == 1000
  PASS: euid after setuid(1000) == 1000
  PASS: non-root seteuid(0) denied errno=1
  PASS: euid unchanged after denied seteuid(0) == 1000
  PASS: non-root seteuid(1000)
  PASS: root-group setegid(2000)
  PASS: egid after setegid(2000) == 2000
  PASS: root-group setegid(0) restore
  PASS: setgid(3000)
  PASS: gid after setgid(3000) == 3000
  PASS: egid after setgid(3000) == 3000
  PASS: non-root setegid(0) denied errno=1
  PASS: egid unchanged after denied setegid(0) == 3000
multiuser_suid_child: 0 failure(s)
  PASS: mu_suid child completed successfully
  PASS: parent euid after child == 0
  PASS: parent egid after child == 0
multiuser: pseudoFS chmod/chown/open permissions
  PASS: /ostest_mu_perm owner 0:0
  PASS: root chmod(0600)
  PASS: non-owner chmod(0777) denied errno=1
  PASS: non-root chown(0,0) denied errno=1
  PASS: root chown to 1000:1000
  PASS: /ostest_mu_perm owner 1000:1000
  PASS: owner chmod(0777)
  PASS: /ostest_mu_secret owner 0:0
  PASS: open(/ostest_mu_secret) denied with EACCES
  PASS: open(/ostest_mu_secret) allowed
  PASS: /ostest_mu_user owner 1000:1000
  PASS: open(/ostest_mu_user) allowed
multiuser: tmpfs open permission enforcement
  PASS: /tmp/ostest_mu_secret owner 0:0
  PASS: open(/tmp/ostest_mu_secret) denied with EACCES
  PASS: open(/tmp/ostest_mu_secret) allowed
multiuser: passwd lookup after credential drop
  PASS: provisioned /tmp/ostest_passwd
  PASS: getpwnam(root) uid=0
  PASS: getpwnam(testuser) uid=1000
multiuser_test: 0 failure(s)

@github-actions github-actions Bot added Size: L The size of the change in this PR is large Area: BINFMT labels Jul 30, 2026
Perform pseudo-filesystem permission checks inside inode_reserve() and
inode_remove() while the inode tree lock is held, and hold that lock across
pseudorename mutations so symlink swaps cannot bypass directory checks.

On setuid/setgid exec, update saved set-IDs, mark the task group secure,
sanitize dangerous environment variables, clear debug/dumpable flags, and
add issetugid(), secure_getenv(), and PR_SET/GET_DUMPABLE support.

Signed-off-by: Abhishek Mishra <mishra.abhishek2808@gmail.com>
@Abhishekmishra2808
Abhishekmishra2808 force-pushed the fs/exec-security-toctou-hygiene branch 2 times, most recently from cb067ec to 5576e21 Compare July 30, 2026 20:42
Gate PR_SET/GET_DUMPABLE on CONFIG_SCHED_USER_IDENTITY, restore rename
source-parent permission checks under the inode lock, hold a read lock
around pseudo-fs open permission checks, and provide issetugid() stubs
when user identity is disabled.

Signed-off-by: Abhishek Mishra <mishra.abhishek2808@gmail.com>
@Abhishekmishra2808
Abhishekmishra2808 force-pushed the fs/exec-security-toctou-hygiene branch from 5576e21 to 633e7d8 Compare July 30, 2026 20:50
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

MemBrowse Memory Report

arduino-mega2560

  • flash: .text +14 B (+0.0%, 67,444 B / 262,144 B, total: 26% used)

esp32-devkitc

  • ROM: .flash.text +20 B (+0.0%, 124,392 B / 4,194,272 B, total: 3% used)
  • irom0_0_seg: .flash.text +20 B (+0.0%, 88,600 B / 3,342,304 B, total: 3% used)

hifive1-revb

  • flash: .text +28 B (+0.0%, 83,380 B / 4,194,304 B, total: 2% used)

mirtoo

  • kseg0_progmem: .text +72 B (+0.1%, 67,508 B / 131,072 B, total: 52% used)

qemu-armv8a

  • Code: .text.file_vopen +36 B, .text.group_allocate +12 B, .text.mkdir -24 B, .text.nx_unlink -28 B, .text.rename +16 B (+0.0%, 318,134 B)

qemu-intel64

  • Code: .text -19 B (-0.0%, 8,658,029 B)

s698pm-dkit

  • Code: .text +112 B (+0.0%, 363,360 B)

stm32-nucleo-f103rb

  • flash: .text +36 B (+0.1%, 33,964 B / 131,072 B, total: 26% used)
    No memory changes detected for:
  • rx65n-rsk2mb

@github-actions github-actions Bot added Size: M The size of the change in this PR is medium and removed Size: L The size of the change in this PR is large labels Jul 30, 2026
@Abhishekmishra2808

Copy link
Copy Markdown
Contributor Author

Hi, the build is failing in mainline.
It’s a LAME link error (*_avx512 undefined) from apps, likely a CMake gap after nuttx-apps#3666 (SSE2/AVX2 added, AVX512 still enabled by configure).

@casaroli, please help us to add AVX512 vector sources in the sim/CMake LAME build. Thanks!

@xiaoxiang781216 and @acassis PTAL.

@casaroli

casaroli commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Hi, the build is failing in mainline. It’s a LAME link error (*_avx512 undefined) from apps, likely a CMake gap after nuttx-apps#3666 (SSE2/AVX2 added, AVX512 still enabled by configure).

@casaroli, please help us to add AVX512 vector sources in the sim/CMake LAME build. Thanks!

@xiaoxiang781216 and @acassis PTAL.

I can fix the cmake file again, however the root problem is that lame revision is not pinned:

So when they update, sometimes it breaks:

I believe our options are:

  1. Pin lame to r6718
  2. Pin lame to r6720 and fix the CMakeLists.txt once more
  3. Do not Pin lame and keep fixing CMakeLists.txt forever
  4. Try to contribute to lame so their upstream cmake build can be just included / called from us. Or to find a way we can call build cmake library projects from Makefile

I think 3 is bad decision

I recommend we do (1) -> (4); or (2) -> (4)

What do you think?

Ref: https://sourceforge.net/p/lame/svn/HEAD/log/?path=/trunk/lame/libmp3lame/vector

@casaroli

Copy link
Copy Markdown
Contributor

apache/nuttx-apps#3684

@Abhishekmishra2808

Copy link
Copy Markdown
Contributor Author

Agreed.

Pinning to a known revision and updating both Make and CMake to match it (option 2) is the better approach (IMO). Keeping CMake in sync with trunk all the time isn't sustainable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: BINFMT Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants