Overview: When a client IP is removed from a share's "hosts allow" list and the configuration is reloaded via "smbcontrol all reload-config", existing SMB sessions from that client are NOT re-validated. The client can continue to mount new shares and perform read/write operations on the server, effectively bypassing the updated access control policy. This is a security-relevant issue: it allows a client whose access has been explicitly revoked to retain access until either the smbd daemon is restarted or the client fully disconnects all sessions. Steps to Reproduce: 1. Configure a share with "hosts allow" containing the client's IP. Example: [testshare] path = /srv/testshare read only = no hosts deny = ALL hosts allow = 10.64.3.122 127.0.0.1 2. From client (IP 10.64.3.122), mount the share: mount -t cifs //server/testshare /mnt/test -o username=user 3. On the server, edit smb.conf to remove 10.64.3.122 from "hosts allow": hosts allow = 127.0.0.1 4. Reload the configuration: smbcontrol all reload-config 5. From the same client (without unmounting), try to: a) Create or modify a file in the mounted share. b) Mount a different share on the same server. Actual Results: Both operations succeed. The client retains full access to the server despite being removed from the "hosts allow" list. Expected Results: The client should be denied access immediately after the configuration reload. Existing sessions should be re-validated against the new "hosts allow" policy, and any session that no longer passes should be terminated. Build Date & Hardware: Verified against current Samba master branch (as of September 2026), built from source on Linux (x86_64). Tested with SMB2/SMB3 protocol (min protocol = SMB2, max protocol = SMB3). Additional Builds and Platforms: Not tested on other versions or platforms. Based on code inspection, the underlying session/IP caching mechanism (sconn->remote_hostname) has been present since SMB2 support was introduced, so the issue is likely to affect other Samba 4.x versions as well, but this has not been empirically verified. Additional Information: Root cause analysis: In Samba, the client IP is cached in sconn->remote_hostname at session setup time. Subsequent TreeConnect requests (i.e. mounting new shares) call share_sanity_checks() with this cached value. The function allow_access() then compares the cached IP against the current "hosts allow" list. When "smbcontrol all reload-config" is issued, reload_services() reloads the configuration from disk (updating lp_hosts_allow()), but it does NOT re-validate existing sessions or refresh the cached sconn->remote_hostname. As a result, existing sessions continue to pass the access check using the stale cached IP. This is similar in nature to CVE-2025-0620 (Samba Kerberos session re-authentication does not update group membership), which was rated as a medium-severity issue. Both are "authorization change not immediately enforced" class issues. We have developed and validated a patch that: 1. Tracks which shares have had "hosts allow/deny" modified during reload-config. 2. Fetches the real-time client IP from the TCP socket (sconn->remote_address) during reload. 3. Re-runs allow_access() for affected shares and terminates any TCP connection that no longer passes the updated policy. The patch introduces a new parametric option "reload:immediate_hosts_check" (default: no, for backward compatibility). The patch will be attached separately. We are willing to work with the Samba team to adapt it for current master.
Created attachment 19212 [details] Patch: enforce hosts allow/deny changes on existing sessions after reload-config Proposed patch. Adds change tracking for hosts allow/deny, re-validates existing sessions on reload, and terminates unauthorized connections.
Created attachment 19213 [details] Reproduction log: client 100.73.8.3 retains access after reload-config Log showing the bug: after removing client IP and running 'smbcontrol all reload-config', client 100.73.8.3 can still read/write the share.
Created attachment 19214 [details] Fixed log: client 100.73.8.3 terminated after reload-config with patch applied Log showing the fix: after applying the patch, the same test now terminates the client connection immediately after reload-config.
Created attachment 19215 [details] smb.conf before reload (hosts allow includes client IP) Configuration used for reproduction, before removing the client IP.
Created attachment 19216 [details] smb.conf after reload (hosts allow excludes client IP) Configuration after removing the client IP, used to trigger the reload.
Correction to the description: The parametric option name is "reload:instant_access_check" (not "reload:immediate_hosts_check" as originally written). The patch and all validation logs use "reload:instant_access_check".
Wouldn't it be sufficient to just recheck the options against the current configuration? Why such a large infrastructure maintaining changed parameters?
(In reply to Volker Lendecke from comment #7) Thank you for the review, Volker. I understand the simpler full re-check approach. However, there is a scenario where change tracking matters: A client may have multiple shares mounted over a single TCP connection. If we re-check all shares on every reload, a share whose "hosts allow" was NOT modified in this reload could still fail the check (e.g. due to a pre-existing config issue). This would cause the entire TCP connection to be terminated, affecting all other shares that the client has mounted. The change tracking ensures we only terminate the connection when a share whose "hosts allow/deny" was actually modified fails the check. That said, I'm open to simplifying if the team prefers. Please let me know which approach is preferred. Best regards, Weiqiang Meng
You could hook into the flow of msg_force_tdis_denied to disconnect shares that are disallowed by current status.
(In reply to Volker Lendecke from comment #9) Thank you for the suggestion, Volker. I will look into hooking into the msg_force_tdis_denied flow to disconnect only the shares that are no longer allowed by the current configuration. I'll verify this approach locally and update the patch accordingly. Best regards, Weiqiang Meng
Created attachment 19217 [details] Patch v2: use conn_force_tdis() to disconnect only disallowed shares Updated patch per Volker's suggestion. Uses conn_force_tdis() to disconnect only the share that fails the hosts allow/deny check, without terminating the whole TCP connection.
Created attachment 19218 [details] Fixed log v2: only disallowed share disconnected, other shares remain accessible Log showing that only testshare is disconnected while testshare2 remains accessible.
Thank you for the suggestion, Volker. I have updated the patch to use conn_force_tdis() instead of smbd_server_connection_terminate(). With this approach, only the specific share that fails the hosts allow/deny check is disconnected, while other shares on the same TCP connection remain accessible. Verification: - A client mounted both testshare and testshare2. - Only testshare's "hosts allow" was modified to remove the client IP. - After "smbcontrol all reload-config": - testshare was disconnected (conn_force_tdis: Forcing close of share 'testshare') - testshare2 remained fully accessible New patch (v2) and verification log are attached. Best regards, Weiqiang Meng
To be honest, instead of seeing this with a new parameter I would rather put this into the "smbcontrol smbd close-denied-share" as a mandatory new behavior without a parameter. For reload-config we don't check what close-denied-share checks and this could also be seen as a similar security problem.
(In reply to Volker Lendecke from comment #14) Thank you for the clarification, Volker. I understand the direction now: instead of a new parameter, the hosts allow/deny re-check should be part of the existing close-denied-share flow, and reload-config should also perform the same check. I have looked at the relevant code and here is what I think the change should look like: 1. Extend force_tdis_denied_check() in conn_msg.c to also call allow_access() with the real-time client address from conn->sconn->remote_address. If the check fails, return true so that conn_force_tdis() disconnects the share. 2. In reload_services(), after the configuration is reloaded, unconditionally trigger the same check on all current connections (i.e., equivalent to running close-denied-share for each share), so that the behavior is consistent and does not depend on a new parameter. Before I update the patch, could you please confirm a couple of points: - For reload-config, should we always run the check for all connections, or only when some hosts allow/deny parameter actually changed? - Should the "close-denied-share" behavior be extended in the same patch, or would you prefer that as a separate change? I'm happy to implement whichever way you prefer. Best regards, Weiqiang Meng
Please leave reload-config untouched. Yes, this might be flagged by your compliance team as a security vulnerability, but I would rather leave this open and refer to the documentation that reload-config is not sufficient to be compliant with all new security restrictions, a subsequent close-denied-share message is required to be compliant with your security requirements.
Created attachment 19219 [details] Patch v3: extend close-denied-share to check hosts allow/deny Updated patch per Volker's suggestion. reload-config is left untouched; close-denied-share now also checks hosts allow/deny using the real-time client address.
Created attachment 19220 [details] Fixed log v3: close-denied-share disconnects testshare, testshare2 remains accessible Log showing that after reload-config and close-denied-share, only testshare is disconnected while testshare2 remains accessible.
Thank you for the clarification, Volker. I have updated the patch as you requested: - reload-config is left untouched. - force_tdis_denied_check() in conn_msg.c now also re-checks "hosts allow/deny" using the real-time client address from conn->sconn->remote_address. Shares that are no longer allowed are disconnected when "smbcontrol smbd close-denied-share" is invoked. Verification: - A client mounted both testshare and testshare2. - testshare's "hosts allow" was modified to remove the client IP. - After "smbcontrol all reload-config" and then "smbcontrol smbd close-denied-share testshare": - testshare was disconnected (Hosts allow/deny denied share testshare for client 100.73.8.3) - testshare2 remained fully accessible Patch v3 and the verification log are attached. Best regards, Weiqiang Meng
(In reply to mengweiqiang2023 from comment #17) > Created attachment 19219 [details] > Patch v3: extend close-denied-share to check hosts allow/deny > > Updated patch per Volker's suggestion. reload-config is left untouched; > close-denied-share now also checks hosts allow/deny using the real-time > client address. Yep, this is how I would like to see addressed. The only last question I have is: I don't think it can happen that either conn->sconn or conn->sconn->remote_address can ever be NULL, so I think those if-statements should not be necessary. Can you check the code paths where those pointers can be NULL?
Created attachment 19226 [details] Patch v4: remove unnecessary NULL checks in force_tdis_denied_check() Updated patch per Volker's review. Removed the unnecessary NULL checks for conn->sconn and conn->sconn->remote_address, since they cannot be NULL in this code path.
Created attachment 19227 [details] Fixed log v4: close-denied-share disconnects testshare, testshare2 remains accessible Verification log for v4: testshare is disconnected while testshare2 remains accessible.
(In reply to Volker Lendecke from comment #20) (In reply to Volker Lendecke from comment #20) Thank you for the review. I checked the code paths: - conn->sconn cannot be NULL here: conn_force_tdis() iterates over sconn->connections, and conn_new() always sets conn->sconn before adding the conn to that list. The destructor only clears conn->sconn after the conn is removed from the list. - conn->sconn->remote_address cannot be NULL here: smbd_process() assigns sconn->remote_address via tsocket_address_copy() and calls exit_server_cleanly() on failure, so it is always valid before the event loop that handles close-denied-share runs. You are right - the two if-statements are unnecessary. Patch v4 attached with those checks removed. Best regards, Weiqiang Meng
Can you p lease explain why you removed the comment in the last version of the patch? You did not remove it before, so I wonder what changed in this regard between those two versions. Please explain your personal view on this.
Created attachment 19228 [details] Patch v5: restore the "all still ok" comment Restored the comment that was accidentally removed in v4.
(In reply to Volker Lendecke from comment #24) Thank you for catching this. I removed the comment by mistake while updating the function. It was not an intentional change. I agree the comment is useful to explain that returning false means the connection is still allowed. I have restored it in patch v5, which is attached. Best regards, Weiqiang Meng
(In reply to mengweiqiang2023 from comment #25) > Created attachment 19228 [details] > Patch v5: restore the "all still ok" comment > > Restored the comment that was accidentally removed in v4. Another one: tsocket_address_inet_addr_string can return NULL.
(In reply to Volker Lendecke from comment #27) You are right - tsocket_address_inet_addr_string() can return NULL. I can see two ways to handle this: 1. If raddr is NULL, conservatively close the share (return true) and log an error. This is safe and simple. 2. If raddr is NULL, fall back to conn->sconn->remote_hostname for the allow_access() check. But remote_hostname is the cached value, which is exactly what we are trying to avoid using here. I would prefer option 1: if we cannot determine the real client address, we should not keep the share open. Before I update the patch, could you let me know how you would like this handled? If you have a different approach in mind, please tell me and I will follow your suggestion. Best regards, Weiqiang Meng
(In reply to mengweiqiang2023 from comment #28) > I can see two ways to handle this: I don't have a clear opinion. I'd lean to closing the share.
(In reply to Volker Lendecke from comment #29) (In reply to Volker Lendecke from comment #29) Thanks. I will go with closing the share when the client address cannot be resolved. Specifically: if tsocket_address_inet_addr_string() returns NULL, force_tdis_denied_check() will log an error and return true to close the share, without calling allow_access(). After that check, the existing hosts allow/deny re-check and the share_access/read_only checks remain unchanged. I will post patch v6 with this change shortly. Best regards, Weiqiang Meng
Created attachment 19229 [details] Patch v6: close the share if the client address cannot be resolved Updated patch: if tsocket_address_inet_addr_string() returns NULL, conservatively close the share and log an error, avoiding passing NULL to allow_access().
(In reply to Volker Lendecke from comment #29) Thanks. I went with closing the share when the client address cannot be resolved. Patch v6 attached. Best regards, Weiqiang Meng
(In reply to Volker Lendecke from comment #29) Just a gentle ping on this one. Is there anything else you'd like me to adjust in patch v6? Thanks, Weiqiang Meng
No, I like this patch now. But the Samba Team process requires that two Samba Team members review a patch, and we now have to wait for another Samba Team member looks at it.