> On Feb 6, 2018, at 11:37 AM, Steve Dickson <SteveD@xxxxxxxxxx> wrote: > > > > On 02/05/2018 02:47 PM, Chuck Lever wrote: >> Hi Steve- >> >>> On Feb 5, 2018, at 2:21 PM, Steve Dickson <SteveD@xxxxxxxxxx> wrote: >>> >>> >>> >>> On 02/05/2018 12:02 PM, Chuck Lever wrote: >>>> Heya Steve- >>>> >>>>> On Feb 5, 2018, at 11:36 AM, Steve Dickson <steved@xxxxxxxxxx> wrote: >>>>> >>>>> Over the weekend I did some experimenting with >>>>> the remote call code in rpcbind. The code does >>>>> functionally work but is very antiquated when >>>>> it comes to the latest NFS versions. >>>>> >>>>> Since only UDP sockets are used to do remote calls >>>>> using the documented interfaces pmap_rmtcall() and callrpc() >>>>> calls to NFS will fail (actual times out) since UDP is no >>>>> longer supported. >>>>> >>>>> The undocumented interface rpc_call() can be used to >>>>> call into NFS since the protocol can specified, which >>>>> also means the PMAPPROC_CALLIT protocol is not used. >>>>> >>>>> It turns out privilege port are not needed to make >>>>> remote calls, at least with my testing. >>>> >>>> It's not quite clear what you are claiming here, but >>>> I'm guessing that what you demonstrated is that the >>>> CALLIT _listener_ does not have to be privileged? >>> Right... >>>> >>>> I claim that is true for all RPC listeners. >>> It could be true.. >> >> "privileged port" always means a source port, never a >> listener. There is no extra privilege given to a listener >> port that is below 1024. >> >> svc_tli_create is simply wrong to use bindresvport. >> >> >>>>> I'm thinking >>>>> the only reason privilege ports were being uses was >>>>> a side effect of create_rmtcall_fd() calling >>>>> svc_tli_create() with an unbound socket. >>>> >>>> Privileged listener ports are being created because >>>> svc_tli_create is using bindresvport when the passed >>>> in socket is not already bound. >>> Right... So handing svc_tli_create a bound socket will >>> cause the bindresvport not to happen. >> >> True, and your patch corrects the behavior of rpcbind. >> >> But I argue that is a workaround: svc_tli_create is >> still doing the wrong thing for any other RPC server >> that does not hand in a pre-bound socket. Fix it there >> and all RPC servers that use libtirpc are fixed. >> >> >>>> svc_tli_create should use bind instead, and it needs >>>> to choose a port higher than 49151.Actual it does when a t_bind structure is passed in. >>> But more to the point, I thought about changing >>> svc_tli_create but that would effect all the callers >>> of the routine which I didn't think was a good idea >>> for code that is basically not used. >> >> I'm not sure what you mean by "code that is basically >> not used." >> >> svc_tli_create is the bottom of the RPC stack. It is the >> libtirpc API that is used every time an RPC server starts >> a listener. mountd and statd call svc_tli_create >> directly for example, from support/nfs/svc_create.c. >> >> Thus this is the right place to fix it for all RPC server >> applications. > Actually I believe those application get their ports from /etc/rpc > via the getrpcbyname() and then hands svc_tli_create a > bound socket which the reason bindresvport is not done. > > Side Note: /etc/rpc will be staying in glibc since it is > "backed by the NSS framework" > > >> >> >>> Also, now that libtirpc is the only Linux RPC implementation >>> (the RPC code has been removed from glibc) I'm a bit >>> sensitive to changing functionality unless its a >>> clear bug or security issue. >> >> This is a clear bug. > Lets do this... Please open up a upstream bz that justifies > why the bindresvport was removed from svc_tli_create(). > > If we break some legacy app I would like something to > point to explaining why the change was made I've opened a bug: https://bugzilla.linux-nfs.org/show_bug.cgi?id=320 >> - The library should not be allowed to choose ports that >> interfere with well-known services. >> >> - The library should choose service ports based on well- >> established standards, such as the IANA service names to >> port numbers standard. >> >> >>>> https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml >>> Hmm... I have been conveniently ignoring Thorsten's blacklist patch. >>> Maybe we should take another look at that. >>> >>>> >>>> >>>>> So the following patch simply binds the socket >>>>> before calling svc_tli_create() which means a >>>>> non-privilege port will be reserved for remote >>>>> calls. >>>>> >>>>> I'm thinking this is the simplest way to >>>>> not pollute the privilege port space. >>>> >>>> This is going in the right direction, but the problem >>>> needs to be addressed in svc_tli_create, not in each >>>> application that calls svc_tli_create. >>>> >>>> This is the same issue that Guillem Jover was trying to >>>> address by making bindresvport skip well-known ports. >>>> >>>> In other words: this code in src/svc_generic.c is wrong: >>> It could be... but the API allows for the bindresvport (or any bind) >>> to not happen... So changing how it is called I think is >>> appropriate... Plus its been working this way for a long to so >>> I'm very resistant to change functionality basically for no reason. >>> >>> But I do think we should look into bindresvport using a black list >>> since it is already established in SuSE >> >> IMO a bindresvport blacklist is absolutely the wrong approach. >> We have standards organizations that have explained exactly how >> this needs to work, and we want it to work without installing >> extra files or giving admins more knobs that they really don't >> need. >> >> This is exactly the problem with distros applying patches >> without upstream review. SuSE fixed this and let their fix sit >> for years without any public bug report or review. IMO SuSE >> should be prepared to take this hit, it should not constrain >> upstream to choose a non-optimal long-term solution. I don't >> see any compelling reason in this particular case that we need >> to be constrained. >> >> Fixing svc_tli_create to use ports higher than 49151 should be >> completely compatible with any reasonable blacklist, and it >> MUST be compatible with /etc/services, which is based on the >> IANA publication (no well-known fixed ports above 49151). >> > Let me look into this... > > steved. > -- > 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 -- 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
