Problem in working with domain DFS links

[Jeremy Allison]

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.

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]

On Tue, May 13, 2008 at 01:09:39PM -0700, Jeremy Allison wrote:
> 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

[Jeff Layton-2]

On Tue, 13 May 2008 23:04:58 +0200
Volker Lendecke <Volker.Lendecke@...> wrote:

> On Tue, May 13, 2008 at 01:09:39PM -0700, Jeremy Allison wrote:
> > 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 <jlayton@...>

[Jeremy Allison]

On Tue, May 13, 2008 at 05:33:30PM -0700, Jeff Layton wrote:

> On Tue, 13 May 2008 23:04:58 +0200
> Volker Lendecke <Volker.Lendecke@...> wrote:
>
> > On Tue, May 13, 2008 at 01:09:39PM -0700, Jeremy Allison wrote:
> > > 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]

> + /* 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

[Ofir Azoulay]

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:jra@...]
Sent: Tuesday, May 13, 2008 11:10 PM
To: Ofir Azoulay
Cc: samba-technical@...
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.

[Jeremy Allison]

On Wed, May 14, 2008 at 08:44:02AM +0200, Stefan (metze) Metzmacher wrote:
>
> 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]

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:jra@...]
Sent: Wednesday, May 14, 2008 6:46 PM
To: Stefan (metze) Metzmacher
Cc: Jeremy Allison; Ofir Azoulay; samba-technical@...
Subject: Re: Problem in working with domain DFS links

On Wed, May 14, 2008 at 08:44:02AM +0200, Stefan (metze) Metzmacher
wrote:
>
> 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]

On Sun, May 18, 2008 at 03:51:31PM +0300, Ofir Azoulay wrote:
> 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]

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:jra@...]
Sent: Sunday, May 18, 2008 7:12 PM
To: Ofir Azoulay
Cc: Jeremy Allison; Stefan (metze) Metzmacher; samba-technical@...
Subject: Re: Problem in working with domain DFS links

On Sun, May 18, 2008 at 03:51:31PM +0300, Ofir Azoulay wrote:
> 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]

On Mon, May 19, 2008 at 09:44:24AM +0300, Ofir Azoulay wrote:

> 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