Assets Users Showed Other Companies - edmozley/freeitsm GitHub Wiki

Assets → Users showed people from other companies

Fixed in 3.0.0 (changelog #2095) · Present in 1.0.0 to 2.10.2 · Security · found while building the People module (#153)

If you run more than one company and limit some analysts to some of them, upgrade to 3.0.0.

What could happen

On an install with more than one company, an analyst who could only see some companies could still read any person's details in Assets → Users:

  • email address
  • phone and mobile numbers
  • job title, department and office
  • employee number
  • manager

They only had to open someone they could see, then change the person's number in the request behind the screen. The equipment the person held was hidden correctly; the person themselves was not. The same details also went into the handover document and the handover email, which are built from the same request.

Installs with a single company were not affected: every analyst sees everyone there anyway.

Why

One request (api/assets/get_user_assets.php) answers this person and everything assigned to them. It feeds the detail panel on Assets → Users and the handover document, so the two can never disagree. Behind it, AssetsService::assetsForUser():

  1. read the person by id: contact details, job, employee number and manager, with no company check
  2. read the person's assets, correctly limited to the analyst's companies
  3. read the people who report to them, also with no company check

The list of people on the screen was scoped properly, which is why nobody saw anything wrong. Only a request for a specific id, which the screen never makes for someone outside your companies, reached past it.

The fix

public static function assetsForUser(PDO $conn, ActorContext $ctx, int $userId): ?array
{
    // The person first. Out of reach reads exactly like "no such person".
    if (!analystCanAccessUser($conn, $ctx->actorId, $userId)) {
        return null;
    }
    ...
    // A manager's reports, scoped like the list: a report in a company the
    // reader cannot see is not shown (a manager can have reports in several).
    [$repTenantSql, $repTenantArgs] = activeTenantFilter($conn, $ctx->actorId, 'r');
  • The person is checked first, with analystCanAccessUser(): the person's company (or the Default company, for nobody's) must be one the analyst can access. If not, the answer is Unknown user, exactly as for an id that doesn't exist, so the request can't be used to discover who exists.
  • A manager's reports are filtered to the analyst's companies, the same way the list is.

The new People module reads people through the same checks.

How it was verified

tests/people-scope.php sets up an analyst limited to one company and calls assetsForUser(), the function behind both the screen and the handover, for a person in another company. It fails on the old code and passes on the new. It also checks that a manager's reports in another company are not listed. The positive controls are that the same analyst still gets a person in their own company, and reports in their own company. The test runs inside a transaction and leaves nothing behind.

What this means for you

  • One company: nothing to do. You were never affected.
  • Several companies, with every analyst able to see them all: nothing was exposed in practice, but upgrade anyway.
  • Several companies, with analysts limited to some: upgrade. An analyst limited to some companies could have read contact details for people in the others. There is no log of the requests that would show whether anyone did.

Related