Bug 13340 - reset on zero vc option resets the wrong client
Summary: reset on zero vc option resets the wrong client
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.7.6
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Volker Lendecke
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-19 17:33 UTC by Joe Frank
Modified: 2019-08-01 14:01 UTC (History)
2 users (show)

See Also:


Attachments
Patch to session setup for reset on zero vc (450 bytes, patch)
2018-03-19 17:33 UTC, Joe Frank
no flags Details
Patch (1.30 KB, patch)
2018-03-26 10:20 UTC, Volker Lendecke
no flags Details
patch (2.49 KB, patch)
2018-03-30 02:57 UTC, Volker Lendecke
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joe Frank 2018-03-19 17:33:57 UTC
Created attachment 14057 [details]
Patch to session setup for reset on zero vc

When "reset on zero vc" is enabled clients are being reset incorrectly when a new client connects using an IP address that's a substring of the existing client's address. For example, if a session is created by a client at 192.168.25.25, followed by a client at 192.168.25.2, the session for the client at 192.168.25.25 will be reset. This appears to be the result of using strstr to check for a match.

The error exists in 4.7.6 and has existed at least as far back as 4.5.

I've attached a patch that resolves the issue.
Comment 1 Volker Lendecke 2018-03-26 10:20:22 UTC
Created attachment 14083 [details]
Patch

Thanks for the report. Your patch has one small flaw: We don't use in-function declarations. See my attached proposal. Does that also help?

I have one worry: What about IPv6? I think both our patches *might* not be correct for that. On the other hand: Is IPv6 and SMB1 really an issue?
Comment 2 Stefan Metzmacher 2018-03-26 12:13:46 UTC
(In reply to Volker Lendecke from comment #1)

smbXsrv_session_add_channel() uses tsocket_address_string(),
while setup_new_vc_session() uses tsocket_address_inet_addr_string().

I think setup_new_vc_session() could also use tsocket_address_string()
and shutdown_other_smbds() would just need 'strcmp()'.
Comment 3 Stefan Metzmacher 2018-03-26 12:15:36 UTC
(In reply to Stefan Metzmacher from comment #2)

Ok, that's wrong, it includes the port number, which should be ignored.
Comment 4 Stefan Metzmacher 2018-03-26 12:17:53 UTC
strncmp() together with strrchr(':') to remove everything including and after the last ':' could work.
Comment 5 Volker Lendecke 2018-03-30 02:57:29 UTC
Created attachment 14095 [details]
patch

This way?
Comment 6 Stefan Metzmacher 2018-03-30 22:06:00 UTC
Comment on attachment 14095 [details]
patch

Looks good, thanks!

Joe can you give it a try and check if it works for you?
Comment 7 Volker Lendecke 2018-05-13 19:00:22 UTC
Pushed to master with metze's RB+ from this bug
Comment 8 Stefan Metzmacher 2019-08-01 14:01:17 UTC
Fixed as 31cba34a8f0e1301423468c6570530b0e298eb20 for 4.9.0