2007-04-23 23:09:24

by NeilBrown

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Monday April 23, [email protected] wrote:
> >
> >> I (hastily) created two git trees:
> >> git://git.infradead.org/~steved/libtirpc.git
> >> git://git.infradead.org/~steved/rpcbind.git
> >>
> >> I think if you take a look, you'll see that
> >> this code may not be as mature as the portmap
> >> code, but its a much better start... imho..
> >
> > Yes, very hasty. Several #temporary# and back~ files :-)
> Yeah... for some reason those temporary files are in the
> the tar ball... I'll work to get that cleaned up..

Is there someone "maintaining" rpcbind? Should there be?
I notice there is an rpcbind at Wietse Venema's site:
ftp://ftp.porcupine.org/pub/security/index.html

Is this rpcbind derived from that?

Should rpcbind and portmap "live" close together so that people
looking for one will find the other, and so that compatibility
(features, options) can be maximised?

>
> >
> > I notice that it has a concept of who 'owns' a registration, but it
> > only works if unix-domain sockets are used for the registration.
> > Adding 'superuser' ownership for localhost/privport registrations is
> > probably a 3 line patch....
> I'm not sure I understand.. are you talking about how getowner()
> is being used?

Sort of. I was actually looking in pmapproc_change which seems to
have 'getowner' open-coded in it.
Both call __rpc_get_local_uid.
That function (in libritpc) returns -1 ( => "unknown") for a AF_INET
connection.
I believe that for AF_INET, it should check sin_addr and sin_port.
If addr == 127.0.0.1 and sin_port < 1024, then uid should be set to
0.

It would be nice if the libritpc version of bindrecvport could be
configured to avoid some list of ports, whether from /etc/services or
from elsewhere.

NeilBrown

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs


2007-04-26 22:22:58

by Steve Dickson

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

Olaf Kirch wrote:
> On Tuesday 24 April 2007 19:52, Steve Dickson wrote:
>> In theory yes... but unfortunately Transport Independent
>> RPC code is much different than the RPC we have in glibc.
>
> Still, there's a whole load of shared code, and this is where
> I agree with Christoph that it would be foolish to throw away
> the work that went into the glibc code.

> The buffer overflow fix I mentioned is just one such thing -
Fixed in git://git.infradead.org/~steved/libtirpc.git
(commit 30431c6d846eab1bc6b7a3a91a7894f3acf2680f)

But there was code that check for nodesize being zero
so it not completely clear that this buffer overflow
was a real threat...

> I did a diff of the files common in glibc and tirpc, and here are some
> of the things I found.
>
> - glibc authunix_create_default works if the process has
> more than NGRPS gids. tirpc wil fail
Note the following comment in the glibc code:
/* This braindamaged Sun code forces us here to truncate the
list of groups to NGRPS members since the code in
authuxprot.c transforms a fixed array. Grrr. */
So the glibc can indeed allocate memory a ton of gids
but then only sends NGRPS of them... which is a complete
waste of memory and time... imho..

>
> - glibc tries to be more multithread friendly, by using
> things like gethostbyname_r instead of gethostbyname.
> tirpc doesn't.
Well since gethostbyname_r() is a GNU extension it understandable
why its not in the BSD based code... Also in tirpc, gethostbyname()
routine is *not* used in the highly used clnt_create() call as
its is in glibc . In tirpc the routine getaddrinfo() is used to
resolve host names in clnt_create() and I don't think there is a
thread safe version of that...

Also the only two place gethostbyname() is used in tirpc is in
getrpcport() and in the a few calls down from authdes_refresh().

Now there are calls to mutex_lock/unlock so in the end there
multithread support... at least in that part of the code...


> - it seems that tirpc unconditionally requires rpcbind,
> even for applications that still use pmap_set and
> friends. In the interest of a smooth transition, applications
> using the old interface should be able to expect the
> old behavior.
I not understanding your point... all the pmap_xxx routine are
supported, taking the same arguments and return same value.
Internally they definitely do things differently, which should
not be a problem...so where are you seeing the potential
transition problems?

>
> - tirpc does truly bizarre things, eg in clnt_raw.c
> it declares a local variable, and then acquires
> a mutex to check this local variable for NULL:
>
> mutex_lock(&clntraw_lock);
> if (clp == NULL) {
> mutex_unlock(&clntraw_lock);
> return (RPC_FAILED);
> }
> mutex_unlock(&clntraw_lock);
Look again... clp is pointing to global data... so locks
are needed...

>
> - tirpc has a mechanism that lets you register
> server side auth flavors dynamically; libc
> doesn't.
>
> - glibc switched to poll(), whereas tirpc still uses
> select with all its limitations
educate me... why is using poll() better than select()?

>
> - in tirpc, svc.c seems to be thread safe, whereas
> glibc isn't.
>
> - tirpc lets the application control the maximum record
> size, making DoS attacks just a little harder.
>
> - in tirpc, xdr_mem.c has different ops for aligned vs
> unaligned buffers, for what it's worth.
>
> - in tirpc, clnt_sperror uses snprintf to avoid buffer overflows,
> but gets it all wrong wrt the return value of snprintf.
hmm... how is:
i = snprintf(str, len, "; errno = %s", strerror(e.re_errno));
if (i > 0) {
str += i;
len -= i;
}
wrong? looks reasonable to me... what am I missing?

>
> - tirpc is K&R all over the place, while glibc is mostly
> cleaned up (not everywhere though)
I'm not sure why "K&R all over the place" is bad but the
tirprc code is much better documented and much less hacker...
Meaning, in the glibc code, fixes did not follow the original
style... which make the fix stand out... this is not the case
with the tirpc code...

>
> - tirpc bindresvport is IPv6 aware, but has a bug: if the
> caller passes a sockaddr with sin_port/sin6_port set,
> the routine will actually bind to htons(port).
I must be missing something... in the AF_INET6: the port is set
to sin6->sin6_port; which will be the first port that is tried... right?
and that is bad?

> the libc bindresvport implementation also has a small
> optimization in that it will continue searching where the
> previous call to bindresvport left off, rather than starting
> all over from 600.
I ironic you bring this up.. this is the result of a bug I found
and with this experience I learned how difficult it truly is
to get things fixed in glibc.. anyways this is fixed with
in my git tree with:
commit c254b435007ebd4ed471737198975d5ccf4e7949

>
> - the authunix code in tirpc assumes sizeof(gid_t)
> equals sizeof(int)
this is true... are there any modern day machine architecture
where sizeof(gid_t) == sizeof(short)?

>
> - the glibc RPC client side seems to be thread safe, whereas
> tirpc seems to be lacking in some areas (see rpc_thread.c)
I think *seems* is the key word here... since I can not find any
functions that make use of these routines but there is a lot
of glibc black magic in that file so maybe indirectly they are...

>
> I stopped reading the code after an hour, but I think this list could
> be extended quite a bit.
Please catch your breath... and continue on! :-)

Again I want to thank Olaf for taking the time which caused me
to take the time to take close look.... and I'm glad I did...
I actually feel tirpc (least the library code) is much better
shape that that I thought it was...

Now I did not spend the time just to be argumentative with Olaf...
Hell... I hope he keeps poking holes... is the best thing for the
code... but I did spend the time because I've never been that
impressed with the RPC in glibc in the first place... So I could
not image any other implementation to be as bad as I perceived the
glibc implementation is and it turns out I was right... the tirpc
is time tested, does support IPv6 and is well documented... among
other thing... Plus... we have no choice... the glibc people
will not change anything... and with Uli... nothing means nothing!

So in the end I truly do think porting rpc service (like nfs-utils,
nis, etc) is the right thing to do...

steved.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-27 02:23:09

by J. Bruce Fields

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Thu, Apr 26, 2007 at 06:22:13PM -0400, Steve Dickson wrote:
> Olaf Kirch wrote:
> > mutex_lock(&clntraw_lock);
> > if (clp == NULL) {
> > mutex_unlock(&clntraw_lock);
> > return (RPC_FAILED);
> > }
> > mutex_unlock(&clntraw_lock);
> Look again... clp is pointing to global data... so locks
> are needed...

We don't touch that global data here--since we don't deference clp in
that fragment, what it points to shouldn't matter.

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-27 06:22:32

by Olaf Kirch

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Friday 27 April 2007 00:22, Steve Dickson wrote:
> So the glibc can indeed allocate memory a ton of gids
> but then only sends NGRPS of them... which is a complete
> waste of memory and time... imho..

No. If you don't do this, you will not get *any* gids.
getgroups simply fails if the array you give it is too short.
And tirpc deals with that rather ungracefully by calling
abort(), of all things.

> Well since gethostbyname_r() is a GNU extension it understandable
> why its not in the BSD based code... Also in tirpc, gethostbyname()
> routine is *not* used in the highly used clnt_create() call as
> its is in glibc . In tirpc the routine getaddrinfo() is used to
> resolve host names in clnt_create() and I don't think there is a
> thread safe version of that...

getaddrinfo is thread-safe, so that is fine.

> Also the only two place gethostbyname() is used in tirpc is in
> getrpcport() and in the a few calls down from authdes_refresh().
>
> Now there are calls to mutex_lock/unlock so in the end there
> multithread support... at least in that part of the code...

Ah, okay.

> > - it seems that tirpc unconditionally requires rpcbind,
> > even for applications that still use pmap_set and
> > friends. In the interest of a smooth transition, applications
> > using the old interface should be able to expect the
> > old behavior.
> I not understanding your point... all the pmap_xxx routine are
> supported, taking the same arguments and return same value.
> Internally they definitely do things differently, which should
> not be a problem...so where are you seeing the potential
> transition problems?

It means you cannot run a binary linked against tirpc
on a host that runs portmap but no rpcbind. E.g.
pmap_set will simply fail if it can't connect to
rpcbind via AF_LOCAL. I think any replacement for
libc's sunrpc code should continue to work in a
portmap environment.

