Bug 7781 - Samba transforms ShareName to lowercase (sharename) when adding new share via MMC
Samba transforms ShareName to lowercase (sharename) when adding new share via...
Status: RESOLVED FIXED
Product: Samba 3.5
Classification: Unclassified
Component: DCE-RPCs and pipes
3.5.4
x86 Linux
: P3 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
: 7782 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-08 06:10 UTC by Volodymyr Khomenko
Modified: 2012-11-05 10:11 UTC (History)
3 users (show)

See Also:


Attachments
patch for 3.5.x. (16.28 KB, patch)
2010-11-08 21:57 UTC, Jeremy Allison
no flags Details
Jeremy's Patch turned to a "git format-patch" patch. (17.28 KB, patch)
2012-09-24 04:51 UTC, Michael Adam
no flags Details
This additional patch fixes make test. (825 bytes, patch)
2012-11-01 18:57 UTC, Jeremy Allison
obnox: review+
jra: review? (metze)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Volodymyr Khomenko 2010-11-08 06:10:44 UTC
Due a hidden side-effect of find_service() function when adding a new share via MMC (Microsoft Console: rclick on My Computer-> Manage -> Action -> Connect to another computer-> Enter samba server name -> System tools -> rclick on Shares -> New share...) entered share name is distorted (converted to lowercase).
In such view mixed-case share names are impossible (they are always lowercased).

For example, when adding new share 'MyShareWithCapitalLetters' via MMC, the following command-line is running:

[2010/11/08 13:05:12.022746, 10, pid=13966] rpc_server/srv_srvsvc_nt.c:2019(_srvsvc_NetShareAdd)
  _srvsvc_NetShareAdd: Running [./tools/add_share "smb.conf" "mysharewithcapitalletters" "/my_share" "Test share with capitals in the name" "0"]

Attached patch can fix this problem:

--- source3/rpc_server/srv_srvsvc_nt.c
+++ source3/rpc_server/srv_srvsvc_nt.c
@@ -1737,7 +1737,7 @@ WERROR _srvsvc_NetShareAdd(pipes_struct 
 			   struct srvsvc_NetShareAdd *r)
 {
 	char *command = NULL;
-	char *share_name = NULL;
+	char *share_name = NULL, *share_name_copy = NULL;
 	char *comment = NULL;
 	char *pathname = NULL;
 	int type;
@@ -1827,7 +1827,17 @@ WERROR _srvsvc_NetShareAdd(pipes_struct 
 		return WERR_ACCESS_DENIED;
 	}
 
-	snum = find_service(share_name);
+	/* Dell 2010 / Samba upgrade project / MMC-related small fixes:
+		Before call to find_service make a copy of share_name
+		because find_service() modifies its parameter -
+		it converts share_name to to canonical form (all lowercase letters).
+		NB: find_service() converts share_name as a size-effect only if the service was not found;
+		so this fix is not applicable for _srvsvc_NetShareSetInfo because no translation there. */
+
+	if ((share_name_copy = talloc_strdup(ctx, share_name)) == NULL) {
+		return WERR_NOMEM;
+	}
+	snum = find_service(share_name_copy);
 
 	/* Share already exists. */
 	if (snum >= 0) {
Comment 1 Stefan Metzmacher 2010-11-08 06:25:14 UTC
That patch seems to be wrong.

find_service() takes a 'fstring' ( a 256byte char array as argument)
not 'char *'.

The caller should copy into a fstring on the stack and pass that one.

There're are some other places in the code where we have that bug...

Jeremy you instruced passing a dynamic string to find_service in
commit acc0218f1396de2aad521dff722ada43631d92f9.

metze
Comment 2 Volodymyr Khomenko 2010-11-08 06:29:19 UTC
*** Bug 7782 has been marked as a duplicate of this bug. ***
Comment 3 Volodymyr Khomenko 2010-11-08 06:42:05 UTC
Yep, strictly speaking you are right, but original code use the same approach (share_name is allocated by talloc_strdup). Also find_service itself uses dynamically-allocated string via SMB_STRDUP when trying to call to itself recursively (with default service).

Does it really matter - fstring or char* ? It does if find_service() goes beyond the EOL (zero-termination char)?
Comment 4 Stefan Metzmacher 2010-11-08 06:56:14 UTC
I haven't looked at this details, but I hope Jeremy will:-)
Comment 5 Jeremy Allison 2010-11-08 18:50:48 UTC
Yes, there's a bug in that map_username uses fstrcpy to replace the username passed into it from find_service(). This can overwrite memory. It's not a security issue as map username is a root-only feature, but it could cause a crash. I'll fix this.

Jeremy.
Comment 6 Jeremy Allison 2010-11-08 21:57:31 UTC
Created attachment 6054 [details]
patch for 3.5.x.

Can you test this patch please ? It should fix the problem you were trying to fix (losing the supplied case when adding a share) plus it should fix the underlying issue with find_service() potentially overwriting memory if map_username  modifies the underlying name.

Jeremy.
Comment 7 Volodymyr Khomenko 2010-11-09 02:56:37 UTC
Yep, I've applied this patch and see that it's Ok (now _srvsvc_NetShareAdd uses original letters case - checked via logs).

Thanks.
Comment 8 Jeremy Allison 2010-11-09 19:28:01 UTC
Comment on attachment 6054 [details]
patch for 3.5.x.

Volker or Michael, please review and assign to Karolin if you're happy. I think this is the right fix for 3.5.7.
Jeremy.
Comment 9 Michael Adam 2011-06-03 11:24:50 UTC
(In reply to comment #8)
> Comment on attachment 6054 [details]
> patch for 3.5.x.
> 
> Volker or Michael, please review and assign to Karolin if you're happy. I think
> this is the right fix for 3.5.7.
> Jeremy.

Jeremy,

this had slipped my attention - sorry!
Should we reactivate this for 3.5.9 ?

Is this still valid or fixed differently?
The corresponding fix in master seems to be different.

Cheers - Michael
Comment 10 Jeremy Allison 2011-06-03 23:43:54 UTC
Yes this patch is still good for 3.5.x. It's fixed via allocation in master/v3-6-test. Please +1 it and re-assign to Karolin to get it into 3.5.9.

Jeremy.
Comment 11 Michael Adam 2012-05-23 20:27:56 UTC
Gosh I neglected "My Requests" list.. :-(

I don't quite understand why this defect is set to fixed...
Comment 12 Michael Adam 2012-09-24 04:51:14 UTC
Created attachment 7928 [details]
Jeremy's Patch turned to a "git format-patch" patch.

I converted Jeremy's patch to git patch.
Comment 13 Michael Adam 2012-09-24 04:55:07 UTC
Sorry, I must have gotten distracted again...

But now I now what confused me:

After testing Jeremy's patch, Volodymyr seems to have set this bug to fixed.
But bugs should only be set to fixed when the patch has made it into the
release branch...

Assigning to Karolin for inclusion into 3.5.$NEXT.

Karo: The patch is missing the "review" flag, but my
git format-patch version is effectively the "+" of Jeremy's original patch.

Cheers - Michael
Comment 14 Karolin Seeger 2012-09-28 07:25:55 UTC
Pushed to v3-5-test.
Closing out bug report.

Thanks!
Comment 15 Karolin Seeger 2012-10-31 10:34:50 UTC
This patch seems to break 'make test'.
Jeremy, can you have a look again?

Thanks!
Comment 16 Karolin Seeger 2012-10-31 10:57:53 UTC
(In reply to comment #15)
> This patch seems to break 'make test'.
> Jeremy, can you have a look again?
> 
> Thanks!

Reverted patch for now.
Please note that the next 3.5 release is scheduled for Monday, November 5.
Comment 17 Jeremy Allison 2012-10-31 23:34:49 UTC
I have figured out the problem that breaks "make test".

With this patch we do:

fstrcpy(service, service);

but inside safe_strcpy() when in DEVELOPER mode we do:

#ifdef DEVELOPER
        clobber_region(fn,line,dest, maxlength+1);
#endif

so we overwrite the src (as it's the same as dest here).

Still thinking about the correct fix for this - should we allow safe_strcpy() to be safely called when src == dest ?

Jeremy.
Comment 18 Jeremy Allison 2012-11-01 18:57:09 UTC
Created attachment 8128 [details]
This additional patch fixes make test.

I think this is probably something we need in master/3.6.x/4.0.x also.

Jeremy.
Comment 19 Karolin Seeger 2012-11-02 07:47:40 UTC
(In reply to comment #18)
> Created attachment 8128 [details]
> This additional patch fixes make test.
> 
> I think this is probably something we need in master/3.6.x/4.0.x also.
> 
> Jeremy.

Jeremy, can I go ahead with 3.5.19 without these patches and include them in the next bugfix release?
Comment 20 Michael Adam 2012-11-02 08:19:26 UTC
(In reply to comment #18)
> Created attachment 8128 [details]
> This additional patch fixes make test.

This looks like a safe and reasonable shortcut anyways.

I am just running make test on v3-5-test with the reinstated first patch and this patch currently to verify that there are no strange side effects (I can't think of any).
 
> I think this is probably something we need in master/3.6.x/4.0.x also.

In master this function is not present any more, nor is clobber_region.
Comment 21 Jeremy Allison 2012-11-02 17:00:47 UTC
Sure, they can wait until the release afterwards.

Re-assigning to Karolin for inclusion in 3.5.20.

Jeremy.
Comment 22 Michael Adam 2012-11-02 20:43:45 UTC
Comment on attachment 8128 [details]
This additional patch fixes make test.

verified the second patch per make test
Comment 23 Karolin Seeger 2012-11-05 10:11:19 UTC
Pushed both patches to v3-5-test.
Closing out bug report.

Thanks!