diff --git a/changes/windows-mdm-serial-host-linking b/changes/windows-mdm-serial-host-linking new file mode 100644 index 00000000000..38f80db63b6 --- /dev/null +++ b/changes/windows-mdm-serial-host-linking @@ -0,0 +1 @@ +- Improved Windows MDM host linking so the hardware serial a device reports cannot associate its enrollment with a host that a different device's enrollment already manages. Re-enrolling the same device is unaffected. diff --git a/server/datastore/mysql/microsoft_mdm.go b/server/datastore/mysql/microsoft_mdm.go index 47f08b7940b..bc25a2fa7e8 100644 --- a/server/datastore/mysql/microsoft_mdm.go +++ b/server/datastore/mysql/microsoft_mdm.go @@ -400,6 +400,36 @@ func (ds *Datastore) MDMWindowsGetUnlinkedEnrolledDeviceWithHardwareSerial(ctx c return &devices[0], nil } +// MDMWindowsConflictingEnrollmentHardwareID reports whether hostUUID is already claimed by an enrollment belonging to +// hardware other than mdmHardwareID. The returned ID is only for logging which device holds the host; conflicted is the +// answer, because an enrollment may carry an empty mdm_hardware_id and would otherwise be indistinguishable from "no +// incumbent". +// +// A device re-enrolling never conflicts with itself: MDMWindowsDeleteEnrolledDeviceOnReenrollment removes its previous +// row by hardware ID before the new one is inserted, so its claim on the host is gone before any linking runs. That +// holds because the hardware ID is stable across a wipe (even when enrolling with a different Entra ID). +func (ds *Datastore) MDMWindowsConflictingEnrollmentHardwareID(ctx context.Context, hostUUID string, mdmHardwareID string) (conflicted bool, conflictingHardwareID string, err error) { + if hostUUID == "" { + return false, "", nil + } + + const stmt = ` + SELECT mdm_hardware_id + FROM mdm_windows_enrollments + WHERE host_uuid = ? AND mdm_hardware_id != ? + ORDER BY id DESC + LIMIT 1` + + err = sqlx.GetContext(ctx, ds.reader(ctx), &conflictingHardwareID, stmt, hostUUID, mdmHardwareID) + switch { + case errors.Is(err, sql.ErrNoRows): + return false, "", nil + case err != nil: + return false, "", ctxerr.Wrap(ctx, err, "get conflicting windows mdm enrollment hardware id") + } + return true, conflictingHardwareID, nil +} + // GetWindowsEnrollmentDefaultFleet returns the configured default fleet for new user-driven Windows MDM enrollments. // Returns (nil, "") when no default is configured (including when the referenced fleet was deleted, which nulls the FK). func (ds *Datastore) GetWindowsEnrollmentDefaultFleet(ctx context.Context) (*uint, string, error) { diff --git a/server/datastore/mysql/microsoft_mdm_test.go b/server/datastore/mysql/microsoft_mdm_test.go index a667683f7d4..e58dc259826 100644 --- a/server/datastore/mysql/microsoft_mdm_test.go +++ b/server/datastore/mysql/microsoft_mdm_test.go @@ -92,6 +92,7 @@ func TestMDMWindows(t *testing.T) { {"TestMDMWindowsInsertCommandSkipsUnenrolledHosts", testMDMWindowsInsertCommandSkipsUnenrolledHosts}, {"TestCleanupWindowsMDMCommandQueue", testCleanupWindowsMDMCommandQueue}, {"TestMDMWindowsGetUnlinkedEnrolledDeviceWithDeviceName", testMDMWindowsGetUnlinkedEnrolledDeviceWithDeviceName}, + {"TestMDMWindowsConflictingEnrollmentHardwareID", testMDMWindowsConflictingEnrollmentHardwareID}, {"TestWindowsHostLiteByHardwareSerial", testWindowsHostLiteByHardwareSerial}, {"TestMDMWindowsUnlinkedEnrollmentHardwareSerial", testMDMWindowsUnlinkedEnrollmentHardwareSerial}, {"TestMDMWindowsClaimEnrolledActivity", testMDMWindowsClaimEnrolledActivity}, @@ -8914,3 +8915,98 @@ func testWindowsProfileRetryOnDeviceFailure(t *testing.T, ds *Datastore) { // Terminal failures must reach the rollup that GetMDMWindowsProfilesSummary reads. require.Equal(t, string(fleet.MDMDeliveryFailed), readWindowsProfilesStatusRollup(t, ds)[host.UUID]) } + +func testMDMWindowsConflictingEnrollmentHardwareID(t *testing.T, ds *Datastore) { + ctx := t.Context() + + newEnrollment := func(hostUUID string) *fleet.MDMWindowsEnrolledDevice { + return &fleet.MDMWindowsEnrolledDevice{ + MDMDeviceID: uuid.New().String(), + MDMHardwareID: uuid.New().String() + uuid.New().String(), + MDMDeviceState: microsoft_mdm.MDMDeviceStateEnrolled, + MDMDeviceType: "CIMClient_Windows", + MDMDeviceName: "DESKTOP-CONFLICT", + MDMEnrollType: "AzureADJoin", + MDMEnrollProtoVersion: "5.0", + MDMEnrollClientVersion: "10.0.19045.2965", + HostUUID: hostUUID, + } + } + + hostUUID := uuid.New().String() + + t.Run("an unclaimed host has no conflict", func(t *testing.T) { + conflicted, conflict, err := ds.MDMWindowsConflictingEnrollmentHardwareID(ctx, hostUUID, "some-hardware-id") + require.NoError(t, err) + assert.False(t, conflicted) + assert.Empty(t, conflict) + }) + + t.Run("an empty host uuid has no conflict", func(t *testing.T) { + // Every enrollment starts unlinked, so an empty UUID must never be reported as claimed by all of them. + conflicted, conflict, err := ds.MDMWindowsConflictingEnrollmentHardwareID(ctx, "", "some-hardware-id") + require.NoError(t, err) + assert.False(t, conflicted) + assert.Empty(t, conflict) + }) + + incumbent := newEnrollment(hostUUID) + require.NoError(t, ds.MDMWindowsInsertEnrolledDevice(ctx, incumbent)) + + t.Run("the same hardware re-enrolling is not a conflict", func(t *testing.T) { + conflicted, conflict, err := ds.MDMWindowsConflictingEnrollmentHardwareID(ctx, hostUUID, incumbent.MDMHardwareID) + require.NoError(t, err) + assert.False(t, conflicted, "a device must always be able to reclaim the host it already holds") + assert.Empty(t, conflict) + }) + + t.Run("different hardware claiming the same host conflicts", func(t *testing.T) { + claimant := newEnrollment("") + require.NoError(t, ds.MDMWindowsInsertEnrolledDevice(ctx, claimant)) + + conflicted, conflict, err := ds.MDMWindowsConflictingEnrollmentHardwareID(ctx, hostUUID, claimant.MDMHardwareID) + require.NoError(t, err) + assert.True(t, conflicted) + assert.Equal(t, incumbent.MDMHardwareID, conflict, "the incumbent's hardware id is reported so it can be logged") + }) + + t.Run("re-enrolling the same hardware clears the claim", func(t *testing.T) { + // This is what keeps legitimate re-enrollment from ever colliding with the guard, so it exercises the path + // production actually takes: a re-enrolling device is deleted by hardware ID first + // (MDMWindowsDeleteEnrolledDeviceOnReenrollment) and then inserted fresh, rather than upserting in place. + rowIDForHardware := func() uint { + var id uint + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.GetContext(ctx, q, &id, + `SELECT id FROM mdm_windows_enrollments WHERE mdm_hardware_id = ?`, incumbent.MDMHardwareID) + }) + return id + } + beforeID := rowIDForHardware() + + require.NoError(t, ds.MDMWindowsDeleteEnrolledDeviceOnReenrollment(ctx, incumbent.MDMHardwareID)) + reEnroll := newEnrollment("") + reEnroll.MDMHardwareID = incumbent.MDMHardwareID + require.NoError(t, ds.MDMWindowsInsertEnrolledDevice(ctx, reEnroll)) + + // The row is replaced rather than updated in place, which is what a real wiped device produced. + assert.NotEqual(t, beforeID, rowIDForHardware(), "re-enrollment replaces the row instead of reusing it") + + conflicted, conflict, err := ds.MDMWindowsConflictingEnrollmentHardwareID(ctx, hostUUID, "brand-new-hardware-id") + require.NoError(t, err) + assert.False(t, conflicted, "the incumbent released the host when it re-enrolled") + assert.Empty(t, conflict) + }) + + t.Run("an incumbent with an empty hardware id still conflicts", func(t *testing.T) { + emptyHWHostUUID := uuid.New().String() + emptyHWIncumbent := newEnrollment(emptyHWHostUUID) + emptyHWIncumbent.MDMHardwareID = "" + require.NoError(t, ds.MDMWindowsInsertEnrolledDevice(ctx, emptyHWIncumbent)) + + conflicted, conflict, err := ds.MDMWindowsConflictingEnrollmentHardwareID(ctx, emptyHWHostUUID, "claimant-hardware-id") + require.NoError(t, err) + assert.True(t, conflicted, "an empty-hardware-id incumbent must still block the claim") + assert.Empty(t, conflict, "there is no id to log, which is why conflicted is the answer and not this string") + }) +} diff --git a/server/fleet/datastore.go b/server/fleet/datastore.go index 211085a7a5a..e038e341a49 100644 --- a/server/fleet/datastore.go +++ b/server/fleet/datastore.go @@ -2483,6 +2483,11 @@ type Datastore interface { // MDM enrollment whose device-reported SMBIOS serial matches. Returns a NotFound error when there is none. MDMWindowsGetUnlinkedEnrolledDeviceWithHardwareSerial(ctx context.Context, hardwareSerial string) (*MDMWindowsEnrolledDevice, error) + // MDMWindowsConflictingEnrollmentHardwareID returns the mdm_hardware_id of an enrollment already linked to hostUUID + // that belongs to hardware other than mdmHardwareID, or "" when the host is unclaimed or claimed by this same + // hardware. + MDMWindowsConflictingEnrollmentHardwareID(ctx context.Context, hostUUID string, mdmHardwareID string) (conflicted bool, conflictingHardwareID string, err error) + // MDMWindowsClaimEnrolledActivity claims the right to record the mdm_enrolled activity for the given Windows MDM // enrollment, returning true for the first caller only. MDMWindowsClaimEnrolledActivity(ctx context.Context, mdmHardwareID string, claimedAt time.Time) (bool, error) diff --git a/server/mock/datastore_mock.go b/server/mock/datastore_mock.go index d64591d2db3..d2ec15bfb0f 100644 --- a/server/mock/datastore_mock.go +++ b/server/mock/datastore_mock.go @@ -1458,6 +1458,8 @@ type MDMWindowsSaveUnlinkedEnrollmentHardwareSerialFunc func(ctx context.Context type MDMWindowsGetUnlinkedEnrolledDeviceWithHardwareSerialFunc func(ctx context.Context, hardwareSerial string) (*fleet.MDMWindowsEnrolledDevice, error) +type MDMWindowsConflictingEnrollmentHardwareIDFunc func(ctx context.Context, hostUUID string, mdmHardwareID string) (conflicted bool, conflictingHardwareID string, err error) + type MDMWindowsClaimEnrolledActivityFunc func(ctx context.Context, mdmHardwareID string, claimedAt time.Time) (bool, error) type MDMWindowsReleaseEnrolledActivityClaimFunc func(ctx context.Context, mdmHardwareID string, claimedAt time.Time) error @@ -4548,6 +4550,9 @@ type DataStore struct { MDMWindowsGetUnlinkedEnrolledDeviceWithHardwareSerialFunc MDMWindowsGetUnlinkedEnrolledDeviceWithHardwareSerialFunc MDMWindowsGetUnlinkedEnrolledDeviceWithHardwareSerialFuncInvoked bool + MDMWindowsConflictingEnrollmentHardwareIDFunc MDMWindowsConflictingEnrollmentHardwareIDFunc + MDMWindowsConflictingEnrollmentHardwareIDFuncInvoked bool + MDMWindowsClaimEnrolledActivityFunc MDMWindowsClaimEnrolledActivityFunc MDMWindowsClaimEnrolledActivityFuncInvoked bool @@ -10977,6 +10982,13 @@ func (s *DataStore) MDMWindowsGetUnlinkedEnrolledDeviceWithHardwareSerial(ctx co return s.MDMWindowsGetUnlinkedEnrolledDeviceWithHardwareSerialFunc(ctx, hardwareSerial) } +func (s *DataStore) MDMWindowsConflictingEnrollmentHardwareID(ctx context.Context, hostUUID string, mdmHardwareID string) (conflicted bool, conflictingHardwareID string, err error) { + s.mu.Lock() + s.MDMWindowsConflictingEnrollmentHardwareIDFuncInvoked = true + s.mu.Unlock() + return s.MDMWindowsConflictingEnrollmentHardwareIDFunc(ctx, hostUUID, mdmHardwareID) +} + func (s *DataStore) MDMWindowsClaimEnrolledActivity(ctx context.Context, mdmHardwareID string, claimedAt time.Time) (bool, error) { s.mu.Lock() s.MDMWindowsClaimEnrolledActivityFuncInvoked = true diff --git a/server/service/mdm_test.go b/server/service/mdm_test.go index 7afe6d46e74..4a5020345ad 100644 --- a/server/service/mdm_test.go +++ b/server/service/mdm_test.go @@ -5036,6 +5036,12 @@ func TestProcessIncomingMDMCmdsDevDetailLinkage(t *testing.T) { ds.MDMWindowsGetEnrolledDeviceWithDeviceIDFunc = func(_ context.Context, _ string) (*fleet.MDMWindowsEnrolledDevice, error) { return &fleet.MDMWindowsEnrolledDevice{MDMDeviceID: testDeviceID, MDMHardwareID: testHardwareID, MDMEnrollUserID: ""}, nil } + // No incumbent holds the host, which is the ordinary case. + ds.MDMWindowsConflictingEnrollmentHardwareIDFunc = func(_ context.Context, hostUUID, mdmHardwareID string) (bool, string, error) { + assert.Equal(t, testHostUUID, hostUUID) + assert.Equal(t, testHardwareID, mdmHardwareID) + return false, "", nil + } } t.Run("unlinked enrollment: Get for DevDetail SMBIOSSerialNumber is injected", func(t *testing.T) { @@ -5086,6 +5092,40 @@ func TestProcessIncomingMDMCmdsDevDetailLinkage(t *testing.T) { assert.False(t, hasGetForDevDetailSerial(cmds), "after successful linkage, no further Get should be injected") }) + t.Run("serial claims a host already held by other hardware: refused", func(t *testing.T) { + svc, ds, _, ctx := newSvc(t) + // A second device reporting the victim's serial. Nothing corroborates the claim, so it must not take the host. + claimantHardwareID := "claimant-hardware-id" + enrolledDevice := &fleet.MDMWindowsEnrolledDevice{MDMDeviceID: testDeviceID, MDMHardwareID: claimantHardwareID, HostUUID: ""} + stubLink(t, ds, true) + ds.MDMWindowsConflictingEnrollmentHardwareIDFunc = func(_ context.Context, hostUUID, mdmHardwareID string) (bool, string, error) { + assert.Equal(t, testHostUUID, hostUUID) + assert.Equal(t, claimantHardwareID, mdmHardwareID) + return true, testHardwareID, nil + } + + _, err := svc.processIncomingMDMCmds(ctx, enrolledDevice, buildReqMsg(t, serialResults(testSerial)), RequestAuthStateTrusted) + require.NoError(t, err, "the session must continue; only the link is refused") + assert.True(t, ds.MDMWindowsConflictingEnrollmentHardwareIDFuncInvoked) + assert.False(t, ds.UpdateMDMWindowsEnrollmentsHostUUIDFuncInvoked, "the host must not be relinked to the claimant") + assert.Empty(t, enrolledDevice.HostUUID, "in-memory HostUUID must not be set from a refused claim") + assert.False(t, ds.MDMWindowsSaveUnlinkedEnrollmentHardwareSerialFuncInvoked, + "the refused serial must not be persisted, or the orbit reverse-link path inherits the same bad claim") + }) + + t.Run("conflict lookup fails: link is refused rather than allowed", func(t *testing.T) { + svc, ds, _, ctx := newSvc(t) + enrolledDevice := &fleet.MDMWindowsEnrolledDevice{MDMDeviceID: testDeviceID, MDMHardwareID: testHardwareID, HostUUID: ""} + stubLink(t, ds, true) + ds.MDMWindowsConflictingEnrollmentHardwareIDFunc = func(_ context.Context, _, _ string) (bool, string, error) { + return false, "", errors.New("db is down") + } + + _, err := svc.processIncomingMDMCmds(ctx, enrolledDevice, buildReqMsg(t, serialResults(testSerial)), RequestAuthStateTrusted) + require.NoError(t, err) + assert.False(t, ds.UpdateMDMWindowsEnrollmentsHostUUIDFuncInvoked, "an unreadable guard must fail closed") + }) + t.Run("serial with no matching host (NotFound): Get is reinjected for retry", func(t *testing.T) { svc, ds, _, ctx := newSvc(t) enrolledDevice := &fleet.MDMWindowsEnrolledDevice{MDMDeviceID: testDeviceID, MDMHardwareID: testHardwareID, HostUUID: ""} diff --git a/server/service/microsoft_mdm.go b/server/service/microsoft_mdm.go index 2aaea3525ab..871234e67d8 100644 --- a/server/service/microsoft_mdm.go +++ b/server/service/microsoft_mdm.go @@ -1786,6 +1786,27 @@ scan: } return false } + // The serial arrives in the device's own DevDetail response and nothing corroborates it, so it must not be able to + // take over a host that already belongs to different hardware. Refusing here costs the device nothing: the fleetd + // installer is enqueued by MDM device ID, so an unlinked enrollment still receives it, and osquery's + // directIngestMDMDeviceIDWindows backstop then links this enrollment to whichever host actually reports this MDM + // device ID. + conflicted, conflictingHardwareID, err := svc.ds.MDMWindowsConflictingEnrollmentHardwareID(ctx, host.UUID, enrolledDevice.MDMHardwareID) + if err != nil { + svc.logger.ErrorContext(ctx, "windows mdm: conflicting enrollment lookup failed", + "err", err, "device_id", enrolledDevice.MDMDeviceID) + ctxerr.Handle(ctx, err) + return false + } + if conflicted { + svc.logger.WarnContext(ctx, "windows mdm: refusing to link enrollment to a host already claimed by other hardware", + "device_id", enrolledDevice.MDMDeviceID, + "hardware_serial", serial, + "host_uuid", host.UUID, + "claimed_by_hardware_id", conflictingHardwareID) + return false + } + updated, err := osquery_utils.LinkWindowsHostMDMEnrollment(ctx, svc.logger, svc.ds, host.ID, host.UUID, enrolledDevice.MDMDeviceID) if err != nil { svc.logger.ErrorContext(ctx, "windows mdm: link by DevDetail failed", "err", err, "device_id", enrolledDevice.MDMDeviceID) diff --git a/server/service/orbit.go b/server/service/orbit.go index 45214bf3ea5..bfab82e9ebf 100644 --- a/server/service/orbit.go +++ b/server/service/orbit.go @@ -370,14 +370,27 @@ func (svc *Service) EnrollOrbit(ctx context.Context, hostInfo fleet.OrbitHostInf svc.logger.ErrorContext(ctx, "failed to look up unlinked windows mdm enrollment by serial", "err", err, "host_uuid", host.UUID, "hardware_serial", hostInfo.HardwareSerial) case err == nil: - if _, err := osquery_utils.LinkWindowsHostMDMEnrollment(ctx, svc.logger, svc.ds, host.ID, host.UUID, device.MDMDeviceID); err != nil { - svc.logger.ErrorContext(ctx, "failed to reverse-link windows mdm enrollment at orbit enroll", - "err", err, "host_uuid", host.UUID, "device_id", device.MDMDeviceID) - } else { - // The enrollment predates this host record, so its mdm_enrolled activity was deferred; record it now - // that there is a host to attribute it to, rather than waiting for the next management session. - device.HostUUID = host.UUID - svc.maybeCreateWindowsMDMEnrolledActivity(ctx, device) + // Same trust as the DevDetail path this mirrors: the serial on the unlinked enrollment was asserted by the + // device, so it must not claim a host that already belongs to different hardware. + conflicted, conflictingHardwareID, cErr := svc.ds.MDMWindowsConflictingEnrollmentHardwareID(ctx, host.UUID, device.MDMHardwareID) + switch { + case cErr != nil: + svc.logger.ErrorContext(ctx, "failed to check for conflicting windows mdm enrollment at orbit enroll", + "err", cErr, "host_uuid", host.UUID, "device_id", device.MDMDeviceID) + case conflicted: + svc.logger.WarnContext(ctx, "refusing to reverse-link windows mdm enrollment to a host already claimed by other hardware", + "host_uuid", host.UUID, "device_id", device.MDMDeviceID, + "hardware_serial", hostInfo.HardwareSerial, "claimed_by_hardware_id", conflictingHardwareID) + default: + if _, err := osquery_utils.LinkWindowsHostMDMEnrollment(ctx, svc.logger, svc.ds, host.ID, host.UUID, device.MDMDeviceID); err != nil { + svc.logger.ErrorContext(ctx, "failed to reverse-link windows mdm enrollment at orbit enroll", + "err", err, "host_uuid", host.UUID, "device_id", device.MDMDeviceID) + } else { + // The enrollment predates this host record, so its mdm_enrolled activity was deferred; record it now + // that there is a host to attribute it to, rather than waiting for the next management session. + device.HostUUID = host.UUID + svc.maybeCreateWindowsMDMEnrolledActivity(ctx, device) + } } // A Windows orbit enrollment is not linked when it is not MDM, when it is already linked, or when it is a // programmatic fleetd-first enrollment. Note this matches on serial alone, so the lookup refuses when several diff --git a/server/service/orbit_eua_test.go b/server/service/orbit_eua_test.go index 5cd061ce101..6f5a7d88929 100644 --- a/server/service/orbit_eua_test.go +++ b/server/service/orbit_eua_test.go @@ -351,6 +351,10 @@ func TestEnrollOrbitWindowsReverseLink(t *testing.T) { inner.MDMWindowsClaimEnrolledActivityFunc = func(ctx context.Context, mdmHardwareID string, claimedAt time.Time) (bool, error) { return true, nil } + // No incumbent holds the host by default; the conflict subtest overrides this. + inner.MDMWindowsConflictingEnrollmentHardwareIDFunc = func(ctx context.Context, hostUUID, mdmHardwareID string) (bool, string, error) { + return false, "", nil + } return svc, ds, serverOpts } @@ -462,4 +466,27 @@ func TestEnrollOrbitWindowsReverseLink(t *testing.T) { require.Equal(t, testSerial, *enrolledActivity.HostSerial) require.Equal(t, fleet.MDMPlatformMicrosoft, enrolledActivity.MDMPlatform) }) + + t.Run("unlinked enrollment claims a host held by other hardware: refused", func(t *testing.T) { + svc, ds, _ := newSvc(t) + // The serial on the unlinked enrollment was asserted by that device over OMA-DM. If the host it names already + // belongs to different hardware, this reverse-link must not hand the host over. + ds.MDMWindowsGetUnlinkedEnrolledDeviceWithHardwareSerialFunc = func(ctx context.Context, serial string) (*fleet.MDMWindowsEnrolledDevice, error) { + return &fleet.MDMWindowsEnrolledDevice{ + ID: 1, MDMDeviceID: "device-1", MDMHardwareID: "claimant-hardware-id", MDMEnrollUserID: "user@example.com", + }, nil + } + ds.MDMWindowsConflictingEnrollmentHardwareIDFunc = func(ctx context.Context, hostUUID, mdmHardwareID string) (bool, string, error) { + require.Equal(t, "host-uuid-1", hostUUID) + require.Equal(t, "claimant-hardware-id", mdmHardwareID) + return true, "incumbent-hardware-id", nil + } + + nodeKey, err := svc.EnrollOrbit(t.Context(), hostInfo, "secret", "") + require.NoError(t, err, "orbit enrollment itself must still succeed; only the reverse-link is refused") + require.NotEmpty(t, nodeKey) + require.True(t, ds.MDMWindowsConflictingEnrollmentHardwareIDFuncInvoked) + require.False(t, ds.UpdateMDMWindowsEnrollmentsHostUUIDFuncInvoked, "the host must not be relinked to the claimant") + require.False(t, ds.AddHostsToTeamFuncInvoked, "a refused link must not carry the default fleet with it") + }) }