From 350d697919ade843198c2bb93269eb118648b52f Mon Sep 17 00:00:00 2001
From: Aleksa Sarai <cyphar@cyphar.com>
Date: Fri, 13 Sep 2024 15:48:37 +1000
Subject: [PATCH] mkdirall: explicitly return an error for suid/sgid bits

These bits are ignored by mkdirat(2) on every POSIX system (Linux does
honour the sticky bit, though it is an outlier). We could ignore them
but this seems less than ideal (a user could be under the impression
their code works when actually the bit is not getting set) so returning
an informative error is preferable.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 CHANGELOG.md        |  9 +++++++++
 mkdir_linux.go      |  7 +++++++
 mkdir_linux_test.go | 11 +++++------
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7436896..0960cfb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased] ##
 
+### Changed ###
+- Passing the `S_ISUID` or `S_ISGID` modes to `MkdirAllInRoot` will now return
+  an explicit error saying that those bits are ignored by `mkdirat(2)`. In the
+  past a different error was returned, but since the silent ignoring behaviour
+  is codified in the man pages a more explicit error seems apt. While silently
+  ignoring these bits would be the most compatible option, it could lead to
+  users thinking their code sets these bits when it doesn't. Programs that need
+  to deal with compatibility can mask the bits themselves. (#23, #25)
+
 ## [0.3.1] - 2024-07-23 ##
 
 ### Changed ###
diff --git a/mkdir_linux.go b/mkdir_linux.go
index ad2bd79..0c537ba 100644
--- a/mkdir_linux.go
+++ b/mkdir_linux.go
@@ -46,6 +46,13 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 	if mode&^0o7777 != 0 {
 		return nil, fmt.Errorf("%w for mkdir 0o%.3o", errInvalidMode, mode)
 	}
+	// On Linux, mkdirat(2) (and os.Mkdir) silently ignore the suid and sgid
+	// bits. We could also silently ignore them but since we have very few
+	// users it seems more prudent to return an error so users notice that
+	// these bits will not be set.
+	if mode&^0o1777 != 0 {
+		return nil, fmt.Errorf("%w for mkdir 0o%.3o: suid and sgid are ignored by mkdir", errInvalidMode, mode)
+	}
 
 	// Try to open as much of the path as possible.
 	currentDir, remainingPath, err := partialLookupInRoot(root, unsafePath)
diff --git a/mkdir_linux_test.go b/mkdir_linux_test.go
index bdad1c9..9b1ddbd 100644
--- a/mkdir_linux_test.go
+++ b/mkdir_linux_test.go
@@ -204,12 +204,11 @@ func testMkdirAll_InvalidMode(t *testing.T, mkdirAll func(t *testing.T, root, un
 		{unix.S_IFDIR | 0o777, errInvalidMode},
 		{unix.S_IFREG | 0o777, errInvalidMode},
 		{unix.S_IFIFO | 0o777, errInvalidMode},
-		// suid/sgid bits are valid but you get an error because they don't get
-		// applied by mkdirat.
-		// TODO: Figure out if we want to allow this.
-		{unix.S_ISUID | 0o777, errPossibleAttack},
-		{unix.S_ISGID | 0o777, errPossibleAttack},
-		{unix.S_ISUID | unix.S_ISGID | unix.S_ISVTX | 0o777, errPossibleAttack},
+		// suid/sgid bits are silently ignored by mkdirat and so we return an
+		// error explicitly.
+		{unix.S_ISUID | 0o777, errInvalidMode},
+		{unix.S_ISGID | 0o777, errInvalidMode},
+		{unix.S_ISUID | unix.S_ISGID | unix.S_ISVTX | 0o777, errInvalidMode},
 		// Proper sticky bit should work.
 		{unix.S_ISVTX | 0o777, nil},
 		// Regular mode bits.