Bug 10888 - smbclient doesn't ignore "not_defined_in_RFC4178@please_ignore"
smbclient doesn't ignore "not_defined_in_RFC4178@please_ignore"
Status: ASSIGNED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient
4.1.12
All Linux
: P5 normal
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-10-21 12:15 UTC by martin.wilck
Modified: 2015-04-09 19:02 UTC (History)
4 users (show)

See Also:


Attachments
patch for this problem (474 bytes, patch)
2014-10-21 12:15 UTC, martin.wilck
gd: review+
Details
Patch for master, 4.1.x, 4.2.x. (2.88 KB, patch)
2014-10-21 17:10 UTC, Jeremy Allison
no flags Details
git-am fix for master. (3.29 KB, patch)
2015-03-19 20:12 UTC, Jeremy Allison
no flags Details
git-am cherry-pick from master for 4.2.next, 4.1.next. (3.66 KB, patch)
2015-03-26 19:32 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description martin.wilck 2014-10-21 12:15:55 UTC
Created attachment 10361 [details]
patch for this problem

If  "client use spnego principal = yes" is set in smb.conf, smbclient may use the SPNEGO provided principal rather than the user-provided server name. Later Windows versions pass the bogus principal "not_defined_in_RFC4178@please_ignore" here, which should consequently be ignored.

cli_session_setup_get_principal() contains code to check this condition, but this code is obviously broken (https://git.samba.org/?p=samba.git;a=blob;f=source3/libsmb/cliconnect.c;h=789a85d5e9df7bed2f8af6b372f2560c599167a9;hb=HEAD#l1649). The code needs to compare the bogus string to "spnego_principal" rather than "principal", which is always NULL at that point of the code.

The attached patch solves this problem.
Comment 1 martin.wilck 2014-10-21 12:18:01 UTC
bug #7649 and bug #4382 may have the same root cause.
Comment 2 Guenther Deschner 2014-10-21 12:27:58 UTC
Comment on attachment 10361 [details]
patch for this problem

Patch looks good, thanks!
Comment 3 Simo Sorce 2014-10-21 14:34:30 UTC
Should we really fix a security issue to make it more seamless ?

Let me explain what is the problem here. The reason why Microsoft change that field to an invalid name is to discourage people from using thisd feature as using a server provided name is a security issue as you are trusting the server name before you have established a secure channel and done mutual authentication (one of the security features of using Kerberos authentication).

I would rather propose we deprecate this option and remove it in the long term.

My concern with "fixing" this behaviour is that wee incetivate, instead of disccourage the use of this option by making it "work" when it should fail.
Comment 4 Jeremy Allison 2014-10-21 16:26:50 UTC
(In reply to Simo Sorce from comment #3)

Let's do both. We can deprecate this parameter, but whilst it's there it really should work. We can mark it as deprecated in 4.2.0, and plan for removal in 4.3.

Guenther, can you prepare an additional patch to mark this as FLAG_DEPRECATED in master ? In the meantime I'll push the fix we have.
Comment 5 Jeremy Allison 2014-10-21 17:10:51 UTC
Created attachment 10363 [details]
Patch for master, 4.1.x, 4.2.x.

What do you think of the following ?
Comment 6 Simo Sorce 2014-10-21 19:00:56 UTC
(In reply to Jeremy Allison from comment #4)
> We can deprecate this parameter, but whilst it's there it really should work

Well maybe it wasn't clear, but I submit that it is working by throwing an error.
It compells the admin to realize this setting should not be used and turn it off.

By "fixing" it you just let admins keep using it, and keep being vulnerable to MITM attacks.
Comment 7 Jeremy Allison 2014-10-21 19:12:41 UTC
No I understand that, but I don't think we can just deliberately remove the option without a deprecation period.

Note I added "DO NOT USE THIS OPTION" in the strongest possible terms to the docs :-).

Plus we could remove after a few months. But I think we have to give notice.
Comment 8 martin.wilck 2014-10-23 14:03:27 UTC
I understand the arguments for deprecating this option. But some people are forced to use it. In our environment, most systems have totally human-unfriendly DNS host names (= principals) such as "g33defcdubh05.p38.company.local". It's highly desirable to be able to use the more brain-friendly CNAMES to access their shares. 

So far, I know two tricks to achieve this under Linux: 
 1. "client use spnego principal" (currently supported by smbclient) 
 2. reverse DNS lookups (currently supported by cifs.upcall). 
Both are deprecated in the SAMBA communitiy for security reasons. But in order to work comfortably in our environment under Linux, I need to activate them both.

If there is a non-deprecated way to make this happen, please educate me about it, and I'll use it happily. There must be, because Windows users never use the cryptic principal names directly.
Comment 9 Andreas Schneider 2015-01-16 09:36:31 UTC
Ping!!!
Comment 10 Jeremy Allison 2015-01-16 23:08:32 UTC
(In reply to Andreas Schneider from comment #9)

Andreas - it isn't me you have to convince here, it's Simo. Simo - are you OK wit h allowing this with a deprecation notice ?
Comment 11 Stefan Metzmacher 2015-03-13 14:22:51 UTC
Jeremy, can you split the patch into 2 parts.

We should at least fix the bug soon.

I don't mind adding the deprecation, but that should be a separate decision.
Comment 12 Jeremy Allison 2015-03-19 20:12:36 UTC
Created attachment 10893 [details]
git-am fix for master.

Split into 2 parts. Sorry for the delay !
Comment 13 martin.wilck 2015-03-23 13:02:36 UTC
Thanks a lot for the patch. However I'd be grateful if one of the insiders could comment on the question what to do when "client use spnego principal" is eventually removed (see comment #8). 

I still can't see any user-friendly and secure way to resolve host names into principal names under Linux+Samba. Yet this is a non-issue on Windows systems, so it must be a solvable problem.
Comment 14 Stefan Metzmacher 2015-03-24 01:40:15 UTC
Comment on attachment 10893 [details]
git-am fix for master.

Jeremy, please push the 2nd patch with my review to master.
Comment 15 Jeremy Allison 2015-03-25 21:18:07 UTC
Pushed !
Comment 16 Jeremy Allison 2015-03-26 19:32:33 UTC
Created attachment 10915 [details]
git-am cherry-pick from master for 4.2.next, 4.1.next.
Comment 17 Jeremy Allison 2015-03-26 19:33:14 UTC
Karolin please push to 4.2.next, 4.1.next. Patch cherry-picked from master and is the same patch approved by metze and myself.

Thanks !
Comment 18 Karolin Seeger 2015-03-27 20:23:43 UTC
Pushed to autobuild-v4-[1|2]-test.
Comment 19 martin.wilck 2015-03-31 15:53:46 UTC
... another ping for comment#8, comment #13 ...
Comment 20 Karolin Seeger 2015-04-09 19:02:21 UTC
(In reply to Karolin Seeger from comment #18)
Pushed to both branches.
Comment 21 Karolin Seeger 2015-04-09 19:02:50 UTC
Re-assigning to Jeremy to answer the questions.