> > - tirpc does truly bizarre things, eg in clnt_raw.c
> > it declares a local variable, and then acquires
> > a mutex to check this local variable for NULL:
> >
> > mutex_lock(&clntraw_lock);
> > if (clp == NULL) {
> > mutex_unlock(&clntraw_lock);
> > return (RPC_FAILED);
> > }
> > mutex_unlock(&clntraw_lock);
> Look again... clp is pointing to global data... so locks
> are needed...

Yes, clp is *pointing* at global data, so the data it
points at may change. But this pointer itself is
a variable on your local stack which is not even
visible to any other thread; so it can't change.

I assume this was once meant to protect access
to the clntraw_private pointer, which is a global
variable and could do with some mutex protection,
and I guess at some point the code looked like this:

mutex_lock()
clp = clntraw_private;
if (clp == NULL) {
unlock and fail
}

and some misguided soul probably moved the
pointer assignment out of the protected region
so that the code now looks the way it does.
This needs fixing.

> > - glibc switched to poll(), whereas tirpc still uses
> > select with all its limitations
> educate me... why is using poll() better than select()?

fd_set has a limit on the max file descriptor it can
accomodate (1024 I think). poll does not. So if you use
that code in apps that have files with descriptors > 1024
you will start clobbering heap or stack memory.

Under the hood, they're all the same - glibc's select
will call sys_poll.

> > - in tirpc, clnt_sperror uses snprintf to avoid buffer overflows,
> > but gets it all wrong wrt the return value of snprintf.
> hmm... how is:
> i = snprintf(str, len, "; errno = %s", strerror(e.re_errno));
> if (i > 0) {
> str += i;
> len -= i;
> }
> wrong? looks reasonable to me... what am I missing?

snprintf returns the number of characters it *would* have
printed if the buffer size had been sufficient. It does not
return the number of characters actually printed, which is
what this code seems to assume. So if the buffer is
too small, you end up with str pointing beyond the end of
the buffer, and len a huge unsigned value. That is decidedly
not what you want :-)

> > - tirpc is K&R all over the place, while glibc is mostly
> > cleaned up (not everywhere though)
> I'm not sure why "K&R all over the place" is bad but the
> tirprc code is much better documented and much less hacker...
> Meaning, in the glibc code, fixes did not follow the original
> style... which make the fix stand out... this is not the case
> with the tirpc code...

I agree it's a matter of taste. Like wearing bell bottom jeans
and pink shirts :-)

> > - tirpc bindresvport is IPv6 aware, but has a bug: if the
> > caller passes a sockaddr with sin_port/sin6_port set,
> > the routine will actually bind to htons(port).
> I must be missing something... in the AF_INET6: the port is set
> to sin6->sin6_port; which will be the first port that is tried... right?
> and that is bad?

It uses htons alright, but it forgot the ntohs

switch (af) {
case AF_INET:
sin = (struct sockaddr_in *)sa;
salen = sizeof(struct sockaddr_in);
port = sin->sin_port; /* <--- needs ntohs */
...
*portp = htons(port++);

> >
> > - the authunix code in tirpc assumes sizeof(gid_t)
> > equals sizeof(int)
> this is true... are there any modern day machine architecture
> where sizeof(gid_t) == sizeof(short)?

Not as far as I know, but that's beside the point I think. This kind
of assumption is always bad.

Two more things:

- tirpc does a getenv("NETNAME") which should probably be
secure_getenv().

- in libc, clntudp_call uses the IP_RECVERR feature of the
Linux kernel, so it gets notified of ICMP errors (like
port_unreach). The TIRPC code does not, so the
application will never see any error and has to wait
for a timeout instead.

> Now I did not spend the time just to be argumentative with Olaf...

No offense taken. I should have probably explained my points
better than I did

> Hell... I hope he keeps poking holes... is the best thing for the
> code... but I did spend the time because I've never been that
> impressed with the RPC in glibc in the first place... So I could
> not image any other implementation to be as bad as I perceived the
> glibc implementation is and it turns out I was right... the tirpc
> is time tested, does support IPv6 and is well documented... among
> other thing... Plus... we have no choice... the glibc people
> will not change anything... and with Uli... nothing means nothing!
>
> So in the end I truly do think porting rpc service (like nfs-utils,
> nis, etc) is the right thing to do...

That's where we disagree :-) I think ripping out the glibc
code and replacing it will cause a lot of maintenance for
the distros.

Olaf
--
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play
[email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-27 14:02:07

by Peter Staubach

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

Olaf Kirch wrote:
> On Friday 27 April 2007 00:22, Steve Dickson wrote:
>
>> So the glibc can indeed allocate memory a ton of gids
>> but then only sends NGRPS of them... which is a complete
>> waste of memory and time... imho..
>>
>
> No. If you don't do this, you will not get *any* gids.
> getgroups simply fails if the array you give it is too short.
> And tirpc deals with that rather ungracefully by calling
> abort(), of all things.
>
>

It is a distinct possibility that calling abort() is the wrong thing,
but so is sending out a message with only part of the gid list. The
request can fail due to authentication problems, the user looks and
see that he is indeed in the right group(s), and then can't figure
out why things didn't work right.

There isn't a clear cut right answer, but the described behavior is
at least definitive. There were a _lot_ of discussions inside of
Sun, comp.protocols.nfs, and at Connectathons regarding this issue,
without clear resolutions that were better than the current behavior.

>> Also the only two place gethostbyname() is used in tirpc is in
>> getrpcport() and in the a few calls down from authdes_refresh().
>>
>> Now there are calls to mutex_lock/unlock so in the end there
>> multithread support... at least in that part of the code...
>>
>
> Ah, okay.
>
>

The TI-RPC code is definitely thread-safe and many portions are also
MT-hot. Many of the Solaris RPC based daemons are multi-threaded,
like mountd, for example.

This version of the library may or may not be MT-safe, but newer
versions are definitely safe. Perhaps the Bull people could tell
us how old the library is?


>>> - it seems that tirpc unconditionally requires rpcbind,
>>> even for applications that still use pmap_set and
>>> friends. In the interest of a smooth transition, applications
>>> using the old interface should be able to expect the
>>> old behavior.
>>>
>> I not understanding your point... all the pmap_xxx routine are
>> supported, taking the same arguments and return same value.
>> Internally they definitely do things differently, which should
>> not be a problem...so where are you seeing the potential
>> transition problems?
>>
>
> It means you cannot run a binary linked against tirpc
> on a host that runs portmap but no rpcbind. E.g.
> pmap_set will simply fail if it can't connect to
> rpcbind via AF_LOCAL. I think any replacement for
> libc's sunrpc code should continue to work in a
> portmap environment.
>
>

This sounds like a good goal to add to the list of work to do.

>>> - tirpc does truly bizarre things, eg in clnt_raw.c
>>> it declares a local variable, and then acquires
>>> a mutex to check this local variable for NULL:
>>>
>>> mutex_lock(&clntraw_lock);
>>> if (clp == NULL) {
>>> mutex_unlock(&clntraw_lock);
>>> return (RPC_FAILED);
>>> }
>>> mutex_unlock(&clntraw_lock);
>>>
>> Look again... clp is pointing to global data... so locks
>> are needed...
>>
>
> Yes, clp is *pointing* at global data, so the data it
> points at may change. But this pointer itself is
> a variable on your local stack which is not even
> visible to any other thread; so it can't change.
>
> I assume this was once meant to protect access
> to the clntraw_private pointer, which is a global
> variable and could do with some mutex protection,
> and I guess at some point the code looked like this:
>
> mutex_lock()
> clp = clntraw_private;
> if (clp == NULL) {
> unlock and fail
> }
>
> and some misguided soul probably moved the
> pointer assignment out of the protected region
> so that the code now looks the way it does.
> This needs fixing.
>
>

It seems to me that you are still missing the point of the lock.

The lock is not being used to protect the assignment of the address
of clnt_raw_private to clp, it is being used to ensure that clp is not
being used before clnt_raw_private is completely initialized.

The lock is also used to ensure that multiple copies of clnt_raw_private
are not allocated.

Arguably, this could have been implemented using a different locking
strategy, one which recovered from discovered races, instead of
preventing them upfront, but it works and is correct.

>>> - glibc switched to poll(), whereas tirpc still uses
>>> select with all its limitations
>>>
>> educate me... why is using poll() better than select()?
>>
>
> fd_set has a limit on the max file descriptor it can
> accomodate (1024 I think). poll does not. So if you use
> that code in apps that have files with descriptors > 1024
> you will start clobbering heap or stack memory.
>
> Under the hood, they're all the same - glibc's select
> will call sys_poll.
>
>

Here is a place where a newer version of the TI-RPC library would help.

>>> - in tirpc, clnt_sperror uses snprintf to avoid buffer overflows,
>>> but gets it all wrong wrt the return value of snprintf.
>>>
>> hmm... how is:
>> i = snprintf(str, len, "; errno = %s", strerror(e.re_errno));
>> if (i > 0) {
>> str += i;
>> len -= i;
>> }
>> wrong? looks reasonable to me... what am I missing?
>>
>
> snprintf returns the number of characters it *would* have
> printed if the buffer size had been sufficient. It does not
> return the number of characters actually printed, which is
> what this code seems to assume. So if the buffer is
> too small, you end up with str pointing beyond the end of
> the buffer, and len a huge unsigned value. That is decidedly
> not what you want :-)
>
>

Yup, that was a bug, and has already been fixed in a newer version of the
library.

>>> - tirpc is K&R all over the place, while glibc is mostly
>>> cleaned up (not everywhere though)
>>>
>> I'm not sure why "K&R all over the place" is bad but the
>> tirprc code is much better documented and much less hacker...
>> Meaning, in the glibc code, fixes did not follow the original
>> style... which make the fix stand out... this is not the case
>> with the tirpc code...
>>
>
> I agree it's a matter of taste. Like wearing bell bottom jeans
> and pink shirts :-)
>
>

