We had a customer report that attempting to make CIFS mount with a null
username (i.e. doing an anonymous mount) doesn't work. Looking through the
code, it looks like CIFS expects a NULL username from userspace in order
to trigger an anonymous mount. The mount.cifs code doesn't seem to ever
pass a null username to the kernel, however.
It looks also like the kernel can take a sec=none option, but it only seems
to look at it if the username is already NULL. This seems redundant and
effectively makes sec=none useless.
The following patch makes sec=none force an anonymous mount. I've briefly
tested it and it seems to work. I suppose we could alternately do some
stuff in userspace to make mount.cifs force a null username instead, but this
seems more straightforward to me.
Signed-off-by: Jeff Layton <[email protected]>
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index cf40e24..330e290 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1721,12 +1721,13 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
return -EINVAL;
}
- if (volume_info.username) {
+ if (volume_info.nullauth) {
+ cFYI(1,("null user"));
+ volume_info.username = NULL;
+ } else if (volume_info.username) {
/* BB fixme parse for domain name here */
cFYI(1, ("Username: %s ", volume_info.username));
- } else if (volume_info.nullauth) {
- cFYI(1,("null user"));
} else {
cifserror("No username specified");
/* In userspace mount helper we can get user name from alternate
On Thu, May 03, 2007 at 02:32:33PM -0400, Jeff Layton wrote:
> We had a customer report that attempting to make CIFS mount with a null
> username (i.e. doing an anonymous mount) doesn't work. Looking through the
> code, it looks like CIFS expects a NULL username from userspace in order
> to trigger an anonymous mount. The mount.cifs code doesn't seem to ever
> pass a null username to the kernel, however.
>
> It looks also like the kernel can take a sec=none option, but it only seems
> to look at it if the username is already NULL. This seems redundant and
> effectively makes sec=none useless.
>
> The following patch makes sec=none force an anonymous mount. I've briefly
> tested it and it seems to work. I suppose we could alternately do some
> stuff in userspace to make mount.cifs force a null username instead, but this
> seems more straightforward to me.
Looks useful. In case you have some spare time at your hand it would
be really nice to convert cifs option parsing to the lib/parser.c code
and move all validation of the arguments into one place, so it's easily
understanable and better to maintain.
Jeff Layton wrote:
> We had a customer report that attempting to make CIFS mount with a null
> username (i.e. doing an anonymous mount) doesn't work. Looking through the
> code, it looks like CIFS expects a NULL username from userspace in order
> to trigger an anonymous mount. The mount.cifs code doesn't seem to ever
>pass a null username to the kernel, however.
Yes - cifs kernel code expects a NULL username (e.g. "username=") if
you really don't want to pass the default username of the uid of
the current process and you don't set the username explicitly
(e.g. in a credential file or mount parameter).
Samba userspace tools (and smbfs) handled this by first trying to
setup the SMB session using the default user, and if that fails
with access denied then retrying sessionsetup with a null username
string. This would be easy to change in mount.cifs (ie as long
as username was not explicitly passed on mount then if mount fails
with access denied simply add a retry with "username="). This was
discussed at SambaXP.
Christoph Hellwig wrote:
> Looks useful. In case you have some spare time at your hand it would
> be really nice to convert cifs option parsing to the lib/parser.c code
> and move all validation of the arguments into one place, so it's easily
> understanable and better to maintain.
Yes - that would be excellent. The parse_mount_options badly needs to
be rewritten now that the number of mount options needed has grown.
This is something Alex Bokovoy and I discussed last week at SambaXP
for both the kernel code and the user space mount.cifs code.
Alex had volunteered to rewrite the user space cifs mount option
parsing code (and also change to use the safer talloc library)
On Fri, May 04, 2007 at 10:26:29AM -0500, Steve French (smfltc) wrote:
> Jeff Layton wrote:
> >We had a customer report that attempting to make CIFS mount with a null
> >username (i.e. doing an anonymous mount) doesn't work. Looking through the
> >code, it looks like CIFS expects a NULL username from userspace in order
> >to trigger an anonymous mount. The mount.cifs code doesn't seem to ever
> >pass a null username to the kernel, however.
> Yes - cifs kernel code expects a NULL username (e.g. "username=") if
> you really don't want to pass the default username of the uid of
> the current process and you don't set the username explicitly
> (e.g. in a credential file or mount parameter).
>
> Samba userspace tools (and smbfs) handled this by first trying to
> setup the SMB session using the default user, and if that fails
> with access denied then retrying sessionsetup with a null username
> string. This would be easy to change in mount.cifs (ie as long
> as username was not explicitly passed on mount then if mount fails
> with access denied simply add a retry with "username="). This was
> discussed at SambaXP.
>
Does this mean you're NAK'ing this patch in favor of a userspace fix? My
perspective is that if someone explicitly requests sec=none, then we ought
to do an anonymous mount regardless of how the username is set. Would
you agree that that behavior is what you would want?
>
> Christoph Hellwig wrote:
> >Looks useful. In case you have some spare time at your hand it would
> >be really nice to convert cifs option parsing to the lib/parser.c code
> >and move all validation of the arguments into one place, so it's easily
> >understanable and better to maintain.
>
> Yes - that would be excellent. The parse_mount_options badly needs to
> be rewritten now that the number of mount options needed has grown.
> This is something Alex Bokovoy and I discussed last week at SambaXP
> for both the kernel code and the user space mount.cifs code.
> Alex had volunteered to rewrite the user space cifs mount option
> parsing code (and also change to use the safer talloc library)
>
Definitely, though that sounds like a big project and I don't have the time to
tackle it at the moment :-)
Thanks,
Jeff
Jeff Layton wrote:
>On Fri, May 04, 2007 at 10:26:29AM -0500, Steve French (smfltc) wrote:
>
>
>>Jeff Layton wrote:
>>
>>
>>>We had a customer report that attempting to make CIFS mount with a null
>>>username (i.e. doing an anonymous mount) doesn't work. Looking through the
>>>code, it looks like CIFS expects a NULL username from userspace in order
>>>to trigger an anonymous mount. The mount.cifs code doesn't seem to ever
>>>pass a null username to the kernel, however.
>>>
>>>
>>Yes - cifs kernel code expects a NULL username (e.g. "username=") if
>>you really don't want to pass the default username of the uid of
>>the current process and you don't set the username explicitly
>>(e.g. in a credential file or mount parameter).
>>
>>Samba userspace tools (and smbfs) handled this by first trying to
>>setup the SMB session using the default user, and if that fails
>>with access denied then retrying sessionsetup with a null username
>>string. This would be easy to change in mount.cifs (ie as long
>>as username was not explicitly passed on mount then if mount fails
>>with access denied simply add a retry with "username="). This was
>>discussed at SambaXP.
>>
>>
>>
>
>Does this mean you're NAK'ing this patch in favor of a userspace fix? My
>perspective is that if someone explicitly requests sec=none, then we ought
>to do an anonymous mount regardless of how the username is set. Would
>you agree that that behavior is what you would want?
>
>
>
>
Your patch is probably ok to add, although I would like to see if any of
the other Samba team
had thoughts on this, as "null user" sessions are a fairly obscure part
of the protocol. But
even with the kernel change, mount.cifs also should change for a loosely
related case that of
1) sec=none is not specified by the user
2) but username also is not specified explicitly
For that case we need to retry on access denied as if it were a request
for a "null user" mount
ie send sec=none (or equivalently username=) the 2nd time. This gets
more complicated
since mount.cifs also has to retry on a couple of other cases (e.g. when
the server does
not support port 445 but does not take the standard server string
"*SMBSERVER"
on the RFC1001 called name).
If there are no objections from any of the other Samba guys I will take
your patch which has
the effect of treating "sec=none" as meaning "ingore any userid if
specified, and set the username to null
on the session setup"). That is consistent with what we documented.
I had though for a while that a user who mounts passing both "sec=none"
and a username might
also expect to get a null password (they could have done this with
"guest" or with "password=") or
might want to try to send the password in plaintext - but I doubt that
we want to support
a user who wants to send the password plaintext without the server
requiring it (and in that
case cifs can be built and configured to allow plaintext if absolutely
necessary to support
those ancient servers).
Basically if we set username to null in kernel when (sec=none)
>Thanks,
>Jeff
>
>
>