From b36a8c8952a230afaab2067672a1818f23c347e1 Mon Sep 17 00:00:00 2001 From: Nuno Cruces Date: Wed, 25 Sep 2024 00:44:52 +0100 Subject: [PATCH 1/2] Issue #2321. Signed-off-by: Nuno Cruces --- internal/sysfs/dirfs.go | 25 ++++++++++++++----------- internal/sysfs/dirfs_supported.go | 9 ++++++--- internal/sysfs/dirfs_test.go | 2 +- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/internal/sysfs/dirfs.go b/internal/sysfs/dirfs.go index 04384038f4..40b24e68d4 100644 --- a/internal/sysfs/dirfs.go +++ b/internal/sysfs/dirfs.go @@ -3,6 +3,8 @@ package sysfs import ( "io/fs" "os" + "path" + "strings" experimentalsys "github.com/tetratelabs/wazero/experimental/sys" "github.com/tetratelabs/wazero/internal/platform" @@ -12,7 +14,7 @@ import ( func DirFS(dir string) experimentalsys.FS { return &dirFS{ dir: dir, - cleanedDir: ensureTrailingPathSeparator(dir), + cleanedDir: strings.TrimSuffix(dir, "/"), } } @@ -84,16 +86,17 @@ func (d *dirFS) Utimens(path string, atim, mtim int64) experimentalsys.Errno { return utimens(d.join(path), atim, mtim) } -func (d *dirFS) join(path string) string { - switch path { - case "", ".", "/": - if d.cleanedDir == "/" { - return "/" +func (d *dirFS) join(name string) string { + last := strings.HasSuffix(name, "/") + name = path.Clean("/" + name) + if name == "/" { + if d.cleanedDir != "" { + return d.cleanedDir } - // cleanedDir includes an unnecessary delimiter for the root path. - return d.cleanedDir[:len(d.cleanedDir)-1] + return "/" } - // TODO: Enforce similar to safefilepath.FromFS(path), but be careful as - // relative path inputs are allowed. e.g. dir or path == ../ - return d.cleanedDir + path + if last { + return d.cleanedDir + name + "/" + } + return d.cleanedDir + name } diff --git a/internal/sysfs/dirfs_supported.go b/internal/sysfs/dirfs_supported.go index ff93415b9c..f319f0c7c7 100644 --- a/internal/sysfs/dirfs_supported.go +++ b/internal/sysfs/dirfs_supported.go @@ -5,6 +5,8 @@ package sysfs import ( "io/fs" "os" + "path" + "strings" experimentalsys "github.com/tetratelabs/wazero/experimental/sys" ) @@ -34,9 +36,10 @@ func (d *dirFS) Chmod(path string, perm fs.FileMode) experimentalsys.Errno { // Symlink implements the same method as documented on sys.FS func (d *dirFS) Symlink(oldName, link string) experimentalsys.Errno { - // Note: do not resolve `oldName` relative to this dirFS. The link result is always resolved - // when dereference the `link` on its usage (e.g. readlink, read, etc). - // https://github.com/bytecodealliance/cap-std/blob/v1.0.4/cap-std/src/fs/dir.rs#L404-L409 + oldName = path.Clean(oldName) + if strings.HasPrefix(oldName, "../") { + return experimentalsys.EFAULT + } err := os.Symlink(oldName, d.join(link)) return experimentalsys.UnwrapOSError(err) } diff --git a/internal/sysfs/dirfs_test.go b/internal/sysfs/dirfs_test.go index 55e5ee1090..22d2a7cbdd 100644 --- a/internal/sysfs/dirfs_test.go +++ b/internal/sysfs/dirfs_test.go @@ -51,7 +51,7 @@ func TestDirFS_join(t *testing.T) { require.Equal(t, ".", testFS.join("")) require.Equal(t, ".", testFS.join(".")) require.Equal(t, ".", testFS.join("/")) - require.Equal(t, "."+string(os.PathSeparator)+"tmp", testFS.join("tmp")) + require.Equal(t, "./tmp", testFS.join("tmp")) } func TestDirFS_String(t *testing.T) { From 2ce9632a0173331080f74fc00422bc280969578e Mon Sep 17 00:00:00 2001 From: Nuno Cruces Date: Wed, 25 Sep 2024 11:47:14 +0100 Subject: [PATCH 2/2] More. Signed-off-by: Nuno Cruces --- internal/sysfs/dirfs.go | 16 ++++++++++------ internal/sysfs/dirfs_supported.go | 8 +++++++- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/internal/sysfs/dirfs.go b/internal/sysfs/dirfs.go index 40b24e68d4..366fd1c942 100644 --- a/internal/sysfs/dirfs.go +++ b/internal/sysfs/dirfs.go @@ -87,16 +87,20 @@ func (d *dirFS) Utimens(path string, atim, mtim int64) experimentalsys.Errno { } func (d *dirFS) join(name string) string { - last := strings.HasSuffix(name, "/") - name = path.Clean("/" + name) - if name == "/" { + // Lexically clean `name` to enforce that it always stays within the root + // hierarchy. This is necessary to avoid trivially escaping the root. + // See: https://github.com/tetratelabs/wazero/issues/2321 + // TODO: this violates POSIX pathname resolution semantics. + // https://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html#tag_04_11 + cleanedName := path.Clean("/" + name) + if cleanedName == "/" { if d.cleanedDir != "" { return d.cleanedDir } return "/" } - if last { - return d.cleanedDir + name + "/" + if strings.HasSuffix(name, "/") { + return d.cleanedDir + cleanedName + "/" } - return d.cleanedDir + name + return d.cleanedDir + cleanedName } diff --git a/internal/sysfs/dirfs_supported.go b/internal/sysfs/dirfs_supported.go index f319f0c7c7..e61338783e 100644 --- a/internal/sysfs/dirfs_supported.go +++ b/internal/sysfs/dirfs_supported.go @@ -36,8 +36,14 @@ func (d *dirFS) Chmod(path string, perm fs.FileMode) experimentalsys.Errno { // Symlink implements the same method as documented on sys.FS func (d *dirFS) Symlink(oldName, link string) experimentalsys.Errno { + // Note: do not resolve `oldName` relative to this dirFS. + // The link result is always resolved on usage (e.g. readlink, read, etc). + // However, this trivially allows escaping the root mount point. + // See: https://github.com/tetratelabs/wazero/issues/2321 + // So, lexically clean `oldName`, and enforce that it always points down + // the hierarchy. oldName = path.Clean(oldName) - if strings.HasPrefix(oldName, "../") { + if strings.HasPrefix(oldName, "../") || strings.HasPrefix(oldName, "/") { return experimentalsys.EFAULT } err := os.Symlink(oldName, d.join(link))