From b41ca11598b3e8402ccdf413679fa13021163497 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Mon, 13 Mar 2023 01:52:03 -0700 Subject: [PATCH 1/2] Fix access denied on mounted vhdx root It seems that in certain situations, like having the containerd root and state on a file system hosted on a mounted VHDX, we need SeSecurityPrivilege when opening a file with winio.ACCESS_SYSTEM_SECURITY. This happens in the base layer writer in hcsshim when adding a new file. Enabling SeSecurityPrivilege allows the containerd root to be hosted on a vhdx. Signed-off-by: Gabriel Adrian Samfira --- archive/tar_opts_windows.go | 10 ++++++++++ snapshots/windows/windows.go | 8 ++++++++ 2 files changed, 18 insertions(+) diff --git a/archive/tar_opts_windows.go b/archive/tar_opts_windows.go index 0ba3cd082..fc51999bd 100644 --- a/archive/tar_opts_windows.go +++ b/archive/tar_opts_windows.go @@ -18,14 +18,24 @@ package archive import ( "context" + "fmt" "io" + "github.com/Microsoft/go-winio" "github.com/Microsoft/hcsshim/pkg/ociwclayer" ) // applyWindowsLayer applies a tar stream of an OCI style diff tar of a Windows layer // See https://github.com/opencontainers/image-spec/blob/main/layer.md#applying-changesets func applyWindowsLayer(ctx context.Context, root string, r io.Reader, options ApplyOptions) (size int64, err error) { + // It seems that in certain situations, like having the containerd root and state on a file system hosted on a + // mounted VHDX, we need SeSecurityPrivilege when opening a file with winio.ACCESS_SYSTEM_SECURITY. This happens + // in the base layer writer in hcsshim when adding a new file. + if err := winio.EnableProcessPrivileges([]string{winio.SeSecurityPrivilege}); err != nil { + return 0, fmt.Errorf("enabling privileges: %w", err) + } + defer winio.DisableProcessPrivileges([]string{winio.SeSecurityPrivilege}) + return ociwclayer.ImportLayerFromTar(ctx, r, root, options.Parents) } diff --git a/snapshots/windows/windows.go b/snapshots/windows/windows.go index 8e79f10e7..0d90128db 100644 --- a/snapshots/windows/windows.go +++ b/snapshots/windows/windows.go @@ -478,6 +478,14 @@ func (s *snapshotter) convertScratchToReadOnlyLayer(ctx context.Context, snapsho writer.CloseWithError(err) }() + // It seems that in certain situations, like having the containerd root and state on a file system hosted on a + // mounted VHDX, we need SeSecurityPrivilege when opening a file with winio.ACCESS_SYSTEM_SECURITY. This happens + // in the base layer writer in hcsshim when adding a new file. + if err := winio.EnableProcessPrivileges([]string{winio.SeSecurityPrivilege}); err != nil { + return fmt.Errorf("enabling privileges: %w", err) + } + defer winio.DisableProcessPrivileges([]string{winio.SeSecurityPrivilege}) + if _, err := ociwclayer.ImportLayerFromTar(ctx, reader, path, parentLayerPaths); err != nil { return fmt.Errorf("failed to reimport snapshot: %w", err) } From 6f0714efcb9ef07d1e5881e9f8eb032b5e5088ea Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Wed, 15 Mar 2023 05:30:46 -0700 Subject: [PATCH 2/2] Use RunWithPrivileges RunWithPrivileges() will enable privileges will lock a thread, change privileges, and run the function passed in, within that thread. This allows us to limit the scope in which we enable privileges and avoids accidentally enabling privileges in threads that should never have them. Signed-off-by: Gabriel Adrian Samfira --- archive/tar_opts_windows.go | 13 ++++++------- snapshots/windows/windows.go | 10 ++++------ 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/archive/tar_opts_windows.go b/archive/tar_opts_windows.go index fc51999bd..567a3c352 100644 --- a/archive/tar_opts_windows.go +++ b/archive/tar_opts_windows.go @@ -18,7 +18,6 @@ package archive import ( "context" - "fmt" "io" "github.com/Microsoft/go-winio" @@ -31,12 +30,12 @@ func applyWindowsLayer(ctx context.Context, root string, r io.Reader, options Ap // It seems that in certain situations, like having the containerd root and state on a file system hosted on a // mounted VHDX, we need SeSecurityPrivilege when opening a file with winio.ACCESS_SYSTEM_SECURITY. This happens // in the base layer writer in hcsshim when adding a new file. - if err := winio.EnableProcessPrivileges([]string{winio.SeSecurityPrivilege}); err != nil { - return 0, fmt.Errorf("enabling privileges: %w", err) - } - defer winio.DisableProcessPrivileges([]string{winio.SeSecurityPrivilege}) - - return ociwclayer.ImportLayerFromTar(ctx, r, root, options.Parents) + err = winio.RunWithPrivileges([]string{winio.SeSecurityPrivilege}, func() error { + var innerErr error + size, innerErr = ociwclayer.ImportLayerFromTar(ctx, r, root, options.Parents) + return innerErr + }) + return } // AsWindowsContainerLayer indicates that the tar stream to apply is that of diff --git a/snapshots/windows/windows.go b/snapshots/windows/windows.go index 0d90128db..b96041b1e 100644 --- a/snapshots/windows/windows.go +++ b/snapshots/windows/windows.go @@ -481,12 +481,10 @@ func (s *snapshotter) convertScratchToReadOnlyLayer(ctx context.Context, snapsho // It seems that in certain situations, like having the containerd root and state on a file system hosted on a // mounted VHDX, we need SeSecurityPrivilege when opening a file with winio.ACCESS_SYSTEM_SECURITY. This happens // in the base layer writer in hcsshim when adding a new file. - if err := winio.EnableProcessPrivileges([]string{winio.SeSecurityPrivilege}); err != nil { - return fmt.Errorf("enabling privileges: %w", err) - } - defer winio.DisableProcessPrivileges([]string{winio.SeSecurityPrivilege}) - - if _, err := ociwclayer.ImportLayerFromTar(ctx, reader, path, parentLayerPaths); err != nil { + if err := winio.RunWithPrivileges([]string{winio.SeSecurityPrivilege}, func() error { + _, err := ociwclayer.ImportLayerFromTar(ctx, reader, path, parentLayerPaths) + return err + }); err != nil { return fmt.Errorf("failed to reimport snapshot: %w", err) }