I think that a short bit of work would convert the argument lists from
K&R style to ANSI, so that shouldn't be a factor.

We could also take a shot at addressing the multitude of types which
are used and update them to the current thinking.

>>> - tirpc bindresvport is IPv6 aware, but has a bug: if the
>>> caller passes a sockaddr with sin_port/sin6_port set,
>>> the routine will actually bind to htons(port).
>>>
>> I must be missing something... in the AF_INET6: the port is set
>> to sin6->sin6_port; which will be the first port that is tried... right?
>> and that is bad?
>>
>
> It uses htons alright, but it forgot the ntohs
>
> switch (af) {
> case AF_INET:
> sin = (struct sockaddr_in *)sa;
> salen = sizeof(struct sockaddr_in);
> port = sin->sin_port; /* <--- needs ntohs */
> ...
> *portp = htons(port++);
>
>

This seems like another bug that is already fixed in newer versions of
TI-RPC.

>>> - the authunix code in tirpc assumes sizeof(gid_t)
>>> equals sizeof(int)
>>>
>> this is true... are there any modern day machine architecture
>> where sizeof(gid_t) == sizeof(short)?
>>
>
> Not as far as I know, but that's beside the point I think. This kind
> of assumption is always bad.
>
> Two more things:
>
> - tirpc does a getenv("NETNAME") which should probably be
> secure_getenv().
>
> - in libc, clntudp_call uses the IP_RECVERR feature of the
> Linux kernel, so it gets notified of ICMP errors (like
> port_unreach). The TIRPC code does not, so the
> application will never see any error and has to wait
> for a timeout instead.
>

This seems like a porting issue to me. Thank you for pointing it
out so that it can get addressed.

>
>> Now I did not spend the time just to be argumentative with Olaf...
>>
>
> No offense taken. I should have probably explained my points
> better than I did
>
>
>> Hell... I hope he keeps poking holes... is the best thing for the
>> code... but I did spend the time because I've never been that
>> impressed with the RPC in glibc in the first place... So I could
>> not image any other implementation to be as bad as I perceived the
>> glibc implementation is and it turns out I was right... the tirpc
>> is time tested, does support IPv6 and is well documented... among
>> other thing... Plus... we have no choice... the glibc people
>> will not change anything... and with Uli... nothing means nothing!
>>
>> So in the end I truly do think porting rpc service (like nfs-utils,
>> nis, etc) is the right thing to do...
>>
>
> That's where we disagree :-) I think ripping out the glibc
> code and replacing it will cause a lot of maintenance for
> the distros.
>

I am curious what basis that you are using for this? Why will this
cause either more or less maintenance for distros?

It seems to me that this would give us some RPC code, which we could
support, which was newer than the current glibc code, and would have
more people working on and supporting it. That all seems like goodness
to me. The current situation is that we can't maintain the RPC code
at all, which is not goodness at all.

Thanx...

ps

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-27 14:10:15

by Christoph Hellwig

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Fri, Apr 27, 2007 at 10:01:53AM -0400, Peter Staubach wrote:
> This version of the library may or may not be MT-safe, but newer
> versions are definitely safe. Perhaps the Bull people could tell
> us how old the library is?

The README file claims it's a Sun codedrop from 1993 (!)

> >That's where we disagree :-) I think ripping out the glibc
> >code and replacing it will cause a lot of maintenance for
> >the distros.
> >
>
> I am curious what basis that you are using for this? Why will this
> cause either more or less maintenance for distros?

It's the same reason why we tell people to send us incremental
kernel patches. Replacing a codebase completely is a not calculateable
risk. It might work out just fine, but usually it doesn't because
something silently breaks. Updating the existing codebases with
small bugfixes or new features means you have to think about and justify
every little understandable change and there's a much higher chance that
it won't cause problems. In the end some of them will cause problems
anyway, but at least you can easily track the problem down to a single
change now.

> It seems to me that this would give us some RPC code, which we could
> support, which was newer than the current glibc code, and would have
> more people working on and supporting it. That all seems like goodness
> to me. The current situation is that we can't maintain the RPC code
> at all, which is not goodness at all.

I don't think the problem is that we can't maintain it, but rather
that no one stepped up to maintain it so far.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-27 14:21:22

by Peter Staubach

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

Christoph Hellwig wrote:
> On Fri, Apr 27, 2007 at 10:01:53AM -0400, Peter Staubach wrote:
>
>> This version of the library may or may not be MT-safe, but newer
>> versions are definitely safe. Perhaps the Bull people could tell
>> us how old the library is?
>>
>
> The README file claims it's a Sun codedrop from 1993 (!)
>
>

Woof! There may have been a few bug fixes made since then... :-)

>>> That's where we disagree :-) I think ripping out the glibc
>>> code and replacing it will cause a lot of maintenance for
>>> the distros.
>>>
>>>
>> I am curious what basis that you are using for this? Why will this
>> cause either more or less maintenance for distros?
>>
>
> It's the same reason why we tell people to send us incremental
> kernel patches. Replacing a codebase completely is a not calculateable
> risk. It might work out just fine, but usually it doesn't because
> something silently breaks. Updating the existing codebases with
> small bugfixes or new features means you have to think about and justify
> every little understandable change and there's a much higher chance that
> it won't cause problems. In the end some of them will cause problems
> anyway, but at least you can easily track the problem down to a single
> change now.
>
>

Yes, I agree with all that.

However, we are also looking at making fairly major changes to the
support already. I think that the situation will be more complex
than we would ideally like, no matter what we do. The IPv6 support
is not trivial stuff and will most likely have an impact on the
applications that use RPC no matter what we do. There are other
things that we can and perhaps should do as well to increase the
usefulness of the support. I suspect that there are things that
we could do to reduce the number of UDP and TCP ports being used,
for example.

>> It seems to me that this would give us some RPC code, which we could
>> support, which was newer than the current glibc code, and would have
>> more people working on and supporting it. That all seems like goodness
>> to me. The current situation is that we can't maintain the RPC code
>> at all, which is not goodness at all.
>>
>
> I don't think the problem is that we can't maintain it, but rather
> that no one stepped up to maintain it so far.
>

I had the impression that SteveD had stepped up. Or am I speaking out of
turn for him? :-)

ps

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-27 14:38:02

by Christoph Hellwig

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Fri, Apr 27, 2007 at 10:21:08AM -0400, Peter Staubach wrote:
> However, we are also looking at making fairly major changes to the
> support already. I think that the situation will be more complex
> than we would ideally like, no matter what we do. The IPv6 support
> is not trivial stuff and will most likely have an impact on the
> applications that use RPC no matter what we do. There are other
> things that we can and perhaps should do as well to increase the
> usefulness of the support. I suspect that there are things that
> we could do to reduce the number of UDP and TCP ports being used,
> for example.

All this would hopefully be optional so we can at least switch
back to the old behaviour for debugging purposed.

Anyway, back to the topic of codebases. I'm not sure a 1993 codedrop
that went through a few hands since is a good start. I suspect the
best that could be done (although it involves a lot of the work) is
the following:

(1) split out the glibc rpc code into a library of it's own
(2) reformat the latest sun tirpc code drop and the ex-glibc code
the same way so it can actually be properly diffed
(3) compare it and port important fixes and possibly features over
from that codebase

Now there's some interesting differences between the two, not just
in the bug/feature categories Olaf already wrote a lot of notes.
For example I assume the latest Sun code still uses their own
threading package with the thr_ prefix functions that are quite
different from pthreads in many ways, while the glibc code currently
uses glibc's internal locking macros so both would need a conversion
to the proper pthreads interfaces first, etc.

