Re: [PATCH] CIFS: make sec=none force an anonymous mount

Re: [PATCH] CIFS: make sec=none force an anonymous mount

[Steve French (smfltc)]

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)

Re: [PATCH] CIFS: make sec=none force an anonymous mount

[Jeff Layton]

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

Re: [PATCH] CIFS: make sec=none force an anonymous mount

[Steve French (smfltc)]

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
>
>  
>

Re: [linux-cifs-client] Re: [PATCH] CIFS: make sec=none force an anonymous mount

[Jeff Layton]

On Sat, May 05, 2007 at 05:41:35AM -0500, Shirish S Pargaonkar wrote:
> 
> When a session setup request is sent as an anonymous user (NUL user),
> should/could there be
> password associated with that?
> Right now, sec=none option, will prompt you for a password.

We should probably turn off password prompting if sec=none is specified.

> And when we add code to retry session setup as anonymous user if the first
> session setup request
> fails, should that retry request be sent with the password or without
> password?
> 
> When smbfs sends requests as an anonymous user, it does not send a password
> along with it.
> 

I'd say we'd want to avoid sending along the password in any situation where
it wasn't really needed.

-- Jeff

Re: [linux-cifs-client] Re: [PATCH] CIFS: make sec=none force an anonymous mount

[Steve French (smfltc)]

Shirish S Pargaonkar wrote:

>
>
> When a session setup request is sent as an anonymous user (NUL user), 
> should/could there be
> password associated with that?
> Right now, sec=none option, will prompt you for a password.
> And when we add code to retry session setup as anonymous user if the 
> first session setup request
> fails, should that retry request be sent with the password or without 
> password?
>
> When smbfs sends requests as an anonymous user, it does not send a 
> password along with it.
>
> Regards,
>
> Shirish
>
We should allow a password to be specified (presumably it is not common 
for a server to have a password associated with a null user),
but probably not prompt (similar to "guest" - except for the case of 
guest, we start with the username of uid of current process, and
only if it fails with access denied do we try "user=" (or equivalently 
sec=none))

[PATCH] CIFS: make sec=none force an anonymous mount

[Jeff Layton]

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 <jlayton@redhat.com>

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

Re: [PATCH] CIFS: make sec=none force an anonymous mount

[Christoph Hellwig]

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.