> On Apr 9, 2018, at 2:04 PM, Steve Dickson <SteveD@xxxxxxxxxx> wrote:
>
>
>
> On 04/09/2018 02:40 PM, Chuck Lever wrote:
>>
>>
>>> On Apr 9, 2018, at 11:58 AM, Steve Dickson <SteveD@xxxxxxxxxx> wrote:
>>>
>>>
>>>
>>> On 04/09/2018 01:32 PM, Chuck Lever wrote:
>>>>
>>>>
>>>>> On Apr 9, 2018, at 11:28 AM, Steve Dickson <steved@xxxxxxxxxx> wrote:
>>>>>
>>>>> Commit 46e04a73 changed clnt_com_create() to avoid
>>>>> using reserved ports when creating the the CLIENT ptr.
>>>>> This change breaks backward compatibility with the
>>>>> legacy RPC code that was in glibc.
>>>>>
>>>>> This patch reverts that commit to restore backwards compatibility
>>>>>
>>>>> Signed-off-by: Steve Dickson <steved@xxxxxxxxxx>
>>>>> ---
>>>>> src/rpc_soc.c | 10 ++++++----
>>>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/src/rpc_soc.c b/src/rpc_soc.c
>>>>> index af6c482..ed0892a 100644
>>>>> --- a/src/rpc_soc.c
>>>>> +++ b/src/rpc_soc.c
>>>>> @@ -67,8 +67,6 @@
>>>>>
>>>>> extern mutex_t rpcsoc_lock;
>>>>>
>>>>> -extern int __binddynport(int fd);
>>>>> -
>>>>> static CLIENT *clnt_com_create(struct sockaddr_in *, rpcprog_t, rpcvers_t,
>>>>> int *, u_int, u_int, char *, int);
>>>>> static SVCXPRT *svc_com_create(int, u_int, u_int, char *);
>>>>> @@ -147,8 +145,7 @@ clnt_com_create(raddr, prog, vers, sockp, sendsz, recvsz, tp, flags)
>>>>> bindaddr.maxlen = bindaddr.len = sizeof (struct sockaddr_in);
>>>>> bindaddr.buf = raddr;
>>>>>
>>>>> - if (__binddynport(fd) == -1)
>>>>> - goto err;
>>>>> + bindresvport(fd, NULL);
>>>>> cl = clnt_tli_create(fd, nconf, &bindaddr, prog, vers,
>>>>> sendsz, recvsz);
>>>>> if (cl) {
>>>>> @@ -316,6 +313,7 @@ svc_com_create(fd, sendsize, recvsize, netid)
>>>>> SVCXPRT *svc;
>>>>> int madefd = FALSE;
>>>>> int port;
>>>>> + struct sockaddr_in sin;
>>>>>
>>>>> if ((nconf = __rpc_getconfip(netid)) == NULL) {
>>>>> (void) syslog(LOG_ERR, "Could not get %s transport", netid);
>>>>> @@ -332,6 +330,10 @@ svc_com_create(fd, sendsize, recvsize, netid)
>>>>> madefd = TRUE;
>>>>> }
>>>>>
>>>>> + memset(&sin, 0, sizeof sin);
>>>>> + sin.sin_family = AF_INET;
>>>>> + bindresvport(fd, &sin);
>>>>> + listen(fd, SOMAXCONN);
>>>>
>>>> Why do we need to fix the server API too?
>>> I thought about this... and I'm thinking there is
>>> more of an exception of server listening on reserve
>>> ports than clients using using reserve ports.
>>
>> Hi Steve-
>>
>> I think you mean that there are fewer legacy servers
>> than there are legacy clients? I don't see how that
>> matters: There's no benefit at all for having a
>> server listen on a dynamically-assigned privileged
>> port. Nothing can be broken if the server API uses
>> an ephemeral port.
>>
>>
>>> Plus this make us completely backwards compatibility
>>> which in the long run I think is the right place to be.
>>
>> Philosophically I agree that backwards compatibility
>> is good, but I think you're "going to hell with the
>> joke" as they say.
>>
>> The point of backwards compatibility is that we don't
>> want to break applications that depend on some behavior.
>> There can't be any applications that depend on this
>> behavior because there's no functional benefit to it,
>> only a downside, which we have no reason not to fix.
> The downside is we are changing an API that has
> been established for since the 80's... What good
> could that possibly do WRT legacy servers?
That's the wrong question, IMO. It won't hurt legacy
servers at all, and will benefit everyone else.
>> I'm going to firmly object on this one, unless you have
>> a clearly documented breakage that requires the legacy
>> server API to use bindresvport(3).
> Here is what the man page says "If the socket is not bound to a
> local TCP port, then this routine binds it to an arbitrary port."
>
> The point being it also does not talk about creating a
> listening connection either... Changing the (undocumented)
> API like this can cause nothing but problems... IMHO...
Please show me one specific breakage that will result
in removing bindresvport from svc_com_create. Just one,
and I promise I will crawl back into my hole.
In case it wasn't clear, my preference is for a patch
that reverts only the removal of bindresvport(3) from
clnt_com_create. The svc_com_create change is safe and
should remain unreverted.
I don't like reverting the clnt_com_create change, but
I won't object to it, and there is clear evidence that
some old programs are inconvenienced by that change.
> Basically, not making this change will cause a fork with
> all the major distros since it very import for them to be
> backward compatible esp in enterprise worlds.
That sounds like an overstatement. Who claims they won't
take libtirpc with an unprivileged svc_com_create? I would
really like to understand why.
> Upstream not
> so much... Who really uses raw upstream bits in a stable
> environment... understood... But this brings me to my point.
>
> What problem is being fixed by changing an 20+ year API?
The problem is legacy clients and servers are squatting on
ports that are assigned to other network services. These
patches mitigate that problem. There is more to be done.
The backwards compatibility issue is that some old servers
believe that clients have to use a privileged source port
to show that they are an authorized RPC consumer. We address
that by reverting clnt_com_create to use bindresvport(3),
although we can easily decide this is a security bug that
is worth forcing legacy API consumers to fix their usage.
There is no similar backwards compatibility issue for
clients talking to servers. That's just a fact about how
RPC operates: the client uses rpcbind to discover the
server's port. After that, there's no additional check to
see if that service port is less than 1024.
> Where are the bug reports that this change is needed
> or wanted?
I was presented with a list of five or more bug reports
from various distributors that go back to almost 2000
where RPC consumers have caused issues occupying IANA-
assigned reserved ports.
https://sourceforge.net/p/libtirpc/mailman/libtirpc-devel/?viewmonth=201801&viewday=12&style=flat
The bottom item on that page is Guillem's e-mail that
cites bug reports on this issue.
>> Also, it would be great to get a man page update on the
>> legacy clnt API that documents the behavior when a root
>> caller uses that API. That gives some guarantee that it
>> doesn't get changed again inadvertently.
> I need to look, but I think glibc still "owns" the
> man pages...
That should be fixed if that's true. Since RPC is to be
removed from glibc I can't think why they'd want to keep
the RPC man pages.
However, libtirpc has a bunch of man pages for the legacy
APIs. At least those can be updated.
--
Chuck Lever
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