(and the 2.3-based, ex-freebsd bull code uses a lot of ugly macros
to convert freebsd internalish locking and other libc bits back to
posix, that's not exactly maintainable either)

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-27 16:51:38

by Olaf Kirch

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Friday 27 April 2007 16:01, Peter Staubach wrote:
> It seems to me that you are still missing the point of the lock.
>
> The lock is not being used to protect the assignment of the address
> of clnt_raw_private to clp, it is being used to ensure that clp is not
> being used before clnt_raw_private is completely initialized.
>
> The lock is also used to ensure that multiple copies of clnt_raw_private
> are not allocated.

Are we talking of the same piece of code? Can you explain to me how
the following, which is a copy of Bull's TIRPC library, ensures this?

clntraw_create:

struct clntraw_private *clp = clntraw_private;
[...]

mutex_lock(&clntraw_lock);
if (clp == NULL) {
clp = (struct clntraw_private *)calloc(1, sizeof (*clp));
if (clp == NULL) {
mutex_unlock(&clntraw_lock);
return NULL;
}
[...]
clntraw_private = clp;
}

Assume 2 threads enter clntraw_create simultaneously,
and clntraw_private is NULL.
Both execute clp = clntraw_private, and both see a
value of NULL. Thread A grabs the mutex, thread B blocks.

Thread A finds clp == NULL, allocates memory, puts the
pointer into clntraw_private, does clever stuff and releases the
mutex.

Thread B wakes up, acquires the mutex. It checks
clp, which is still NULL, and allocates another
private object.

I really do think I understand the code, and there is
a problem. The mutex protects the clntraw_private
pointer, so the only safe thing to do is access that
variable while holding the lock, *not* before.

Olaf
--
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play
[email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-27 17:05:56

by Olaf Kirch

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Friday 27 April 2007 16:01, Peter Staubach wrote:
> There isn't a clear cut right answer, but the described behavior is
> at least definitive. There were a _lot_ of discussions inside of
> Sun, comp.protocols.nfs, and at Connectathons regarding this issue,
> without clear resolutions that were better than the current behavior.

At least authunix_create should return NULL, not make the
application die with SIGABRT.

Yes, authentication may fail, but it's better to try and
fail than give up without trying.

[...]
> This version of the library may or may not be MT-safe, but newer
> versions are definitely safe. Perhaps the Bull people could tell
> us how old the library is?
[...]
> Here is a place where a newer version of the TI-RPC library would help.
[...]
> Yup, that was a bug, and has already been fixed in a newer version of the
> library.
[...]
> This seems like another bug that is already fixed in newer versions of
> TI-RPC.
[...]
> This seems like a porting issue to me. Thank you for pointing it
> out so that it can get addressed.

You see why I'm arguing against just replacing working code
with something else, without understanding the ramifications?

Seems like at least it's time for a mor recent tirpc version.

> > That's where we disagree :-) I think ripping out the glibc
> > code and replacing it will cause a lot of maintenance for
> > the distros.
>
> I am curious what basis that you are using for this? Why will this
> cause either more or less maintenance for distros?

I admit this is not founded on anything quantifiable. Let's say
based on experience, if you replace 22,000 lines of code used
heavily by almost everyone, there's going to be considerable
churn. If you just rip out the getgroups change in glibc and
go back to the abort() behavior we discsussed above, I'd bet
there will be level3 support calls at $your_favorite_linux_vendor
that force you to put that code back in.

> It seems to me that this would give us some RPC code, which we could
> support, which was newer than the current glibc code, and would have
> more people working on and supporting it. That all seems like goodness
> to me. The current situation is that we can't maintain the RPC code
> at all, which is not goodness at all.

I'm all for forking the sunrpc code. I'm just not convinced that
it is a good idea to replace code that is arguably stable with
something else which may have been tested on Solaris but not
on Linux.

Olaf
--
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play
[email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-27 17:06:52

by Peter Staubach

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

Olaf Kirch wrote:
> On Friday 27 April 2007 16:01, Peter Staubach wrote:
>
>> It seems to me that you are still missing the point of the lock.
>>
>> The lock is not being used to protect the assignment of the address
>> of clnt_raw_private to clp, it is being used to ensure that clp is not
>> being used before clnt_raw_private is completely initialized.
>>
>> The lock is also used to ensure that multiple copies of clnt_raw_private
>> are not allocated.
>>
>
> Are we talking of the same piece of code? Can you explain to me how
> the following, which is a copy of Bull's TIRPC library, ensures this?
>
> clntraw_create:
>
> struct clntraw_private *clp = clntraw_private;
> [...]
>
> mutex_lock(&clntraw_lock);
> if (clp == NULL) {
> clp = (struct clntraw_private *)calloc(1, sizeof (*clp));
> if (clp == NULL) {
> mutex_unlock(&clntraw_lock);
> return NULL;
> }
> [...]
> clntraw_private = clp;
> }
>
> Assume 2 threads enter clntraw_create simultaneously,
> and clntraw_private is NULL.
> Both execute clp = clntraw_private, and both see a
> value of NULL. Thread A grabs the mutex, thread B blocks.
>
> Thread A finds clp == NULL, allocates memory, puts the
> pointer into clntraw_private, does clever stuff and releases the
> mutex.
>
> Thread B wakes up, acquires the mutex. It checks
> clp, which is still NULL, and allocates another
> private object.
>
> I really do think I understand the code, and there is
> a problem. The mutex protects the clntraw_private
> pointer, so the only safe thing to do is access that
> variable while holding the lock, *not* before.
>

You are right, the code snippet above is obviously incorrect.

My apologies, I was looking at two different pieces of code and
commented about one, in response to comments about the other. I
also hadn't realized that you were talking about clntraw_create
because there are several other instances of pretty much the
same construct in other places in the same file.

The newer versions of the TI-RPC support have this bug fixed.
Plus, they use the global variable, clnt_raw_private, instead of
clntraw_private. (That's why I changed the name in my response...)

ps

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-27 17:34:26

by Peter Staubach

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

Olaf Kirch wrote:
> On Friday 27 April 2007 16:01, Peter Staubach wrote:
>
>> There isn't a clear cut right answer, but the described behavior is
>> at least definitive. There were a _lot_ of discussions inside of
>> Sun, comp.protocols.nfs, and at Connectathons regarding this issue,
>> without clear resolutions that were better than the current behavior.
>>
>
> At least authunix_create should return NULL, not make the
> application die with SIGABRT.
>
> Yes, authentication may fail, but it's better to try and
> fail than give up without trying.
>
>

Here, we are probably just going to have to agree to disagree, since
I don't agree and I don't think that this is the place to hold that
discussion, again. :-)


> [...]
>
>> This version of the library may or may not be MT-safe, but newer
>> versions are definitely safe. Perhaps the Bull people could tell
>> us how old the library is?
>>
> [...]
>
>> Here is a place where a newer version of the TI-RPC library would help.
>>
> [...]
>
>> Yup, that was a bug, and has already been fixed in a newer version of the
>> library.
>>
> [...]
>
>> This seems like another bug that is already fixed in newer versions of
>> TI-RPC.
>>
> [...]
>
>> This seems like a porting issue to me. Thank you for pointing it
>> out so that it can get addressed.
>>
>
> You see why I'm arguing against just replacing working code
> with something else, without understanding the ramifications?
>
>

Well, I don't quite think that it is that black and white. Sometimes,
changes like this one will prove to be, can affect functionality.

> Seems like at least it's time for a mor recent tirpc version.
>
>

Yes, definitely. :-)

>>> That's where we disagree :-) I think ripping out the glibc
>>> code and replacing it will cause a lot of maintenance for
>>> the distros.
>>>
>> I am curious what basis that you are using for this? Why will this
>> cause either more or less maintenance for distros?
>>
>
> I admit this is not founded on anything quantifiable. Let's say
> based on experience, if you replace 22,000 lines of code used
> heavily by almost everyone, there's going to be considerable
> churn. If you just rip out the getgroups change in glibc and
> go back to the abort() behavior we discsussed above, I'd bet
> there will be level3 support calls at $your_favorite_linux_vendor
> that force you to put that code back in.
>
>

Maybe. I also wouldn't be surprised if no one noticed for quite
some time too.

>> It seems to me that this would give us some RPC code, which we could
>> support, which was newer than the current glibc code, and would have
>> more people working on and supporting it. That all seems like goodness
>> to me. The current situation is that we can't maintain the RPC code
>> at all, which is not goodness at all.
>>
>
> I'm all for forking the sunrpc code. I'm just not convinced that
> it is a good idea to replace code that is arguably stable with
> something else which may have been tested on Solaris but not
> on Linux.

I am one of those who believe that there finally comes a time
to cut the past loose and move forward with a whole new code base.
This sort of decision must be made carefully, but continuing to
hack and kludge on the same old code, just for the sake of the
perception that doing so won't impact existing users, always just
ends up being false and those users get impacted.

Change is not something to be afraid of. It is healthy and as
long as we make intelligent decisions, based on as much information
as we can get, we will be fine and our users will be pleased with us.

ps

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-29 23:38:33

by Steve Dickson

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

Peter Staubach wrote:
>>
>> I don't think the problem is that we can't maintain it, but rather
>> that no one stepped up to maintain it so far.
>>
>
> I had the impression that SteveD had stepped up. Or am I speaking out of
> turn for him? :-)
Since the libtirpc is already in FC6 (extras) and will be in FC7, I
have every intention on help the Bull people in maintaining this
code...

steved.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-24 06:44:24

by Olaf Kirch

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Tuesday 24 April 2007 01:09, Neil Brown wrote:
> It would be nice if the libritpc version of bindrecvport could be
> configured to avoid some list of ports, whether from /etc/services or
> from elsewhere.

BTW, I wouldn't use /etc/services as the blacklist for bindresvport.
The range of available privileged ports is rather tight already. If
you exclude everything found in /etc/services, you're down to
249 ports in the 512-1024 range (for TCP and UDP, each). This
will not please the 10,000 mounts crowd at Prominent CPU Vendor :)
I really think you want to go with a separate blacklist file.

