Discussion:
Problem in working with domain DFS links
(too old to reply)
Jeremy Allison
2008-05-14 01:09:49 UTC
Permalink
I'm working at Connectathon to fix an issue
with the change we made to remove the hostname
checks in the DFS code.

The problem is RHEL5.0 shipped a CIFS client
that sets the DFS bit on pathnames but doesn't
send DFS paths. This causes lookups to fail as
the smbd/msdfs.c code now just eats the first
two parts of the pathname and uses the rest as
the local path. The previous hostname check
used to protect us from that as we knew that
when the hostname was invalid it was a local
path (and a broken client).

I don't want to put that check back in, but
came up with another idea - even though the
hostname can be a different one, the sharename
must be valid on this machine - right ? So
we can check for a valid sharename instead.

Here is a patch for 3.2-stable that implements
this - please check if it's ok in your environment.

Thanks,

Jeremy.
-------------- next part --------------
diff --git a/source/smbd/conn.c b/source/smbd/conn.c
index 5aedadc..1a55522 100644
--- a/source/smbd/conn.c
+++ b/source/smbd/conn.c
@@ -63,10 +63,10 @@ bool conn_snum_used(int snum)
return(False);
}

-
/****************************************************************************
-find a conn given a cnum
+ Find a conn given a cnum.
****************************************************************************/
+
connection_struct *conn_find(unsigned cnum)
{
int count=0;
@@ -84,6 +84,27 @@ connection_struct *conn_find(unsigned cnum)
return NULL;
}

+/****************************************************************************
+ Find a conn given a service name.
+****************************************************************************/
+
+connection_struct *conn_find_byname(const char *service)
+{
+ int count=0;
+ connection_struct *conn;
+
+ for (conn=Connections;conn;conn=conn->next,count++) {
+ if (strequal(lp_servicename(SNUM(conn)),service)) {
+ if (count > 10) {
+ DLIST_PROMOTE(Connections, conn);
+ }
+ return conn;
+ }
+ }
+
+ return NULL;
+}
+

/****************************************************************************
find first available connection slot, starting from a random position.
diff --git a/source/smbd/msdfs.c b/source/smbd/msdfs.c
index 4f9e739..7400f79 100644
--- a/source/smbd/msdfs.c
+++ b/source/smbd/msdfs.c
@@ -133,6 +133,16 @@ static NTSTATUS parse_dfs_path(const char *pathname,
if(p == NULL) {
pdp->servicename = temp;
pdp->reqpath = eos_ptr; /* "" */
+ /* Is this really our servicename ? */
+ if (NULL == conn_find_byname(pdp->servicename)) {
+ DEBUG(10,("parse_dfs_path: %s is not our servicename\n",
+ pdp->servicename));
+ p = temp;
+ DEBUG(10,("parse_dfs_path: trying to convert %s "
+ "to a local path\n",
+ temp));
+ goto local_path;
+ }
return NT_STATUS_OK;
}
*p = '\0';
Volker Lendecke
2008-05-14 02:04:22 UTC
Permalink
Post by Jeremy Allison
Here is a patch for 3.2-stable that implements
this - please check if it's ok in your environment.
This conn_find_byname would happen on every path-based
operation? Can't we make RH to fix the client? They don't
need to bump the version number, just stick in the patch.

Volker
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20080513/9cd821fc/attachment.bin
Jeff Layton
2008-05-14 05:36:21 UTC
Permalink
On Tue, 13 May 2008 23:04:58 +0200
Post by Volker Lendecke
Post by Jeremy Allison
Here is a patch for 3.2-stable that implements
this - please check if it's ok in your environment.
This conn_find_byname would happen on every path-based
operation? Can't we make RH to fix the client? They don't
need to bump the version number, just stick in the patch.
I plan to get that fix in as soon as I can, but it will be a little
while before it can go in. The Linux CIFS client has apparently been
doing this for a long time. This will probably cause most of the
existing Linux CIFS clients in the field to break, not just the newly
shipped ones from Red Hat. It's also not unlikely that there are other
clients that do the same thing that we're not aware of...
--
Jeff Layton <***@redhat.com>
Jeremy Allison
2008-05-14 05:41:04 UTC
Permalink
Post by Jeff Layton
On Tue, 13 May 2008 23:04:58 +0200
Post by Volker Lendecke
Post by Jeremy Allison
Here is a patch for 3.2-stable that implements
this - please check if it's ok in your environment.
This conn_find_byname would happen on every path-based
operation? Can't we make RH to fix the client? They don't
need to bump the version number, just stick in the patch.
I plan to get that fix in as soon as I can, but it will be a little
while before it can go in. The Linux CIFS client has apparently been
doing this for a long time. This will probably cause most of the
existing Linux CIFS clients in the field to break, not just the newly
shipped ones from Red Hat. It's also not unlikely that there are other
clients that do the same thing that we're not aware of...
Yeah, I know we've shipped an smbclient that did this in
the past too :-(.

Jeremy.
Stefan (metze) Metzmacher
2008-05-14 11:45:19 UTC
Permalink
Post by Jeremy Allison
+ /* Is this really our servicename ? */
+ if (NULL == conn_find_byname(pdp->servicename)) {
+ DEBUG(10,("parse_dfs_path: %s is not our servicename\n",
+ pdp->servicename));
+ p = temp;
+ DEBUG(10,("parse_dfs_path: trying to convert %s "
+ "to a local path\n",
+ temp));
+ goto local_path;
Hi Jeremy,

Why do we need to check for a open connection to the share?
Why don't we just check that the share exists in the config?

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : http://lists.samba.org/archive/samba-technical/attachments/20080514/ed2e1ac5/signature.bin
Jeremy Allison
2008-05-14 20:53:21 UTC
Permalink
Post by Stefan (metze) Metzmacher
Hi Jeremy,
Why do we need to check for a open connection to the share?
Why don't we just check that the share exists in the config?
Because the incoming path should be on a valid tid.

Jeremy.
Ofir Azoulay
2008-05-18 17:52:11 UTC
Permalink
Hi,

So this answers one of my questions :-)

And if so, why isn't it the tid on which the specific request is coming?

Thanks,
Ofir.

-----Original Message-----
From: Jeremy Allison [mailto:***@samba.org]
Sent: Wednesday, May 14, 2008 6:46 PM
To: Stefan (metze) Metzmacher
Cc: Jeremy Allison; Ofir Azoulay; samba-***@samba.org
Subject: Re: Problem in working with domain DFS links

On Wed, May 14, 2008 at 08:44:02AM +0200, Stefan (metze) Metzmacher
Post by Stefan (metze) Metzmacher
Hi Jeremy,
Why do we need to check for a open connection to the share?
Why don't we just check that the share exists in the config?
Because the incoming path should be on a valid tid.

Jeremy.
Jeremy Allison
2008-05-18 21:19:15 UTC
Permalink
Post by Ofir Azoulay
Hi,
So this answers one of my questions :-)
And if so, why isn't it the tid on which the specific request is coming?
It probably is, that's an extra check I could add. But remember
I have to map the dfs sharename to a tid first.

Jeremy.
Ofir Azoulay
2008-05-19 11:44:52 UTC
Permalink
Hi,
From my short (and maybe wrong :-)) experience, in the case of the
GET_DFS_REFERRAL request, the request is for a specific path in the
current tid. The parameter for this request is in the current tid, and
so we can compare it to this tid's name and not to any connected tid.

Only the result of this request may point to another server/share.

Am I wrong here?

Thanks,
Ofir.

-----Original Message-----
From: Jeremy Allison [mailto:***@samba.org]
Sent: Sunday, May 18, 2008 7:12 PM
To: Ofir Azoulay
Cc: Jeremy Allison; Stefan (metze) Metzmacher; samba-***@samba.org
Subject: Re: Problem in working with domain DFS links
Hi,
So this answers one of my questions :-)
And if so, why isn't it the tid on which the specific request is coming?
It probably is, that's an extra check I could add. But remember
I have to map the dfs sharename to a tid first.

Jeremy.
Jeremy Allison
2008-05-19 22:11:37 UTC
Permalink
Post by Ofir Azoulay
Hi,
From my short (and maybe wrong :-)) experience, in the case of the
GET_DFS_REFERRAL request, the request is for a specific path in the
current tid. The parameter for this request is in the current tid, and
so we can compare it to this tid's name and not to any connected tid.
Only the result of this request may point to another server/share.
Am I wrong here?
No, you're right I think - but we have no access to the req->tid
here (inside the parsing function), and have to do a strequal
call anyway (which is the expensive part of the code path). The
code I added will move the matching tid to the front of the
linked list which will make subsequent hits the first strequal
tried on the same tid path.

I'll look into refactoring to get access to the incoming tid
and see how much code change that will involve.

Jeremy

Ofir Azoulay
2008-05-14 12:06:08 UTC
Permalink
Hi Jeremy,

Seems logical :-)

However I will apply the patch into 3.0.xxx since we do not use 3.2 yet
and notify you regarding the change.

Thanks, Ofir.

-----Original Message-----
From: Jeremy Allison [mailto:***@samba.org]
Sent: Tuesday, May 13, 2008 11:10 PM
To: Ofir Azoulay
Cc: samba-***@samba.org
Subject: Problem in working with domain DFS links

I'm working at Connectathon to fix an issue with the change we made to
remove the hostname checks in the DFS code.

The problem is RHEL5.0 shipped a CIFS client that sets the DFS bit on
pathnames but doesn't send DFS paths. This causes lookups to fail as the
smbd/msdfs.c code now just eats the first two parts of the pathname and
uses the rest as the local path. The previous hostname check used to
protect us from that as we knew that when the hostname was invalid it
was a local path (and a broken client).

I don't want to put that check back in, but came up with another idea -
even though the hostname can be a different one, the sharename must be
valid on this machine - right ? So we can check for a valid sharename
instead.

Here is a patch for 3.2-stable that implements this - please check if
it's ok in your environment.

Thanks,

Jeremy.
Continue reading on narkive:
Loading...