No Race Condition Handling: Two parallel admin-actions at the same time: 2nd Update overwrites the first update

[fashberg]

Description

Is it true that OPNsense lacks general file/config locking for saving configurations?

It appears that if two users are logged into OPNsense simultaneously and both click 'save' at the same time, their configuration updates might overwrite each other.

In the attached screenshot, user tf created an OTP seed, but at the same moment, another user tik created his OTP seed, which overwrote tf's seed, effectively emptying it. Consequently, tf reported to IT support that his OTP codes were not working. This incident clarified the issue.

Background:

We use OPNsense-OpenVPN with Active Directory TOTP Authentication. Our users can self-onboard the required TOTP seed by accessing OPNsense with AD credentials while being in the HQ network. All users have the OPNsense privilege "GUI/System: User Password Manager."

This setup works well, except for this minor bug. After many successful onboardings, this issue has occurred only once. However, it is quite concerning to find out that OPNsense lacks protection against such race conditions.

Code Analysis:

Upon examining the code I noticed that the configuration gets loaded during startup. During a POST request with 'request_otp_seed' set, write_config() writes the entire configuration at line 76 without any checks https://github.com/opnsense/core/blob/master/src/www/system_usermanager_passwordmg.php#L76

Timing Issue:

1. User Alice POSTs to system_usermanager_passwordmg.php, and config Version 1 is loaded.
2. User Bob POSTs to system_usermanager_passwordmg.php, and config Version 1 is loaded.
3. Alice's POST request finishes, and write_config writes a new config Version 2 to file, containing Alice's changes.
4. Bob's POST request finishes, and write_config() writes Version 3, containing Bob's changes but not Alice's, as it is based on Version 1.

Since system_usermanager_passwordmg.php lacks locking, I assume this is a general bug that can occur during any admin actions.

Question:

Shouldn't the system be able to handle such scenarios to prevent race conditions?

Environment

OPNsense 24.7.3_1 (amd64). Proxmox hosted

Important notices

Before you add a new report, we ask you kindly to acknowledge the following:

• [x] I have read the contributing guide lines at https://github.com/opnsense/core/blob/master/CONTRIBUTING.md
• [x] I am convinced that my issue is new after having checked both open and closed issues at https://github.com/opnsense/core/issues?q=is%3Aissue

[fichtner]

For URLs ending with .php that is likely still the case. It's a long term goal.