Olaf
--
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play
[email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-24 07:09:42

by Olaf Kirch

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Tuesday 24 April 2007 01:09, Neil Brown wrote:
> Is there someone "maintaining" rpcbind? Should there be?
> I notice there is an rpcbind at Wietse Venema's site:
> ftp://ftp.porcupine.org/pub/security/index.html
>
> Is this rpcbind derived from that?

Bull maintains a copy of rpcbind alongside their tirpc code.
I think both Wietse's and Bull's rpcbind implementation derive
from the TI-RPC code released by Sun some time during the
last millenium.

Personally, I'm very wary of the tirpc code. I think it needs a haircut
and a thorough security audit before it can go into distributions.
I did a very quick comparison of the tirpc code vs glibc, and
I think there's a potential buffer overflow if a rogue rpcbind server
replies with a string length of 0xffffffff.

The rpcb_clnt.c functions use xdr_wrapstring, which
gives a max string size of MAX_UNSIGNED, so a string
length of 0xffffffff would be acceptable. The subsequent malloc
would allocate a buffer of length 0 however. Depending on
how you link your binary, malloc may return NULL in that case
(simple segfault), or a very small buffer - and the moment you
start copying to that, you will corrupt the heap. This is fixed
in glibc, but still exists in tirpc. A security audit may turn up
more.

Apart from these concerns, the coding style is abominable - K&R
almost everywhere; there's still a bunch of u_long's in
the code which makes it non-64bit-clean, and there's
crap like

clnt_st = CLNT_CALL(client, (rpcproc_t)RPCBPROC_GETADDR,
(xdrproc_t) xdr_rpcb, (char *)(void *)&parms,
(xdrproc_t) xdr_wrapstring, (char *)(void *) &ua, *tp);

Those casts aren't just plain ugly, they're utterly useless too, since
the pointer arguments to cl_call are void *.

Olaf
--
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play
[email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-24 14:38:54

by Steve Dickson

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??



Neil Brown wrote:
> On Monday April 23, [email protected] wrote:
>>>> I (hastily) created two git trees:
>>>> git://git.infradead.org/~steved/libtirpc.git
>>>> git://git.infradead.org/~steved/rpcbind.git
>>>>
>>>> I think if you take a look, you'll see that
>>>> this code may not be as mature as the portmap
>>>> code, but its a much better start... imho..
>>> Yes, very hasty. Several #temporary# and back~ files :-)
>> Yeah... for some reason those temporary files are in the
>> the tar ball... I'll work to get that cleaned up..
>
> Is there someone "maintaining" rpcbind? Should there be?
Well the I got the current code from Bull:
http://nfsv4.bullopensource.org/doc/tirpc_rpcbind.php

As far as maintaining it.. since I have vested interested
in see this code work (being it now in FC7), I will be
playing a active roll in the maintenance, but Bull
should probably be seen as the upstream for this code..

> I notice there is an rpcbind at Wietse Venema's site:
> ftp://ftp.porcupine.org/pub/security/index.html
>
> Is this rpcbind derived from that?
No clue... Added Tony to the cc list to see if he knows...
but I must say it certainly has the look and feel of the
current rpcbind code...

>
> Should rpcbind and portmap "live" close together so that people
> looking for one will find the other, and so that compatibility
> (features, options) can be maximised?
>
>>> I notice that it has a concept of who 'owns' a registration, but it
>>> only works if unix-domain sockets are used for the registration.
>>> Adding 'superuser' ownership for localhost/privport registrations is
>>> probably a 3 line patch....
>> I'm not sure I understand.. are you talking about how getowner()
>> is being used?
>
> Sort of. I was actually looking in pmapproc_change which seems to
> have 'getowner' open-coded in it.
> Both call __rpc_get_local_uid.
> That function (in libritpc) returns -1 ( => "unknown") for a AF_INET
> connection.
> I believe that for AF_INET, it should check sin_addr and sin_port.
> If addr == 127.0.0.1 and sin_port < 1024, then uid should be set to
> 0.
Ah... I see your point...

steved.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-24 15:10:41

by Steve Dickson

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

Olaf Kirch wrote:
> On Tuesday 24 April 2007 01:09, Neil Brown wrote:
>> Is there someone "maintaining" rpcbind? Should there be?
>> I notice there is an rpcbind at Wietse Venema's site:
>> ftp://ftp.porcupine.org/pub/security/index.html
>>
>> Is this rpcbind derived from that?
>
> Bull maintains a copy of rpcbind alongside their tirpc code.
> I think both Wietse's and Bull's rpcbind implementation derive
> from the TI-RPC code released by Sun some time during the
> last millenium.
>
> Personally, I'm very wary of the tirpc code. I think it needs a haircut
> and a thorough security audit before it can go into distributions.
I agree the code is a bit rough at this point... and care should be
taken with who and how the code is used at this point, which is
the reason only rpcbind is using the library and rpcbind is not
running as root... but... I do think correct way to go in
the supporting of IPv6 and other well established features.


> I did a very quick comparison of the tirpc code vs glibc, and
> I think there's a potential buffer overflow if a rogue rpcbind server
> replies with a string length of 0xffffffff.
>
> The rpcb_clnt.c functions use xdr_wrapstring, which
> gives a max string size of MAX_UNSIGNED, so a string
> length of 0xffffffff would be acceptable. The subsequent malloc
> would allocate a buffer of length 0 however. Depending on
> how you link your binary, malloc may return NULL in that case
> (simple segfault), or a very small buffer - and the moment you
> start copying to that, you will corrupt the heap. This is fixed
> in glibc, but still exists in tirpc. A security audit may turn up
> more.
Yes... there is a ton of wisdom we can pull from the glibc code
to make this code better... and since the glibc people are for
this movement (i.e. moving away from the glibc RPC code) I'm
hoping they will be willing to help us out in this effort...

>
> Apart from these concerns, the coding style is abominable - K&R
> almost everywhere; there's still a bunch of u_long's in
> the code which makes it non-64bit-clean, and there's
> crap like
>
> clnt_st = CLNT_CALL(client, (rpcproc_t)RPCBPROC_GETADDR,
> (xdrproc_t) xdr_rpcb, (char *)(void *)&parms,
> (xdrproc_t) xdr_wrapstring, (char *)(void *) &ua, *tp);
>
> Those casts aren't just plain ugly, they're utterly useless too, since
> the pointer arguments to cl_call are void *.
Yes the coding style a bit challenging.... but again this is something
that is definitely fixable over time...

Are there any tools out there that might help with the cleaning up
of this code?


steved.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-24 16:11:01

by Christoph Hellwig

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Tue, Apr 24, 2007 at 09:08:38AM +0200, Olaf Kirch wrote:
> On Tuesday 24 April 2007 01:09, Neil Brown wrote:
> > Is there someone "maintaining" rpcbind? Should there be?
> > I notice there is an rpcbind at Wietse Venema's site:
> > ftp://ftp.porcupine.org/pub/security/index.html
> >
> > Is this rpcbind derived from that?
>
> Bull maintains a copy of rpcbind alongside their tirpc code.
> I think both Wietse's and Bull's rpcbind implementation derive
> from the TI-RPC code released by Sun some time during the
> last millenium.
>
> Personally, I'm very wary of the tirpc code. I think it needs a haircut
> and a thorough security audit before it can go into distributions.
> I did a very quick comparison of the tirpc code vs glibc, and
> I think there's a potential buffer overflow if a rogue rpcbind server
> replies with a string length of 0xffffffff.
>
> The rpcb_clnt.c functions use xdr_wrapstring, which
> gives a max string size of MAX_UNSIGNED, so a string
> length of 0xffffffff would be acceptable. The subsequent malloc
> would allocate a buffer of length 0 however. Depending on
> how you link your binary, malloc may return NULL in that case
> (simple segfault), or a very small buffer - and the moment you
> start copying to that, you will corrupt the heap. This is fixed
> in glibc, but still exists in tirpc. A security audit may turn up
> more.
>
> Apart from these concerns, the coding style is abominable - K&R
> almost everywhere; there's still a bunch of u_long's in
> the code which makes it non-64bit-clean, and there's
> crap like
>
> clnt_st = CLNT_CALL(client, (rpcproc_t)RPCBPROC_GETADDR,
> (xdrproc_t) xdr_rpcb, (char *)(void *)&parms,
> (xdrproc_t) xdr_wrapstring, (char *)(void *) &ua, *tp);
>
> Those casts aren't just plain ugly, they're utterly useless too, since
> the pointer arguments to cl_call are void *.

Although not the fastes approach the only long-term viable one
would be to update the glibc rpc code for the new TIRPC APIs and
IPv6 support. Even if it's unclear whether Uli would actually
include it that gives a better start to avoid all kinds of security
or useability-related regressions.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-24 17:05:10

by Steve Dickson

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??



Christoph Hellwig wrote:
>
> Although not the fastes approach the only long-term viable one
> would be to update the glibc rpc code for the new TIRPC APIs and
> IPv6 support. Even if it's unclear whether Uli would actually
> include it that gives a better start to avoid all kinds of security
> or useability-related regressions.
Well I have a strong hunch that the glibc people do not want anything
to do with changing *any* of the RPC code. I know for a fact that they
are very sorry the accepted in the first place... so with
that type of motivation I really think it would be very painful
to sell them on take any patches... at all...

Secondly, having the to debug and fix a small part of the
RPC code in glibc, I have experienced how truly difficult
it is work with such a large beast... so I pretty confident
that breaking out the RPC into a library we can maintain
ourselves is a good direction to move...


steved.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-24 17:17:21

by Christoph Hellwig

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Tue, Apr 24, 2007 at 01:04:57PM -0400, Steve Dickson wrote:
> >Although not the fastes approach the only long-term viable one
> >would be to update the glibc rpc code for the new TIRPC APIs and
> >IPv6 support. Even if it's unclear whether Uli would actually
> >include it that gives a better start to avoid all kinds of security
> >or useability-related regressions.
> Well I have a strong hunch that the glibc people do not want anything
> to do with changing *any* of the RPC code.

That's the impression I got, yes.

> I know for a fact that they
> are very sorry the accepted in the first place... so with
> that type of motivation I really think it would be very painful
> to sell them on take any patches... at all...
>
> Secondly, having the to debug and fix a small part of the
> RPC code in glibc, I have experienced how truly difficult
> it is work with such a large beast... so I pretty confident
> that breaking out the RPC into a library we can maintain
> ourselves is a good direction to move...

Well, even with a separate library it's probably better to
start out with the glibc code to bug to bug compatible.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-24 18:13:31

by Steve Dickson

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??



Christoph Hellwig wrote:
>> Secondly, having the to debug and fix a small part of the
>> RPC code in glibc, I have experienced how truly difficult
>> it is work with such a large beast... so I pretty confident
>> that breaking out the RPC into a library we can maintain
>> ourselves is a good direction to move...
>
> Well, even with a separate library it's probably better to
> start out with the glibc code to bug to bug compatible.
In theory yes... but unfortunately Transport Independent
RPC code is much different than the RPC we have in glibc.
The TI-RPC code deals with things like CLTS and COTS and
has configuration file that defines how theses CLITS/COTS
are handled... notions that are completely foreign to
the current RPC code...

Again, I realize this code is very raw, but the only way
to bring it up to speed (something I really thing we want
to do) is using it and making it available to the community.

steved.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-24 20:27:00

by Steve Dickson

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

CgpQZXRlciDDhXN0cmFuZCB3cm90ZToKPiBPbmUgdGhpbmcgdGhhdCBib3RoZXJzIG1lIGlzIHRo
YXQgdGhlIGN1cnJlbnQgbGlidGlycGNbKl0gaXMgZGlmZmVyZW50IAo+IGZyb20gb3RoZXIgVEkt
UlBDIHZlcnNpb25zLiBUaGUgY3VycmVudCB2ZXJzaW9uIChhcyBmYXIgYXMgSSB1bmRlcnN0YW5k
KSAKPiBpcyBiYXNlZCBvbiBhIHNuYXBzaG90IGZyb20gRnJlZUJTRCwgd2hpY2ggaXMgdHVybiBp
cyBiYXNlZCBvbiBTdW5zIFRJUlBDIAo+IDIuMy4gVGhpcyBjb2RlIGlzICpub3QqIHVwIHRvIGRh
dGUgd2l0aCBUSS1SUEMgMi45IChha2EgdGlycGNzcmNfOTkpIGFuZCAKPiBjZXJ0YWlubHkgbm90
IHdpdGggdGhlIGxhdGVzdCBPcGVuU29sYXJpcyB2ZXJzaW9uLgpobW0uLi4gV2VyZSBkb2VzIG9u
ZSBnbyB0byBmaW5kIG91dCBob3cgdG8gYmUgY29tZSB0aXJwY3NyY185OQpjb21wbGlhbnQgYW5k
IHdoeSBpcyB0aGlzIGltcG9ydGFudD8gRG9lcyB0aGUgVEktUlBDIDIuOQp1c2UgYSBkaWZmZXJl
bnQgb3Zlci10aGUtd2lyZSBwcm90b2NvbD8gSSBzdXJlbHkgaG9wZSBub3QuLi4gOi1cCgo+IAo+
IEFub3RoZXIgcHJvYmxlbSB3aXRoIFRJLVJQQyBpcyB0aGF0IGEgV2luZG93cyBwb3J0IHNlZW1z
IHRvIGJlIGhhcmQuIAo+IFRoYXQncyBvbmUgcmVhc29uIHdoeSBJJ20gbm90IHBvcnRpbmcgdW5m
czMgdG8gVEktUlBDLiAKV293Li4uIEkgZGlkbid0IGV2ZW4ga25vdyBXaW5kb3dzIGhhZCBSUEMg
c3VwcG9ydC4uLiBidXQKSSBraW5kYSBkb3VidCB3ZSdsbCBiZSBwb3J0IG5mcy11dGlscyB0byBX
aW5kb3dzIGFueXRpbWUKc29vbi4uIDotKQoKPiAKPiBHZXR0aW5nIExpbnV4IGRpc3RyaWJ1dGlv
bnMgdG8gc2hpcCAodGhlIHNhbWUpIHZlcnNpb24gb2YgVEktUlBDIGlzIGEgCj4gd29ydGh3aGls
ZSBnb2FsLCBidXQgaWRlYWxseSwgd2Ugc2hvdWxkIGhhdmUgYW4gdXBzdHJlYW0gcHJvamVjdCB0
aGF0IAo+IGNvdmVycyBub3Qgb25seSBMaW51eCwgYnV0IGFsc28sICpCU0QsIE9wZW5Tb2xhcmlz
IGV0Yy4gSSBrbm93IGl0J3Mgbm90IAo+IGdvaW5nIHRvIGJlIGVhc3ksIGJ1dCBJIHRoaW5rIGl0
J3Mgd29ydGggYSB0cnkuCldlbGwgaWYgd2UgaGF2aW5nIHN1cHBvcnQgdGhhdHMgbmVlZGVkIGFu
ZC9vciB3YW50ZWQgaXQgc2hvdWxkIG5vdApiZSB0b28gZGlmZmljdWx0IHRvIGdldCBwZW9wbGUg
dG8gbW92ZSBvdmVyLi4uCgpzdGV2ZWQuCgoKLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQpUaGlzIFNGLm5ldCBl
bWFpbCBpcyBzcG9uc29yZWQgYnkgREIyIEV4cHJlc3MKRG93bmxvYWQgREIyIEV4cHJlc3MgQyAt
IHRoZSBGUkVFIHZlcnNpb24gb2YgREIyIGV4cHJlc3MgYW5kIHRha2UKY29udHJvbCBvZiB5b3Vy
IFhNTC4gTm8gbGltaXRzLiBKdXN0IGRhdGEuIENsaWNrIHRvIGdldCBpdCBub3cuCmh0dHA6Ly9z
b3VyY2Vmb3JnZS5uZXQvcG93ZXJiYXIvZGIyLwpfX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fXwpORlMgbWFpbGxpc3QgIC0gIE5GU0BsaXN0cy5zb3VyY2Vmb3Jn
ZS5uZXQKaHR0cHM6Ly9saXN0cy5zb3VyY2Vmb3JnZS5uZXQvbGlzdHMvbGlzdGluZm8vbmZzCg==

2007-04-24 20:38:07

by Peter Staubach

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

Steve Dickson wrote:
> Peter =C5strand wrote:
> =

>> One thing that bothers me is that the current libtirpc[*] is different =

>> from other TI-RPC versions. The current version (as far as I understand) =

>> is based on a snapshot from FreeBSD, which is turn is based on Suns TIRP=
C =

>> 2.3. This code is *not* up to date with TI-RPC 2.9 (aka tirpcsrc_99) and =

>> certainly not with the latest OpenSolaris version.
>> =

> hmm... Were does one go to find out how to be come tirpcsrc_99
> compliant and why is this important? Does the TI-RPC 2.9
> use a different over-the-wire protocol? I surely hope not... :-\
>
> =


The chance that Sun actually changed any of the interfaces from one
version of TIRPC to the next is virtually nil. Compatibility is a
huge thing to them. Any changes would most likely have been made
in a backwards compatible fashion.


And no, there are no differences in the over the wire protocol.

>> Getting Linux distributions to ship (the same) version of TI-RPC is a =

>> worthwhile goal, but ideally, we should have an upstream project that =

>> covers not only Linux, but also, *BSD, OpenSolaris etc. I know it's not =

>> going to be easy, but I think it's worth a try.
>> =

> Well if we having support thats needed and/or wanted it should not
> be too difficult to get people to move over...

Having such a project would be a good thing. We would need developers
for each release platform targeted though. Perhaps it might be sufficient
to start with platforms that we are interested in and as people with other
platforms become interested, they can port the support and add their own
patches to the central source base.

I don't think that I understand why being bug for bug compatible with
the current glibc code is interesting. This is a new version of the
RPC support and applications will explicitly need to be ported to it.
Perhaps we could even pleasantly surprise some folks by addressing
some particular pet bug that they might have encountered.

Thanx...

ps

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-25 08:57:23

by Olaf Kirch

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Tuesday 24 April 2007 19:52, Steve Dickson wrote:
> In theory yes... but unfortunately Transport Independent
> RPC code is much different than the RPC we have in glibc.

Still, there's a whole load of shared code, and this is where
I agree with Christoph that it would be foolish to throw away
the work that went into the glibc code. The buffer overflow fix
I mentioned is just one such thing - I did a diff of the files
common in glibc and tirpc, and here are some of the
things I found.

- glibc authunix_create_default works if the process has
more than NGRPS gids. tirpc wil fail

- glibc tries to be more multithread friendly, by using
things like gethostbyname_r instead of gethostbyname.
tirpc doesn't.

- it seems that tirpc unconditionally requires rpcbind,
even for applications that still use pmap_set and
friends. In the interest of a smooth transition, applications
using the old interface should be able to expect the
old behavior.

- tirpc does truly bizarre things, eg in clnt_raw.c
it declares a local variable, and then acquires
a mutex to check this local variable for NULL:

mutex_lock(&clntraw_lock);
if (clp == NULL) {
mutex_unlock(&clntraw_lock);
return (RPC_FAILED);
}
mutex_unlock(&clntraw_lock);

- tirpc has a mechanism that lets you register
server side auth flavors dynamically; libc
doesn't.

- glibc switched to poll(), whereas tirpc still uses
select with all its limitations

- in tirpc, svc.c seems to be thread safe, whereas
glibc isn't.

- tirpc lets the application control the maximum record
size, making DoS attacks just a little harder.

- in tirpc, xdr_mem.c has different ops for aligned vs
unaligned buffers, for what it's worth.

- in tirpc, clnt_sperror uses snprintf to avoid buffer overflows,
but gets it all wrong wrt the return value of snprintf.

- tirpc is K&R all over the place, while glibc is mostly
cleaned up (not everywhere though)

- tirpc bindresvport is IPv6 aware, but has a bug: if the
caller passes a sockaddr with sin_port/sin6_port set,
the routine will actually bind to htons(port).
the libc bindresvport implementation also has a small
optimization in that it will continue searching where the
previous call to bindresvport left off, rather than starting
all over from 600.

- the authunix code in tirpc assumes sizeof(gid_t)
equals sizeof(int)

- the glibc RPC client side seems to be thread safe, whereas
tirpc seems to be lacking in some areas (see rpc_thread.c)

I stopped reading the code after an hour, but I think this list could
be extended quite a bit.

So from my POV the decision between tirpc and glibc rpc isn't
that clear-cut. TIRPC clearly needs some work before distros
should consider using it.

> Again, I realize this code is very raw, but the only way
> to bring it up to speed (something I really thing we want
> to do) is using it and making it available to the community.

Well, there's one thing that makes the transition very tricky.
I looked into this while I was still with Novell, because we had
a request from the Bull folks to include tirpc. If you include
tirpc, you either have to remove all RPC code from glibc
at the same time, and fix up all applications linking against
this code. Downside: no binary compatibility, and the customers
who paid a lot of money for some commercial app will hate
you if an OS upgrade requires them to buy new licenses.

The other approach is to make sure that you provide *any*
RPC related symbol glibc provides, in tirpc as well. Even
if it does nothing, and just errors out. Otherwise you may end
up with somebody's application linked against tirpc, but it
calls some forgotten little function in glibc and falls over.
This is tricky too.

This is why I had very strong reservations about including
tirpc in SLES.

I'm wondering whether all that hassle is really worth
it. There should be a way to IPv6 enable the glibc RPC
implementation without having to go to TIRPC. Yes,
Solaris requires rpcbind and such in order to do IPv6.
But that's not the same as replacing glibc's sunrpc code.

Olaf
--
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play
[email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-25 09:58:53

by Christoph Hellwig

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Wed, Apr 25, 2007 at 10:56:01AM +0200, Olaf Kirch wrote:
> I'm wondering whether all that hassle is really worth
> it. There should be a way to IPv6 enable the glibc RPC
> implementation without having to go to TIRPC. Yes,
> Solaris requires rpcbind and such in order to do IPv6.
> But that's not the same as replacing glibc's sunrpc code.

Yepp. One other thing about tirpc is that it pulls in a lot
of TLI crap. Given that we never implemented TLI on Linux
and never will this seems like a lot of useless baggae.

(Especially as SUN is backing down a lot on TLI aswell)

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-25 12:37:31

by Olaf Kirch

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Tuesday 24 April 2007 22:36, Peter Staubach wrote:
> I don't think that I understand why being bug for bug compatible with
> the current glibc code is interesting. This is a new version of the

Well, you don't want to re-introduce bugs that have been fixed in
glibc ages ago, right? Especially when it comes to security issues
and thread safety.

Olaf
--
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play
[email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-25 13:22:19

by Steve Dickson

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??



Christoph Hellwig wrote:
> On Wed, Apr 25, 2007 at 10:56:01AM +0200, Olaf Kirch wrote:
>> I'm wondering whether all that hassle is really worth
>> it. There should be a way to IPv6 enable the glibc RPC
>> implementation without having to go to TIRPC. Yes,
>> Solaris requires rpcbind and such in order to do IPv6.
>> But that's not the same as replacing glibc's sunrpc code.
>
> Yepp. One other thing about tirpc is that it pulls in a lot
> of TLI crap. Given that we never implemented TLI on Linux
> and never will this seems like a lot of useless baggae.
>
> (Especially as SUN is backing down a lot on TLI aswell)
I really don't have any idea what SUN is or is not doing,
and sure it *might* be possible to simply turn IPv6 in the
glibc RPC code but its been stated very clearly that will
not happen because the glibc people no longer want to
support that code... They have and will continue to
resit any and all changes...

So all I'm trying to is instead of beating our heads against
the glibc wall, I simply trying to go around it with an
alternative solution and the libtirpc library seems like
it would be a good alternative...

Are there any other alternatives out there???

steved.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-25 13:39:38

by Steve Dickson

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??



Olaf Kirch wrote:
> On Tuesday 24 April 2007 19:52, Steve Dickson wrote:
>> In theory yes... but unfortunately Transport Independent
>> RPC code is much different than the RPC we have in glibc.
>
> Still, there's a whole load of shared code, and this is where
> I agree with Christoph that it would be foolish to throw away
> the work that went into the glibc code.
Again I can't agree with you more... so lets port over that
"wisdom" into libtirpc...


> I stopped reading the code after an hour, but I think this list could
> be extended quite a bit.
Thanks for spending the time!!! I'll make sure each an every
one of these observations turning into a bug report...

>
>> Again, I realize this code is very raw, but the only way
>> to bring it up to speed (something I really thing we want
>> to do) is using it and making it available to the community.
>
> Well, there's one thing that makes the transition very tricky.
> I looked into this while I was still with Novell, because we had
> a request from the Bull folks to include tirpc. If you include
> tirpc, you either have to remove all RPC code from glibc
> at the same time, and fix up all applications linking against
> this code. Downside: no binary compatibility, and the customers
> who paid a lot of money for some commercial app will hate
> you if an OS upgrade requires them to buy new licenses.
Removing the RPC code from glibc is not an option. The
glibc people have made this very clear... So I see the
libtirpc library as an alternative to people that want
RPC over IPv6 support... and Yes they will have to recompile
to take advantage of this support... but I really don't think
that will be that big of a deal..

>
> The other approach is to make sure that you provide *any*
> RPC related symbol glibc provides, in tirpc as well. Even
> if it does nothing, and just errors out. Otherwise you may end
> up with somebody's application linked against tirpc, but it
> calls some forgotten little function in glibc and falls over.
> This is tricky too.
Again... libtirpc is not replacing the glibc code. People
will have to explicitly link with the tirpc... Will there
be porting issues... yes... aren't there always?

steved.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-25 14:12:16

by Olaf Kirch

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Wednesday 25 April 2007 15:22, Steve Dickson wrote:
> Are there any other alternatives out there???

Whatever we do, there needs to be some coordination with
the glibc folks. If they don't accept any patches to the sunrpc
code in their code base, then we need to talk about a way
of transitioning that code out of there.

Isn't there a way to frob symbols so that the loader finds them
while hiding them from the linker? That would allow us to link
apps against a new librpc without risk of pulling in symbols from
glibc, and at the same time maintain binary compatibility in glibc
runtime. I'm not enough of a toolchain person, but I think stuff like
that is done routinely.

With that in place, you can decide whether it's easier to
go to libtirpc and pull in any missing pieces from glibc, or
whether it's better to start with a copy of the glibc code,
put it into git somewhere and evolve it from there. I think the
latter approach makes more sense. Gradual transisitons are
always preferrable.

Olaf
--
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play
[email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-25 14:38:21

by Christoph Hellwig

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Wed, Apr 25, 2007 at 09:22:06AM -0400, Steve Dickson wrote:
> and sure it *might* be possible to simply turn IPv6 in the
> glibc RPC code but its been stated very clearly that will
> not happen because the glibc people no longer want to
> support that code... They have and will continue to
> resit any and all changes...

I have the feeling we're talking past each other here :-)

Even if changes to glibc itself are not going to happen you still can
take the glibc rpc code, make changes to it and turn it into a
separate library. Given the indicators that Ulrich doesn't want new
features in glibcs that seems to be the most useful solution.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-25 14:42:16

by Christoph Hellwig

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Wed, Apr 25, 2007 at 04:10:37PM +0200, Olaf Kirch wrote:
> Whatever we do, there needs to be some coordination with
> the glibc folks. If they don't accept any patches to the sunrpc
> code in their code base, then we need to talk about a way
> of transitioning that code out of there.

Yes.

> Isn't there a way to frob symbols so that the loader finds them
> while hiding them from the linker?

The only way to hide symbols seems to adding a new version symbol
of the same name ontop of it. Which actually might make some sense
here by making the new versions stubs while preserving ABI compatiblity
for any old binary around. But I suspect third parties would still
get very upset about this compile-time breakage.

Another alternative would be to not mess with the symbols in glibc
at all but use linker magic to emit a warning that people should use
librpc instead, similar to how linking any application using getc
against glibc will get you a warning that it's dangerous and should
not be used.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-25 15:44:52

by Peter Staubach

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

Olaf Kirch wrote:
> On Tuesday 24 April 2007 22:36, Peter Staubach wrote:
>
>> I don't think that I understand why being bug for bug compatible with
>> the current glibc code is interesting. This is a new version of the
>>
>
> Well, you don't want to re-introduce bugs that have been fixed in
> glibc ages ago, right? Especially when it comes to security issues
> and thread safety.

No, clearly, I wouldn't want to do that. However, I don't believe that
that is what is meant by "bug for bug compatible".

It is also a distinct possibility that the folks maintaining the TI-RPC
code which is in common use may have fixed a few bugs since the Linux
source bases, either the glibc or tirpc versions, were acquired. I
think that it would be good to have those, _plus_ any bugs which may
already been fixed in the glibc version. Wouldn't you agree?

Thanx...

ps

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-25 20:14:09

by Olaf Kirch

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

On Wednesday 25 April 2007 17:44, you wrote:
> It is also a distinct possibility that the folks maintaining the TI-RPC
> code which is in common use may have fixed a few bugs since the Linux
> source bases, either the glibc or tirpc versions, were acquired. I
> think that it would be good to have those, _plus_ any bugs which may
> already been fixed in the glibc version. Wouldn't you agree?

Which is why I did this comparison of the code today. In summary,
in the common files there's stuff in tirpc that glibc misses, but the
other way round there's just more.

I don't say tirpc is bad. What I say is, it's better to handle this the way
we deal with changes to the kernel: start with what you have, and
which you know works well. Add to it, until you have the functionality
you want. On the way, you may find out there's better ways of
solving things that you anticipated. If you run into sudden problems,
you can bisect.

For instance, I would rather like to see the current functionality
extended transparently:

- svc{tcp,udp}_create create an IPv6 socket rather than
an IPv4 socket by default. Try to use rpcbind style
registrations, but fall back to portmap if it doesn't
exist. This needs some extra magic to handle
IPv6 mapped IPv4 addresses in svc_getcaller, but that
should almost do it

- clnt{udp,tcp}_create needs some magic to find out
whether to use IPv6 or IPv4. The ability to create an
IPv6 socket doesn't mean anything; we probably need
a config mechanism somewhere. Else you will end up
with an awful lot of timeouts (ask the web browser folks
about their IPv6 pains). But it doesn't have to be something
complicated like netconf; it's more like an IPv6 on/off
switch, and a "prefer IPv4/IPv6" switch.

Good heuristics to decide whether to try IPv6
- ability to open an IPv6 socket
- DNS returns IPv6 addrs for the remote host
- there's an rpcbind on the server host, and it
returns udp6/tcp6 registrations
- if it's a global address, we need an interface
with a global prefix
- if it's a link local prefix, just try it.

- make clnt_create understand udp6/tcp6

This is the stuff we can do today, with little overhead, to bring
IPv6 support to the distros without making their choices
difficult. All we need is

- rpcbind
- rpcb client in the sunrpc lib
- make bindresvport ipv6 aware
- extended functionality as descrived above

This will not be perfect, but it would be a start. From there,
you can proceed by fixing clnt_broadcast to deal with
IPv6; you can add netconf if you want, and the whole
tirpc API if you wish.

Olaf
--
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play
[email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-04-26 07:51:56

by Aurélien Charbon

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

Hi all,

For us Bull, TI-RPC is just a mean to supply IPv6 support for RPC.
We always have considered that it is a raw code with bugs and code =

craps, that has not been tested enough.
We just use and supply it to test IPv6 support for NFSv4. And the code =

is stable enough to do tests with NFSv4 and IPv6.
However, we are developping some tests to improve its reliability, =

stability, performance, to make it acceptable in a more general context.

In the past, we discussed with glibc people to extend SunRPC library =

with IPv6 features. Uli Drepper clearly refused and explained that he =

did not want to see new bugs entering in RPC glibc library. He seems to =

consider that RPCs should not be in the glibc, and introduction of new =

features must be done in an external library.

That's why we decided to port an external tirpc library from FreeBSD =

(which is based on the same Sun code than Wietse's )

Aur=E9lien



Peter Staubach wrote:

>Steve Dickson wrote:
> =

>
>>Peter =C5strand wrote:
>> =

>> =

>>
>>>One thing that bothers me is that the current libtirpc[*] is different =

>>>from other TI-RPC versions. The current version (as far as I understand) =

>>>is based on a snapshot from FreeBSD, which is turn is based on Suns TIRP=
C =

>>>2.3. This code is *not* up to date with TI-RPC 2.9 (aka tirpcsrc_99) and =

>>>certainly not with the latest OpenSolaris version.
>>> =

>>> =

>>>
>>hmm... Were does one go to find out how to be come tirpcsrc_99
>>compliant and why is this important? Does the TI-RPC 2.9
>>use a different over-the-wire protocol? I surely hope not... :-\
>>
>> =

>> =

>>
>
>The chance that Sun actually changed any of the interfaces from one
>version of TIRPC to the next is virtually nil. Compatibility is a
>huge thing to them. Any changes would most likely have been made
>in a backwards compatible fashion.
>
>
>And no, there are no differences in the over the wire protocol.
>
> =

>
>>>Getting Linux distributions to ship (the same) version of TI-RPC is a =

>>>worthwhile goal, but ideally, we should have an upstream project that =

>>>covers not only Linux, but also, *BSD, OpenSolaris etc. I know it's not =

>>>going to be easy, but I think it's worth a try.
>>> =

>>> =

>>>
>>Well if we having support thats needed and/or wanted it should not
>>be too difficult to get people to move over...
>> =

>>
>
>Having such a project would be a good thing. We would need developers
>for each release platform targeted though. Perhaps it might be sufficient
>to start with platforms that we are interested in and as people with other
>platforms become interested, they can port the support and add their own
>patches to the central source base.
>
>I don't think that I understand why being bug for bug compatible with
>the current glibc code is interesting. This is a new version of the
>RPC support and applications will explicitly need to be ported to it.
>Perhaps we could even pleasantly surprise some folks by addressing
>some particular pet bug that they might have encountered.
>
> Thanx...
>
> ps
>
>-------------------------------------------------------------------------
>This SF.net email is sponsored by DB2 Express
>Download DB2 Express C - the FREE version of DB2 express and take
>control of your XML. No limits. Just data. Click to get it now.
>http://sourceforge.net/powerbar/db2/
>_______________________________________________
>NFS maillist - [email protected]
>https://lists.sourceforge.net/lists/listinfo/nfs
>
> =

>


-- =


********************************
Aurelien Charbon
Linux NFSv4 team
Bull SAS
Echirolles - France
http://nfsv4.bullopensource.org/
********************************


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-05-04 18:52:33

by Steve Dickson

[permalink] [raw]
Subject: Re: Portmap - was Re: Does mountd/statd really need to listen on a privileged port??

The following commits are on:
git://git.infradead.org/~steved/libtirpc.git

Olaf Kirch wrote:
>>> - it seems that tirpc unconditionally requires rpcbind,
>>> even for applications that still use pmap_set and
>>> friends. In the interest of a smooth transition, applications
>>> using the old interface should be able to expect the
>>> old behavior.
>> I not understanding your point... all the pmap_xxx routine are
>> supported, taking the same arguments and return same value.
>> Internally they definitely do things differently, which should
>> not be a problem...so where are you seeing the potential
>> transition problems?
>
> It means you cannot run a binary linked against tirpc
> on a host that runs portmap but no rpcbind. E.g.
> pmap_set will simply fail if it can't connect to
> rpcbind via AF_LOCAL. I think any replacement for
> libc's sunrpc code should continue to work in a
> portmap environment.
I'm still working on this... but I am wondering why this
would be important.. why look back?

>
>>> - tirpc does truly bizarre things, eg in clnt_raw.c
>>> it declares a local variable, and then acquires
>>> a mutex to check this local variable for NULL:
>>>
>>> mutex_lock(&clntraw_lock);
>>> if (clp == NULL) {
>>> mutex_unlock(&clntraw_lock);
>>> return (RPC_FAILED);
>>> }
>>> mutex_unlock(&clntraw_lock);
>> Look again... clp is pointing to global data... so locks
>> are needed...
>
> Yes, clp is *pointing* at global data, so the data it
> points at may change. But this pointer itself is
> a variable on your local stack which is not even
> visible to any other thread; so it can't change.
>
> I assume this was once meant to protect access
> to the clntraw_private pointer, which is a global
> variable and could do with some mutex protection,
> and I guess at some point the code looked like this:
>
> mutex_lock()
> clp = clntraw_private;
> if (clp == NULL) {
> unlock and fail
> }
>
> and some misguided soul probably moved the
> pointer assignment out of the protected region
> so that the code now looks the way it does.
> This needs fixing.
commit 419d35db75ab8bd8f79c424f529a6c2f7c4f5fa7
Author: Steve Dickson <[email protected]>
Date: Fri May 4 09:27:00 2007 -0400

Fixed mutex locking problem in clnt_raw.c. One should grab the
clntraw_lock before accessing at clntraw_private, not after.

Signed-off-by: Steve Dickson <[email protected]>


>
>>> - glibc switched to poll(), whereas tirpc still uses
>>> select with all its limitations
>> educate me... why is using poll() better than select()?
>
> fd_set has a limit on the max file descriptor it can
> accomodate (1024 I think). poll does not. So if you use
> that code in apps that have files with descriptors > 1024
> you will start clobbering heap or stack memory.
>
> Under the hood, they're all the same - glibc's select
> will call sys_poll.
still looking into this... I need to port to some applications
to test this out...

>
>>> - in tirpc, clnt_sperror uses snprintf to avoid buffer overflows,
>>> but gets it all wrong wrt the return value of snprintf.
>> hmm... how is:
>> i = snprintf(str, len, "; errno = %s", strerror(e.re_errno));
>> if (i > 0) {
>> str += i;
>> len -= i;
>> }
>> wrong? looks reasonable to me... what am I missing?
>
> snprintf returns the number of characters it *would* have
> printed if the buffer size had been sufficient. It does not
> return the number of characters actually printed, which is
> what this code seems to assume. So if the buffer is
> too small, you end up with str pointing beyond the end of
> the buffer, and len a huge unsigned value. That is decidedly
> not what you want :-)
commit a3a3a4e5157932c254200e3b31a78739f5878071
Author: Steve Dickson <[email protected]>
Date: Fri May 4 11:27:43 2007 -0400

Ignore the return value of snprintf() and use strlen() instead
to bump the pointer in clnt_sperror()

Also removed calls to assert(), not needed.

Signed-off-by: Steve Dickson <[email protected]>

>>> - tirpc bindresvport is IPv6 aware, but has a bug: if the
>>> caller passes a sockaddr with sin_port/sin6_port set,
>>> the routine will actually bind to htons(port).
>> I must be missing something... in the AF_INET6: the port is set
>> to sin6->sin6_port; which will be the first port that is tried... right?
>> and that is bad?
>
> It uses htons alright, but it forgot the ntohs
>
> switch (af) {
> case AF_INET:
> sin = (struct sockaddr_in *)sa;
> salen = sizeof(struct sockaddr_in);
> port = sin->sin_port; /* <--- needs ntohs */
> ...
> *portp = htons(port++);
commit 83cb8b02f87fe6fd7bbd903e4825f7cb38e59ec4
Author: Steve Dickson <[email protected]>
Date: Fri May 4 12:19:27 2007 -0400

A couple ntohs() were needed in bindresvport_sa()

Signed-off-by: Steve Dickson <[email protected]>

>
> Two more things:
>
> - tirpc does a getenv("NETNAME") which should probably be
> secure_getenv().
Cound not find where secure_getenv() is defined and it didn't
seem to be used in any of the glibc code, but getenv() is...

>
> - in libc, clntudp_call uses the IP_RECVERR feature of the
> Linux kernel, so it gets notified of ICMP errors (like
> port_unreach). The TIRPC code does not, so the
> application will never see any error and has to wait
> for a timeout instead.
commit 40ab0c28e995786d5844bd490a31b788ecabf546
Author: Steve Dickson <[email protected]>
Date: Fri May 4 14:26:56 2007 -0400

Added IP_RECVERR processing with to clnt_dg_call() so
application will see errors instead of timing out

Signed-off-by: Steve Dickson <[email protected]>


>
> That's where we disagree :-) I think ripping out the glibc
> code and replacing it will cause a lot of maintenance for
> the distros.
Don't worry... that glibc code is going anywhere... but
when IPv6 supported is need, I'm hopeful the Linux version
of libtirpc will be an option...

steved.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs