diff --git a/changes/mdm-apple-plist-parsing b/changes/mdm-apple-plist-parsing new file mode 100644 index 0000000000..ec9a3cc72d --- /dev/null +++ b/changes/mdm-apple-plist-parsing @@ -0,0 +1 @@ +* Improved Apple MDM enrollment and device management request parsing diff --git a/server/fleet/request.go b/server/fleet/request.go index 49b89396db..c37a4182d2 100644 --- a/server/fleet/request.go +++ b/server/fleet/request.go @@ -18,6 +18,10 @@ const ( // MaxMultiScriptQuerySize, sets a max size for payloads that take multiple scripts and SQL queries. MaxMultiScriptQuerySize int64 = 5 * units.MiB MaxMicrosoftMDMSize int64 = 2 * units.MiB + // MaxAppleMDMRequestBodySize bounds Apple MDM check-in and command-result + // request bodies. Results are stored in a MEDIUMTEXT column (max 16,777,215 + // bytes), so the limit must not exceed that boundary. + MaxAppleMDMRequestBodySize int64 = (16 * units.MiB) - 1 // DefaultMaxOsqueryLogWriteSize is the default request body size limit // applied to /api/osquery/log when osquery.allow_body_auth_fallback is diff --git a/server/mdm/apple/deviceinfo.go b/server/mdm/apple/deviceinfo.go index c051fa919f..270a7ebeca 100644 --- a/server/mdm/apple/deviceinfo.go +++ b/server/mdm/apple/deviceinfo.go @@ -41,7 +41,6 @@ import ( "github.com/fleetdm/fleet/v4/server/dev_mode" "github.com/fleetdm/fleet/v4/server/fleet" "github.com/fleetdm/fleet/v4/server/mdm/apple/rootcert" - "github.com/micromdm/plist" "github.com/smallstep/pkcs7" ) @@ -159,7 +158,7 @@ func ParseMachineInfoFromPKCS7(buf []byte, verify bool) (*fleet.MDMAppleMachineI } info := new(fleet.MDMAppleMachineInfo) - if err = plist.Unmarshal(p7.Content, info); err != nil { + if err = BoundedPlistUnmarshal(p7.Content, info); err != nil { return nil, fmt.Errorf("could not decode plist: %w", err) } diff --git a/server/mdm/apple/plist_bounds.go b/server/mdm/apple/plist_bounds.go new file mode 100644 index 0000000000..fb3330a957 --- /dev/null +++ b/server/mdm/apple/plist_bounds.go @@ -0,0 +1,256 @@ +package apple_mdm + +import ( + "bytes" + "encoding/binary" + "errors" + "fmt" + + "github.com/micromdm/plist" +) + +// Limits applied to binary property lists before decoding. They are generous +// for the device-info plists these endpoints handle, which are a single flat +// dictionary of scalar values. +const ( + // binaryPlistMagic is the prefix that selects the binary plist decoder. + binaryPlistMagic = "bplist00" + plistTrailerSize = 32 + + maxPlistObjects = 1 << 16 // distinct objects (offset-table size) + maxPlistDepth = 16 // reference nesting + maxPlistNodes = 1 << 16 // objects after references are expanded +) + +var ( + errPlistTooComplex = errors.New("plist exceeds parsing limits") + errMalformedPlist = errors.New("malformed binary plist") +) + +// BoundedPlistUnmarshal decodes a plist into v. Binary plists are first checked +// against the depth, object-count, and object-size limits above; XML plists are +// decoded directly (their size is bounded by the caller's body limit). +func BoundedPlistUnmarshal(data []byte, v any) error { + if bytes.HasPrefix(data, []byte(binaryPlistMagic)) { + if err := checkBinaryPlistBounds(data); err != nil { + return err + } + } + return plist.Unmarshal(data, v) +} + +// checkBinaryPlistBounds walks a binary plist's object references, rejecting +// input that exceeds the limits or points outside the data region. +func checkBinaryPlistBounds(data []byte) error { + if len(data) < len(binaryPlistMagic)+plistTrailerSize { + return fmt.Errorf("%w: shorter than minimum size", errMalformedPlist) + } + + // Trailer is the final 32 bytes (CFBinaryPlistTrailer). + trailer := data[len(data)-plistTrailerSize:] + offsetIntSize := trailer[6] + objectRefSize := trailer[7] + numObjects := binary.BigEndian.Uint64(trailer[8:16]) + rootObject := binary.BigEndian.Uint64(trailer[16:24]) + offsetTableOffset := binary.BigEndian.Uint64(trailer[24:32]) + + if offsetIntSize == 0 || offsetIntSize > 8 || objectRefSize == 0 || objectRefSize > 8 { + return fmt.Errorf("%w: invalid integer sizes", errMalformedPlist) + } + if numObjects == 0 { + return fmt.Errorf("%w: no objects", errMalformedPlist) + } + if numObjects > maxPlistObjects { + return fmt.Errorf("%w: %d objects", errPlistTooComplex, numObjects) + } + + trailerStart := uint64(len(data) - plistTrailerSize) //nolint:gosec // dismiss G115, length is bounded above + tableBytes := numObjects * uint64(offsetIntSize) + if offsetTableOffset > trailerStart || tableBytes > trailerStart-offsetTableOffset { + return fmt.Errorf("%w: offset table out of bounds", errMalformedPlist) + } + + offsetTable := make([]uint64, numObjects) + pos := offsetTableOffset + for i := range offsetTable { + offsetTable[i] = readUintBE(data[pos : pos+uint64(offsetIntSize)]) + pos += uint64(offsetIntSize) + } + + b := &plistBounder{ + data: data, + offsetTable: offsetTable, + objectRefSize: objectRefSize, + dataEnd: offsetTableOffset, // objects live before the offset table + } + return b.visit(rootObject, 0) +} + +type plistBounder struct { + data []byte + offsetTable []uint64 + objectRefSize uint8 + dataEnd uint64 + nodes int +} + +// visit walks the object at index, recursing into array and dict references. +// The depth and node guards keep the walk itself bounded: a cycle stops at +// maxPlistDepth, and reference expansion stops at maxPlistNodes. +func (b *plistBounder) visit(index uint64, depth int) error { + if depth > maxPlistDepth { + return fmt.Errorf("%w: nesting deeper than %d", errPlistTooComplex, maxPlistDepth) + } + b.nodes++ + if b.nodes > maxPlistNodes { + return fmt.Errorf("%w: more than %d expanded objects", errPlistTooComplex, maxPlistNodes) + } + if index >= uint64(len(b.offsetTable)) { + return fmt.Errorf("%w: object ref %d out of range", errMalformedPlist, index) + } + offset := b.offsetTable[index] + if offset >= b.dataEnd { + return fmt.Errorf("%w: object offset out of range", errMalformedPlist) + } + + // High nibble of the marker byte is the object type (CFBinaryPList.c). + cur := plistCursor{data: b.data, pos: offset, end: b.dataEnd} + marker, err := cur.readByte() + if err != nil { + return err + } + switch marker >> 4 { + case 0xa: // array: count object refs + count, err := cur.readCount(marker) + if err != nil { + return err + } + if count > cur.remaining()/uint64(b.objectRefSize) { + return fmt.Errorf("%w: array refs exceed input", errMalformedPlist) + } + return b.visitRefs(&cur, count, depth) + case 0xd: // dictionary: count key refs followed by count value refs + count, err := cur.readCount(marker) + if err != nil { + return err + } + if count > cur.remaining()/uint64(b.objectRefSize)/2 { + return fmt.Errorf("%w: dictionary refs exceed input", errMalformedPlist) + } + return b.visitRefs(&cur, 2*count, depth) + case 0x2: // real: 1<<(low nibble) bytes follow inline + nbytes := uint64(1) << (marker & 0xf) + if cur.pos+nbytes < cur.pos || cur.pos+nbytes > b.dataEnd { + return fmt.Errorf("%w: real size exceeds input", errMalformedPlist) + } + return nil + case 0x4, 0x5: // data, ASCII string: count single-byte units + return b.checkPayload(&cur, marker, 1) + case 0x6: // UTF-16 string: count two-byte units + return b.checkPayload(&cur, marker, 2) + default: + // Other types are fixed-size or non-recursive; nothing to bound. + return nil + } +} + +func (b *plistBounder) visitRefs(cur *plistCursor, total uint64, depth int) error { + for range total { + ref, err := cur.readRef(b.objectRefSize) + if err != nil { + return err + } + if err := b.visit(ref, depth+1); err != nil { + return err + } + } + return nil +} + +// checkPayload verifies a variable-length scalar (data or string) declares a +// payload that fits within the object region. +func (b *plistBounder) checkPayload(cur *plistCursor, marker byte, unitSize uint64) error { + count, err := cur.readCount(marker) + if err != nil { + return err + } + size := count * unitSize + if size/unitSize != count { + return fmt.Errorf("%w: object size overflow", errMalformedPlist) + } + if cur.pos+size < cur.pos || cur.pos+size > b.dataEnd { + return fmt.Errorf("%w: object size exceeds input", errMalformedPlist) + } + return nil +} + +// plistCursor reads forward within a single object's bytes, bounded by end. +type plistCursor struct { + data []byte + pos uint64 + end uint64 +} + +func (c *plistCursor) remaining() uint64 { + if c.pos >= c.end { + return 0 + } + return c.end - c.pos +} + +func (c *plistCursor) readByte() (byte, error) { + if c.pos >= c.end { + return 0, fmt.Errorf("%w: unexpected end of object data", errMalformedPlist) + } + v := c.data[c.pos] + c.pos++ + return v, nil +} + +func (c *plistCursor) readBytes(n uint64) ([]byte, error) { + if n == 0 { + return nil, nil + } + if c.pos+n < c.pos || c.pos+n > c.end { + return nil, fmt.Errorf("%w: unexpected end of object data", errMalformedPlist) + } + v := c.data[c.pos : c.pos+n] + c.pos += n + return v, nil +} + +// readCount decodes the variable-length count used by data, strings, arrays, and dicts. +func (c *plistCursor) readCount(marker byte) (uint64, error) { + if marker&0xf != 0xf { + return uint64(marker & 0xf), nil + } + sizeMarker, err := c.readByte() + if err != nil { + return 0, err + } + nbytes := uint64(1) << (sizeMarker & 0x0f) + if nbytes > 8 { + return 0, fmt.Errorf("%w: invalid count size", errMalformedPlist) + } + buf, err := c.readBytes(nbytes) + if err != nil { + return 0, err + } + return readUintBE(buf), nil +} + +func (c *plistCursor) readRef(size uint8) (uint64, error) { + buf, err := c.readBytes(uint64(size)) + if err != nil { + return 0, err + } + return readUintBE(buf), nil +} + +func readUintBE(b []byte) uint64 { + var n uint64 + for _, c := range b { + n = n<<8 | uint64(c) + } + return n +} diff --git a/server/mdm/apple/plist_bounds_test.go b/server/mdm/apple/plist_bounds_test.go new file mode 100644 index 0000000000..90eb0bd38a --- /dev/null +++ b/server/mdm/apple/plist_bounds_test.go @@ -0,0 +1,180 @@ +package apple_mdm + +import ( + "encoding/binary" + "testing" + + "github.com/fleetdm/fleet/v4/server/fleet" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestBoundedPlistUnmarshalBinary(t *testing.T) { + t.Run("flat dictionary decodes", func(t *testing.T) { + data := buildFlatBinaryPlist(t, [][2]string{ + {"SERIAL", "ABC123"}, + {"UDID", "0000-1111"}, + }) + + var info fleet.MDMAppleMachineInfo + require.NoError(t, BoundedPlistUnmarshal(data, &info)) + assert.Equal(t, "ABC123", info.Serial) + assert.Equal(t, "0000-1111", info.UDID) + }) + + t.Run("nesting beyond the depth limit is rejected", func(t *testing.T) { + data := buildRefChain(t, 24) + require.Less(t, len(data), 200) + + var info fleet.MDMAppleMachineInfo + err := BoundedPlistUnmarshal(data, &info) + require.ErrorIs(t, err, errPlistTooComplex) + }) + + t.Run("self-referential object is rejected", func(t *testing.T) { + // Object 0 is an array referencing itself (a cycle). + data := buildBinaryPlist(t, []byte{0xa2, 0x00, 0x00}) + + var info fleet.MDMAppleMachineInfo + err := BoundedPlistUnmarshal(data, &info) + require.ErrorIs(t, err, errPlistTooComplex) + }) + + t.Run("string length beyond input is rejected", func(t *testing.T) { + // ASCII string declaring a length larger than the input contains. + obj := []byte{0x5f, 0x13, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff} + data := buildBinaryPlist(t, obj) + + var info fleet.MDMAppleMachineInfo + err := BoundedPlistUnmarshal(data, &info) + require.ErrorIs(t, err, errMalformedPlist) + }) + + t.Run("real size beyond input is rejected", func(t *testing.T) { + // Real marker 0x2f declares more inline bytes than the input contains. + data := buildBinaryPlist(t, []byte{0x2f}) + + var info fleet.MDMAppleMachineInfo + err := BoundedPlistUnmarshal(data, &info) + require.ErrorIs(t, err, errMalformedPlist) + }) + + t.Run("object count beyond the limit is rejected", func(t *testing.T) { + data := buildBinaryPlist(t, []byte{0x08}) // bool false + trailer := data[len(data)-plistTrailerSize:] + binary.BigEndian.PutUint64(trailer[8:16], maxPlistObjects+1) + + var info fleet.MDMAppleMachineInfo + err := BoundedPlistUnmarshal(data, &info) + require.ErrorIs(t, err, errPlistTooComplex) + }) +} + +func TestBoundedPlistUnmarshalXML(t *testing.T) { + // XML plists are not reference-encoded, so bounds checking is skipped. + xml := []byte(` + + + + SERIAL + XML-SERIAL + +`) + + var info fleet.MDMAppleMachineInfo + require.NoError(t, BoundedPlistUnmarshal(xml, &info)) + assert.Equal(t, "XML-SERIAL", info.Serial) +} + +// buildBinaryPlist wraps a single root object (placed immediately after the +// header at index 0) with a one-byte offset table and trailer. +func buildBinaryPlist(t *testing.T, rootObject []byte) []byte { + t.Helper() + body := append([]byte("bplist00"), rootObject...) + offsetTableOffset := len(body) + body = appendByte(body, len("bplist00")) // offset of object 0 + return append(body, makeTrailer(1, offsetTableOffset)...) +} + +// buildRefChain lays out n array objects, each referencing the next object +// twice, terminated by a single leaf object. +func buildRefChain(t *testing.T, n int) []byte { + t.Helper() + require.Less(t, n, 250, "single-byte refs require < 250 objects") + + body := []byte("bplist00") + offsets := make([]int, 0, n+1) + for i := range n { + offsets = append(offsets, len(body)) + body = appendByte(body, 0xa2) // array of 2 refs to the next object + body = appendByte(body, i+1) + body = appendByte(body, i+1) + } + offsets = append(offsets, len(body)) + body = appendByte(body, 0x08) // leaf: bool false + + offsetTableOffset := len(body) + for _, off := range offsets { + body = appendByte(body, off) + } + return append(body, makeTrailer(len(offsets), offsetTableOffset)...) +} + +// buildFlatBinaryPlist builds a binary plist of a single dictionary whose keys +// and values are all short ASCII strings. +func buildFlatBinaryPlist(t *testing.T, pairs [][2]string) []byte { + t.Helper() + n := len(pairs) + require.Less(t, n, 15, "builder uses inline dict counts") + + body := []byte("bplist00") + offsets := []int{len(body)} + + // Object 0: dict. Key refs are objects 1..n, value refs are n+1..2n. + body = appendByte(body, 0xd0|n) + for i := range n { + body = appendByte(body, 1+i) + } + for i := range n { + body = appendByte(body, 1+n+i) + } + + appendStr := func(s string) { + require.Less(t, len(s), 15, "builder uses inline string counts") + offsets = append(offsets, len(body)) + body = appendByte(body, 0x50|len(s)) + body = append(body, []byte(s)...) + } + for _, p := range pairs { + appendStr(p[0]) + } + for _, p := range pairs { + appendStr(p[1]) + } + + offsetTableOffset := len(body) + for _, off := range offsets { + body = appendByte(body, off) + } + return append(body, makeTrailer(len(offsets), offsetTableOffset)...) +} + +// makeTrailer builds a 32-byte trailer with single-byte offsets and refs. +func makeTrailer(numObjects, offsetTableOffset int) []byte { + trailer := make([]byte, plistTrailerSize) + trailer[6] = 1 // offset int size + trailer[7] = 1 // object ref size + putUint64(trailer[8:16], numObjects) + putUint64(trailer[16:24], 0) // root object index + putUint64(trailer[24:32], offsetTableOffset) + return trailer +} + +// appendByte and putUint64 keep the fixture's narrowing conversions in one place. +func appendByte(body []byte, v int) []byte { + return append(body, byte(v)) //nolint:gosec // dismiss G115, fixture values are below 256 +} + +func putUint64(dst []byte, v int) { + binary.BigEndian.PutUint64(dst, uint64(v)) //nolint:gosec // dismiss G115, fixture values are bounded +} diff --git a/server/mdm/nanomdm/mdm/checkin.go b/server/mdm/nanomdm/mdm/checkin.go index 4ce8087e68..9840bba968 100644 --- a/server/mdm/nanomdm/mdm/checkin.go +++ b/server/mdm/nanomdm/mdm/checkin.go @@ -1,6 +1,7 @@ package mdm import ( + "bytes" "encoding/base64" "errors" "fmt" @@ -201,7 +202,7 @@ func (w *checkinUnmarshaller) UnmarshalPlist(f func(interface{}) error) error { // DecodeCheckin unmarshals rawMessage into a specific check-in struct in message. func DecodeCheckin(rawMessage []byte) (message interface{}, err error) { w := &checkinUnmarshaller{raw: rawMessage} - err = plist.Unmarshal(rawMessage, w) + err = plist.NewXMLDecoder(bytes.NewReader(rawMessage)).Decode(w) message = w.message return } diff --git a/server/mdm/nanomdm/mdm/command.go b/server/mdm/nanomdm/mdm/command.go index 0d2908dee3..1a2ab7766b 100644 --- a/server/mdm/nanomdm/mdm/command.go +++ b/server/mdm/nanomdm/mdm/command.go @@ -1,6 +1,7 @@ package mdm import ( + "bytes" "errors" "github.com/micromdm/plist" @@ -32,7 +33,7 @@ type CommandResults struct { // DecodeCheckin unmarshals rawMessage into results func DecodeCommandResults(rawResults []byte) (results *CommandResults, err error) { results = new(CommandResults) - err = plist.Unmarshal(rawResults, results) + err = plist.NewXMLDecoder(bytes.NewReader(rawResults)).Decode(results) if err != nil { return } @@ -68,7 +69,7 @@ type CommandWithSubtype struct { // DecodeCommand unmarshals rawCommand into command func DecodeCommand(rawCommand []byte) (command *Command, err error) { command = new(Command) - err = plist.Unmarshal(rawCommand, command) + err = plist.NewXMLDecoder(bytes.NewReader(rawCommand)).Decode(command) if err != nil { return } diff --git a/server/mdm/nanomdm/mdm/plist_test.go b/server/mdm/nanomdm/mdm/plist_test.go new file mode 100644 index 0000000000..da3f0944ef --- /dev/null +++ b/server/mdm/nanomdm/mdm/plist_test.go @@ -0,0 +1,44 @@ +package mdm + +import "testing" + +// MDM check-in and command-result messages are always XML. +func TestDecodeXMLOnly(t *testing.T) { + nonXML := []byte("bplist00\xd1\x01\x02") + if _, err := DecodeCheckin(nonXML); err == nil { + t.Error("DecodeCheckin: want error for non-XML input, got nil") + } + if _, err := DecodeCommandResults(nonXML); err == nil { + t.Error("DecodeCommandResults: want error for non-XML input, got nil") + } + + checkin := []byte(` + + + + MessageType + Authenticate + UDID + 00000000-1111 + Topic + com.apple.mgmt.External.test + +`) + if _, err := DecodeCheckin(checkin); err != nil { + t.Errorf("DecodeCheckin rejected valid XML: %v", err) + } + + result := []byte(` + + + + CommandUUID + abc-123 + Status + Acknowledged + +`) + if _, err := DecodeCommandResults(result); err != nil { + t.Errorf("DecodeCommandResults rejected valid XML: %v", err) + } +} diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index d8434cd477..866e436857 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -2132,7 +2132,7 @@ type mdmAppleAccountEnrollRequest struct { func (mdmAppleAccountEnrollRequest) DecodeRequest(ctx context.Context, r *http.Request) (interface{}, error) { decoded := mdmAppleAccountEnrollRequest{} - rawData, err := io.ReadAll(r.Body) + rawData, err := io.ReadAll(io.LimitReader(r.Body, limit10KiB)) if err != nil { return nil, ctxerr.Wrap(ctx, err, "reading body from request") } @@ -2147,7 +2147,7 @@ func (mdmAppleAccountEnrollRequest) DecodeRequest(ctx context.Context, r *http.R deviceInfo := fleet.MDMAppleAccountDrivenUserEnrollDeviceInfo{} - err = plist.Unmarshal(p7.Content, &deviceInfo) + err = apple_mdm.BoundedPlistUnmarshal(p7.Content, &deviceInfo) if err != nil { return nil, &fleet.BadRequestError{ Message: "invalid request body", @@ -6726,7 +6726,7 @@ func (mdmAppleOTARequest) DecodeRequest(ctx context.Context, r *http.Request) (i idpUUID := r.URL.Query().Get("idp_uuid") // Can be empty. - rawData, err := io.ReadAll(r.Body) + rawData, err := io.ReadAll(io.LimitReader(r.Body, limit10KiB)) if err != nil { return nil, ctxerr.Wrap(ctx, err, "reading body from request") } @@ -6740,7 +6740,7 @@ func (mdmAppleOTARequest) DecodeRequest(ctx context.Context, r *http.Request) (i } var request mdmAppleOTARequest - err = plist.Unmarshal(p7.Content, &request.DeviceInfo) + err = apple_mdm.BoundedPlistUnmarshal(p7.Content, &request.DeviceInfo) if err != nil { return nil, &fleet.BadRequestError{ Message: "invalid request body", diff --git a/server/service/handler.go b/server/service/handler.go index 7be389ac14..e05ec5df21 100644 --- a/server/service/handler.go +++ b/server/service/handler.go @@ -1459,6 +1459,8 @@ func registerMDM( } mdmHandler = httpmdm.CertExtractMdmSignatureMiddleware(mdmHandler, httpmdm.MdmSignatureVerifierFunc(cryptoutil.VerifyMdmSignature), httpmdm.SigLogWithLogger(mdmLogger.With("handler", "cert-extract"))) + // Bound the request body before any middleware reads it. + mdmHandler = http.MaxBytesHandler(mdmHandler, fleet.MaxAppleMDMRequestBodySize) mux.Handle(apple_mdm.MDMPath, otel.WrapHandler(mdmHandler, apple_mdm.MDMPath, fleetConfig)) return nil }