SystemAuthenticator.begin()/end() can result in priviledge escalation

[t.vignal (thibaud altereo)]

Title: SystemAuthenticator.begin()/end() mutate the shared SecurityContext: concurrent threads of the same HTTP session run as system, and a session can remain system permanently

Environment

Jmix version: 2.8.2 (code unchanged in 2.8.5, 3.0.2 and master)

Bug Description

SystemAuthenticatorImpl.begin() and end() (and therefore withSystem, runWithSystem, withUser, runWithUser) change the authentication through SecurityContextHelper.setAuthentication():

SecurityContextHolder.getContext().setAuthentication(authentication);

This replaces the authentication inside the existing SecurityContext instance instead of setting a new context on the current thread. This SecurityContext instance is not local to the thread. The same object is shared by:

• every request of the same HTTP session, because HttpSessionSecurityContextRepository returns the object stored in the session attribute, not a copy;
• background tasks started from the UI: UiAsyncTasks wraps them in DelegatingSecurityRunnable / DelegatingSecuritySupplier built with SecurityContextHolder.getContext();
• push requests (SecurityContextHolderAtmosphereInterceptor).

As a result, a withSystem { } block running on one thread changes the authentication seen by all other threads of the same session while it runs.

Spring Security explicitly warns against this pattern (Servlet Authentication Architecture): create a new SecurityContext instead of calling SecurityContextHolder.getContext().setAuthentication(authentication) “to avoid race conditions across multiple threads”.

There are two consequences.

1. Temporary privilege escalation. While a withSystem block runs in a background task, UI requests of the same session run with the system authentication: full permissions, row-level policies bypassed, createdBy/lastModifiedBy and audit recorded as system, and details lost (for example ClientDetails.sessionId).

2. Permanent privilege escalation. When two begin()/end() pairs overlap on two threads of the same session, the per-thread stack in SystemAuthenticatorSupport restores the wrong value:

   Step	Thread	Stack of the thread	Shared context after the step
   1	T1 begin()	pushes admin	system
   2	T2 begin()	pushes system (what it currently sees)	system
   3	T1 end()	pops admin	admin
   4	T2 end()	pops system	system

   From then on, every request of this session runs as system until logout.

Other sessions are not affected: each session has its own SecurityContext instance.

Steps To Reproduce

In any view of a project with a logged-in user admin:

@Autowired private UiAsyncTasks uiAsyncTasks;
@Autowired private SystemAuthenticator systemAuthenticator;
@Autowired private CurrentAuthentication currentAuthentication;
@Autowired private Notifications notifications;

// Scenario 1: temporary escalation
@Subscribe(id = "longSystemTaskBtn", subject = "clickListener")
public void onLongSystemTaskBtnClick(ClickEvent<JmixButton> event) {
    uiAsyncTasks.runnableConfigurer(() ->
            systemAuthenticator.runWithSystem(() -> sleep(10_000)))
        .runAsync();
}

@Subscribe(id = "whoAmIBtn", subject = "clickListener")
public void onWhoAmIBtnClick(ClickEvent<JmixButton> event) {
    notifications.show(currentAuthentication.getAuthentication().getClass().getSimpleName()
            + " / " + currentAuthentication.getUser().getUsername());
}

// Scenario 2: permanent escalation
@Subscribe(id = "overlappingTasksBtn", subject = "clickListener")
public void onOverlappingTasksBtnClick(ClickEvent<JmixButton> event) {
    uiAsyncTasks.runnableConfigurer(() ->
            systemAuthenticator.runWithSystem(() -> sleep(3_000)))   // T1: ends first
        .runAsync();
    uiAsyncTasks.runnableConfigurer(() -> {
            sleep(1_000);                                            // T2: begins after T1
            systemAuthenticator.runWithSystem(() -> sleep(5_000));   // and ends after T1
        })
        .runAsync();
}

private static void sleep(long millis) {
    try {
        Thread.sleep(millis);
    } catch (InterruptedException e) {
        Thread.currentThread().interrupt();
    }
}

1. Log in as admin and click whoAmI. It shows UsernamePasswordAuthenticationToken / admin.
2. Click longSystemTask, then click whoAmI within 10 seconds.
3. Wait 10 seconds and click whoAmI again.
4. Click overlappingTasks, wait 10 seconds, then click whoAmI and open any view that admin is not allowed to open.

Current Behavior

• Step 2 shows SystemAuthenticationToken / system: the UI request runs as system.
• Step 3 shows admin again.
• Step 4 still shows SystemAuthenticationToken / system after both tasks have finished. The session stays system (with all its permissions) until logout.

Expected Behavior

begin()/end() affect only the current thread. Other threads of the same session always see the user’s own authentication, and overlapping begin()/end() pairs never leave the session with another authentication.

Suggested Fix

In begin(), save the current SecurityContext instance and set a new context on the thread:

SecurityContext context = SecurityContextHolder.createEmptyContext();
context.setAuthentication(authentication);
SecurityContextHolder.setContext(context);

In end(), restore the saved instance with SecurityContextHolder.setContext(previous) (or clearContext() when there was none) instead of calling setAuthentication() on the current context. SystemAuthenticatorSupport would then keep a stack of SecurityContext instead of Authentication.

Note that `setContext()` alone is not enough in a Flow UI application. `FlowuiVaadinWebSecurity` extends `VaadinWebSecurity`, which imports `VaadinAwareSecurityContextHolderStrategyConfiguration`. The resulting `VaadinAwareSecurityContextHolderStrategy.getContext()` returns the `SecurityContext` stored in the HTTP session whenever`VaadinSession.getCurrent()` is set, and ignores what the thread set with `setContext()`. On UI threads, `withSystem` would then have no effect at all. The context set by `begin()` must take precedence over the session context, for example with a strategy that wraps the Vaadin one and lets `begin()`/`end()` push and pop a thread-local context that `getContext()` returns first.

SecurityContextHelper.setAuthentication() is also used this way in BackgroundWorkerImpl, so it may need the same review.

Related older report with the same kind of symptom (“Access denied” on every view after begin()/end()): jmix-projects/jmix-security#69.

Sample Project

Not attached: the snippet above is the whole reproducer.

[krivopustov (Konstantin Krivopustov)]

Thank you for the report and detailed description.
Created issue: SystemAuthenticator.begin()/end() affect other threads of the same HTTP session · Issue #5691 · jmix-framework/jmix · GitHub, will be fixed in the next patches.

Regards,
Konstantin