diff --git a/docs/FORMAT.md b/docs/FORMAT.md index af84af2..5ed503e 100644 --- a/docs/FORMAT.md +++ b/docs/FORMAT.md @@ -93,16 +93,15 @@ Each file entry contains: ## File Mode `mode` holds a file's Unix permission bits, the nine `rwx` bits, so it is never -above `0777` (octal); the setuid, setgid and sticky bits are never recorded. -Writers record `0000` unless whoever creates the manifest asks for permissions. -`0000`, the proto3 default, means no mode was recorded: readers never check or -apply it. +above `0777` (octal); the setuid, setgid and sticky bits are never recorded. A +reader must reject a manifest with an entry whose mode is above `0777`. Writers +record `0000` unless whoever creates the manifest asks for permissions. `0000`, +the proto3 default, means no mode was recorded: readers never check or apply it. The reference implementation records modes when `gen` or `freshen` is given `--include-permissions`. `check` fails a file whose permission bits differ from a recorded mode other than `0000`. `fetch` sets a recorded mode other than -`0000` on each file it writes, and refuses a manifest that records a mode above -`0777` before it requests any file. +`0000` on each file it writes. ## Path Rules diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index d2497f2..d75546b 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -96,9 +96,6 @@ var ( // writes another file. errNameClash = errors.New( "manifest lists a file where fetch writes another file") - // errModeOutOfRange indicates a manifest that lists a mode with more - // than the permission bits, such as setuid. - errModeOutOfRange = errors.New("manifest lists a mode outside 0777") ) // DownloadProgress reports the progress of a single file download. @@ -429,9 +426,9 @@ func (mfa *CLIApp) fetchManifestOperation( // fetchManifest downloads the manifest at manifestURL and parses it, // enforcing --require-signature if it is given and refusing a manifest -// that lists a file where fetch writes another or a mode outside 0777. It -// returns the manifest as downloaded, to be saved once the files are in -// place, and the files it lists. +// that lists a file where fetch writes another. It returns the manifest as +// downloaded, to be saved once the files are in place, and the files it +// lists. func (mfa *CLIApp) fetchManifest( ctx context.Context, cmd *cli.Command, client retryingClient, manifestURL string, ) ([]byte, []*mfer.MFFilePath, error) { @@ -488,16 +485,6 @@ func (mfa *CLIApp) fetchManifest( return nil, nil, err } - // fetch sets each recorded mode on the file it writes, so a mode above - // 0777, which could carry setuid, setgid or sticky bits, is refused - // before any file is requested. - for _, f := range files { - if f.GetMode() > uint32(os.ModePerm) { - return nil, nil, fmt.Errorf("%w: %s (%#o)", - errModeOutOfRange, f.GetPath(), f.GetMode()) - } - } - log.Infof("manifest contains %d files", len(files)) return manifestData, files, nil diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index bb4b053..519d1fb 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -19,15 +19,12 @@ import ( "sync/atomic" "testing" "time" - "uuid" - "github.com/klauspost/compress/zstd" "github.com/multiformats/go-multihash" "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" urfcli "github.com/urfave/cli/v3" - "google.golang.org/protobuf/proto" "sneak.berlin/go/mfer/mfer" ) @@ -1540,27 +1537,9 @@ func TestFetchSetsRecordedMode(t *testing.T) { []string{"/" + defaultManifestName, "/" + testFileTxt}, requested) } -// TestFetchRefusesModeOutsidePermissionBits fetches manifests that record -// a mode with the setuid bit, once as Unix writes it (04755) and once as -// Go keeps it in os.FileMode. fetch must refuse both before it creates the -// destination or requests any file. -func TestFetchRefusesModeOutsidePermissionBits(t *testing.T) { - t.Parallel() - - files := map[string][]byte{testFileTxt: []byte("a file")} - - assertFetchRefused(t, manifestWithMode(t, testFileTxt, files[testFileTxt], 0o4755), - files, "manifest lists a mode outside 0777: file.txt (04755)") - - assertFetchRefused(t, - manifestWithMode(t, testFileTxt, files[testFileTxt], - uint32(os.ModeSetuid|0o755)), - files, "manifest lists a mode outside 0777: file.txt (040000755)") -} - // TestDownloadFileSetsOnlyPermissionBits downloads a file whose entry // records mode 0755 together with setuid, setgid or sticky, as Go keeps -// them in os.FileMode. fetchManifest would refuse such an entry; called +// them in os.FileMode. Loading a manifest refuses such an entry; called // directly, downloadFile must still set only the permission bits. func TestDownloadFileSetsOnlyPermissionBits(t *testing.T) { t.Parallel() @@ -1597,51 +1576,6 @@ func TestDownloadFileSetsOnlyPermissionBits(t *testing.T) { } } -// manifestWithMode returns a manifest listing one file, path, with content -// and the given recorded mode. It is assembled by hand, since the builder -// never records a mode outside 0777. -func manifestWithMode(t *testing.T, path string, content []byte, mode uint32) []byte { - t.Helper() - - digest := sha256.Sum256(content) - hash, err := multihash.Encode(digest[:], multihash.SHA2_256) - require.NoError(t, err) - - id := uuid.NewV4() - - inner, err := proto.Marshal(&mfer.MFFile{ - Version: mfer.MFFile_VERSION_ONE, - Files: []*mfer.MFFilePath{{ - Path: path, - Size: int64(len(content)), - Hashes: []*mfer.MFFileChecksum{{MultiHash: hash}}, - Mode: mode, - }}, - Uuid: id[:], - }) - require.NoError(t, err) - - encoder, err := zstd.NewWriter(nil) - require.NoError(t, err) - - compressed := encoder.EncodeAll(inner, nil) - require.NoError(t, encoder.Close()) - - sum := sha256.Sum256(compressed) - - outer, err := proto.Marshal(&mfer.MFFileOuter{ - Version: mfer.MFFileOuter_VERSION_ONE, - CompressionType: mfer.MFFileOuter_COMPRESSION_ZSTD, - Size: int64(len(inner)), - Sha256: sum[:], - Uuid: id[:], - InnerMessage: compressed, - }) - require.NoError(t, err) - - return append([]byte(mfer.MAGIC), outer...) -} - // builtManifest returns a manifest of files, built directly rather than // scanned, since a scan lists no hidden files. func builtManifest(t *testing.T, files map[string][]byte) []byte { diff --git a/mfer/deserialize.go b/mfer/deserialize.go index b2bbcec..65a3511 100644 --- a/mfer/deserialize.go +++ b/mfer/deserialize.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "os" "strings" "github.com/klauspost/compress/zstd" @@ -29,6 +30,7 @@ var ( errInvalidManifestPath = errors.New("invalid file entry") errEntryMissingHash = errors.New("file entry has no SHA-256 hash") errEntryExtraSHA256Hash = errors.New("file entry has more than one SHA-256 hash") + errModeOutOfRange = errors.New("file entry has a mode above 0777") errDecodedTooLarge = errors.New("too much memory needed") errSignerNotSigningKey = errors.New( "signer is not the fingerprint of the key that made the signature") @@ -320,7 +322,9 @@ func (m *manifest) deserializeInner() error { // dropping entries, which would let a hostile manifest hide files from a // check. A path listed twice is refused too: check would check the one // file against both entries. So is an entry that does not list exactly - // one SHA-256 hash. + // one SHA-256 hash, and one that records a mode above 0777, such as one + // with setuid: a mode holds the permission bits alone, which are all + // check compares and fetch sets. seen := make(map[string]bool, len(m.pbInner.GetFiles())) for _, f := range m.pbInner.GetFiles() { @@ -334,6 +338,10 @@ func (m *manifest) deserializeInner() error { return err } + if f.GetMode() > uint32(os.ModePerm) { + return fmt.Errorf("%w: %q (%#o)", errModeOutOfRange, f.GetPath(), f.GetMode()) + } + if seen[f.GetPath()] { return fmt.Errorf("%w %q", errDuplicatePath, f.GetPath()) } diff --git a/mfer/deserialize_path_test.go b/mfer/deserialize_path_test.go index 864686e..6a5259c 100644 --- a/mfer/deserialize_path_test.go +++ b/mfer/deserialize_path_test.go @@ -7,6 +7,7 @@ import ( "crypto/sha256" "crypto/sha512" "fmt" + "os" "strconv" "strings" "testing" @@ -234,6 +235,66 @@ func TestDeserializeRequiresExactlyOneSHA256Hash(t *testing.T) { } } +// A file entry that records a mode with more than the permission bits is +// refused as it is loaded, naming the path and the mode: once with setuid as +// Unix writes it (04755) and once as Go keeps it in os.FileMode. An entry +// recording 0777 loads. +func TestDeserializeRefusesModeOutsidePermissionBits(t *testing.T) { + t.Parallel() + + hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256) + require.NoError(t, err) + + tests := []struct { + name string + mode uint32 + want string // "" for an entry that loads + }{ + { + "setuid as Unix writes it", + 0o4755, + `file entry has a mode above 0777: "x" (04755)`, + }, + { + "setuid as Go keeps it", + uint32(os.ModeSetuid | 0o755), + `file entry has a mode above 0777: "x" (040000755)`, + }, + {"permission bits only", 0o777, ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + id := uuid.NewV4() + inner, err := proto.Marshal(&MFFile{ + Version: MFFile_VERSION_ONE, + Files: []*MFFilePath{{ + Path: "x", + Size: 1, + Hashes: []*MFFileChecksum{{MultiHash: hash}}, + Mode: tt.mode, + }}, + Uuid: id[:], + }) + require.NoError(t, err) + + m, err := NewManifestFromReader(bytes.NewReader(wrapInner(t, id, inner))) + if tt.want == "" { + require.NoError(t, err) + require.Len(t, m.Files(), 1) + assert.Equal(t, tt.mode, m.Files()[0].GetMode()) + + return + } + + require.ErrorIs(t, err, errModeOutOfRange) + require.EqualError(t, err, tt.want) + }) + } +} + // Entries of a path, a SHA-256 hash, an empty MIME type and empty // modification and change times are counted at 432 bytes each (176 + 112 + // 16 + 64 + 64) and take 52 bytes plus the path to encode. A one-character