rpc.mountd crashes when extensively using netgroups

We do this on a much larger scale though. The bug we ran into is
in line 96 in utils/mountd/auth.c. The strcpy can corrupt
memory when it copies the string returned by client_compose() to
my_client.m_hostname which has a fixed size of 1024 bytes. For our example above, client_compose() returns "@joe,@jane" for any machine in the offices_1 netgroup. Unfortunately we have
a machine to which roughly 150 netgroups like @joe or @jane
export to and client_compose() returns a string over 1300 bytes
long and rpc.mountd nicely segfaults.

To prevent the crash is of course trivial: Inserting a simple 'if (strlen(n) > 1024) return NULL;' before line 96 does the job.

There are however two issues for which we could not find an easy solution:

1. For every client rpc.mountd and the kernel seem to exchange and use lists with _all_ netgroups used in exports that are relevant for granting permission to some share for a particular client. We could imagine two optimizations here:

* Resolve netgroups and only put the (member) netgroups that contained the host name that would be used to authorize a mount in the list.

* Use the list of mounted paths per client and only put the netgroup(s) used to export paths that are actually mounted on a client.

2. Using a fixed size for NFSCLNT_IDMAX does not scale. Mounting shares on a client for which the 'if' clause of the quick fix becomes true will not be possible. We thought about enlarging NFSCLNT_IDMAX and using a custom kernel but dropped the idea.

Our ultimate goal is to get Red Hat fix the code in nfs-utils 1.0.6 that is used in RHEL4. A first step would be to get a suitable fix in the current nfs-utils.

Quoted 11 lines · Stefan Walter

We do this on a much larger scale though. The bug we ran into is in line 96 in utils/mountd/auth.c. The strcpy can corrupt memory when it copies the string returned by client_compose() to my_client.m_hostname which has a fixed size of 1024 bytes. For our example above, client_compose() returns "@joe,@jane" for any machine in the offices_1 netgroup. Unfortunately we have a machine to which roughly 150 netgroups like @joe or @jane export to and client_compose() returns a string over 1300 bytes long and rpc.mountd nicely segfaults.

Quoted 2 lines

To prevent the crash is of course trivial: Inserting a simple 'if (strlen(n) > 1024) return NULL;' before line 96 does the job.

Does the attached patch help?

Quoted 4 lines

Our ultimate goal is to get Red Hat fix the code in nfs-utils 1.0.6 that is used in RHEL4. A first step would be to get a suitable fix in the current nfs-utils.

Please open up bugs on all three of these issues and we'll see what can done...

Quoted 9 lines · Steve Dickson

Quoted 8 lines · Stefan Walter

We do this on a much larger scale though. The bug we ran into is in line 96 in utils/mountd/auth.c. The strcpy can corrupt memory when it copies the string returned by client_compose() to my_client.m_hostname which has a fixed size of 1024 bytes. For our example above, client_compose() returns "@joe,@jane" for any machine in the offices_1 netgroup. Unfortunately we have a machine to which roughly 150 netgroups like @joe or @jane

Quoted 7 lines

export to and client_compose() returns a string over 1300 bytes

Quoted 4 lines

long and rpc.mountd nicely segfaults.

To prevent the crash is of course trivial: Inserting a simple 'if (strlen(n) > 1024) return NULL;' before line 96 does the job.

Does the attached patch help?

rpc.mountd does not crash anymore but I get a 'permission denied' when trying
to mount a share. Doing an 'strace rpc.mountd -F' reveals:

...
open("/proc/net/rpc/auth.unix.ip/channel", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 9
fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0 mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0xb7f41000
time(NULL) = 1186041882
write(9, "nfsd 129.132.10.33 1186043682 @a"..., 1024) = -1 EINVAL (Invalid argument)

write(9, "\n", 1)                       = -1 EINVAL (Invalid argument)
close(9)                                = 0
munmap(0xb7f41000, 4096)                = 0

open("/proc/net/rpc/nfsd.export/channel", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 9
fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0 mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0xb7f41000
write(9, "@anbuehle,@anhorni,@antoinet,@ap"..., 1024) = -1 EINVAL (Invalid argument)
time(NULL) = 1186041882
write(9, "/export/groups/grossm/h1/home/gr"..., 68) = -1 ENOENT (No such file or directory)
close(9) = 0

munmap(0xb7f41000, 4096) = 0
open("/proc/fs/nfsd/filehandle", O_RDWR) = 9
fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0 mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0xb7f41000
write(9, "@anbuehle,@anhorni,@antoinet,@ap"..., 1066) = -1 EPERM (Operation not permitted)
...

Hi,

Quoted 43 lines · Stefan Walter

Quoted 8 lines · Steve Dickson

Quoted 7 lines · Stefan Walter

We do this on a much larger scale though. The bug we ran into is in line 96 in utils/mountd/auth.c. The strcpy can corrupt memory when it copies the string returned by client_compose() to my_client.m_hostname which has a fixed size of 1024 bytes. For our example above, client_compose() returns "@joe,@jane" for any machine in the offices_1 netgroup. Unfortunately we have a machine to which roughly 150 netgroups like @joe or @jane

Quoted 7 lines

export to and client_compose() returns a string over 1300 bytes

Quoted 4 lines

long and rpc.mountd nicely segfaults.

To prevent the crash is of course trivial: Inserting a simple 'if (strlen(n) > 1024) return NULL;' before line 96 does the job.

Does the attached patch help?

rpc.mountd does not crash anymore but I get a 'permission denied' when trying
to mount a share. Doing an 'strace rpc.mountd -F' reveals:

...
open("/proc/net/rpc/auth.unix.ip/channel", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 9
fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0 mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0xb7f41000
time(NULL) = 1186041882
write(9, "nfsd 129.132.10.33 1186043682 @a"..., 1024) = -1 EINVAL (Invalid argument)

write(9, "\n", 1)                       = -1 EINVAL (Invalid argument)
close(9)                                = 0
munmap(0xb7f41000, 4096)                = 0

open("/proc/net/rpc/nfsd.export/channel", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 9
fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0 mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0xb7f41000
write(9, "@anbuehle,@anhorni,@antoinet,@ap"..., 1024) = -1 EINVAL (Invalid argument)
time(NULL) = 1186041882
write(9, "/export/groups/grossm/h1/home/gr"..., 68) = -1 ENOENT (No such file or directory)
close(9) = 0

Quoted 8 lines

munmap(0xb7f41000, 4096) = 0
open("/proc/fs/nfsd/filehandle", O_RDWR) = 9
fstat64(9, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0 mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0) = 0xb7f41000
write(9, "@anbuehle,@anhorni,@antoinet,@ap"..., 1066) = -1 EPERM (Operation not permitted)
...

Yup, the snprintf() in the patch would've truncated the input string.

Steve (D), you should check the return of snprintf() and compare against the size specified (NFSCLNT_IDMAX+1) and do a graceful cleanup + print an error message to the user, when detecting truncation of input:

err = snprintf(my_client.m_hostname, (NFSCLNT_IDMAX+1), "%s", *n?n:"DEFAULT");

if (err >= (NFSCLNT_IDMAX+1)) {
	printf("too large input string ...\n");
	/* cleanups and graceful exit */
}

There are however two issues for which we could not find an easy solution:

1. For every client rpc.mountd and the kernel seem to exchange and use lists with _all_ netgroups used in exports that are relevant for granting permission to some share for a particular client. We could imagine two optimizations here:

* Resolve netgroups and only put the (member) netgroups that contained the host name that would be used to authorize a mount in the list.

* Use the list of mounted paths per client and only put the netgroup(s) used to export paths that are actually mounted on a client.

This also caused us severe performance problems because rpc.mountd queries all these netgroups. We were initially using a LDAP and mouting a directory took up to ten seconds during which rpc.mountd was busily querying the LDAP server. We got this down to two seconds using file based netgroups.

2. Using a fixed size for NFSCLNT_IDMAX does not scale. Mounting shares on a client for which the 'if' clause of the quick fix becomes true will not be possible. We thought about enlarging NFSCLNT_IDMAX and using a custom kernel but dropped the idea.

Our ultimate goal is to get Red Hat fix the code in nfs-utils 1.0.6 that is used in RHEL4. A first step would be to get a suitable fix in the current nfs-utils.

Quoted 22 lines · Stefan Walter

There are however two issues for which we could not find an easy solution:

1. For every client rpc.mountd and the kernel seem to exchange and use lists with _all_ netgroups used in exports that are relevant for granting permission to some share for a particular client. We could imagine two optimizations here:

* Resolve netgroups and only put the (member) netgroups that contained the host name that would be used to authorize a mount in the list.

* Use the list of mounted paths per client and only put the netgroup(s) used to export paths that are actually mounted on a client.

This also caused us severe performance problems because rpc.mountd queries all these netgroups. We were initially using a LDAP and mouting a directory took up to ten seconds during which rpc.mountd was busily querying the LDAP server. We got this down to two seconds using file based netgroups.

Quoted 8 lines

2. Using a fixed size for NFSCLNT_IDMAX does not scale. Mounting shares on a client for which the 'if' clause of the quick fix becomes true will not be possible. We thought about enlarging NFSCLNT_IDMAX and using a custom kernel but dropped the idea.

Our ultimate goal is to get Red Hat fix the code in nfs-utils 1.0.6 that is used in RHEL4. A first step would be to get a suitable fix in the current nfs-utils.

That's an interesting problem. Thanks for the report!

I don't believe that long comma-delimited string actually has any meaning to the kernel--as far as the kernel is concerned, it's just an opaque object that will be passed back to mountd later (along with a path name) to get export options.

So I suppose that string could be replaced by a hash, or maybe even just by the ip address of the particular host--the disadvantage to the latter being that it would require the kernel to keep a separate export for each client address.

--b.

This issue with rpc.mountd crashing sounds familiar. It may be the same as RHBZ 242465 (which is unfortunately marked private so I can't really refer you to it). If it is the same, then there is a fix slated for inclusion in RHEL 4.6.

If you have a non-critical place to do so, I'd suggest testing the RHEL4 nfs-utils packages available on my people page:

http://people.redhat.com/jlayton

Quoted 22 lines · Stefan Walter

There are however two issues for which we could not find an easy solution:

1. For every client rpc.mountd and the kernel seem to exchange and use lists with _all_ netgroups used in exports that are relevant for granting permission to some share for a particular client. We could imagine two optimizations here:

* Resolve netgroups and only put the (member) netgroups that contained the host name that would be used to authorize a mount in the list.

* Use the list of mounted paths per client and only put the netgroup(s) used to export paths that are actually mounted on a client.

This also caused us severe performance problems because rpc.mountd queries all these netgroups. We were initially using a LDAP and mouting a directory took up to ten seconds during which rpc.mountd was busily querying the LDAP server. We got this down to two seconds using file based netgroups.

Quoted 8 lines

2. Using a fixed size for NFSCLNT_IDMAX does not scale. Mounting shares on a client for which the 'if' clause of the quick fix becomes true will not be possible. We thought about enlarging NFSCLNT_IDMAX and using a custom kernel but dropped the idea.

Our ultimate goal is to get Red Hat fix the code in nfs-utils 1.0.6 that is used in RHEL4. A first step would be to get a suitable fix in the current nfs-utils.

That's an interesting problem. Thanks for the report!

I don't believe that long comma-delimited string actually has any meaning to the kernel--as far as the kernel is concerned, it's just an opaque object that will be passed back to mountd later (along with a path name) to get export options.

So I suppose that string could be replaced by a hash, or maybe even just by the ip address of the particular host--the disadvantage to the latter being that it would require the kernel to keep a separate export for each client address.

--b.

Quoted 11 lines · On Tue, 31 Jul 2007 10:48:24 -0400 "J. Bruce Fields" <[email protected]>…

That's an interesting problem. Thanks for the report!

I don't believe that long comma-delimited string actually has any meaning to the kernel--as far as the kernel is concerned, it's just an opaque object that will be passed back to mountd later (along with a path name) to get export options.

So I suppose that string could be replaced by a hash, or maybe even just by the ip address of the particular host--the disadvantage to the latter being that it would require the kernel to keep a separate export for each client address.

I started having a look at this today. The original patches that I proposed to clean up the rmtab a few months ago also eliminated this comma-delimited string. Neil had valid objections to it at the time, but if we switched to using the IP address as a cache key like Bruce describes then doing that becomes more reasonable.

The only downside I see is the one Bruce points out -- the size of the kernel export cache would increase. I don't have a feel for whether this is a show stopper, however.

Quoted 20 lines · Jeff Layton

Quoted 11 lines · On Tue, 31 Jul 2007 10:48:24 -0400 "J. Bruce Fields" <[email protected]>…

That's an interesting problem. Thanks for the report!

I don't believe that long comma-delimited string actually has any meaning to the kernel--as far as the kernel is concerned, it's just an opaque object that will be passed back to mountd later (along with a path name) to get export options.

So I suppose that string could be replaced by a hash, or maybe even just by the ip address of the particular host--the disadvantage to the latter being that it would require the kernel to keep a separate export for each client address.

I started having a look at this today. The original patches that I proposed to clean up the rmtab a few months ago also eliminated this comma-delimited string. Neil had valid objections to it at the time, but if we switched to using the IP address as a cache key like Bruce describes then doing that becomes more reasonable.

Quoted 3 lines

The only downside I see is the one Bruce points out -- the size of the kernel export cache would increase. I don't have a feel for whether this is a show stopper, however.

Yeah, there might be some risk of solving that problem at the expense of people with tons of clients all matching *.example.com. The actual export objects are actually pretty small--68 bytes, last I checked? You'd also need two upcalls to mountd per client (for ip address and export, as opposed to just ip address). I don't know what other costs there might be.

What about the idea of hashing the comma-delimited list and passing that? You'd want hash large enough to be collision-free--might as well use some cryptographic hash, I guess, though it may be mild overkill. Is there any reason why that wouldn't work?

Could you remind me what problems you were trying to fix with your rmtab cleanup?

I started having a look at this today. The original patches that I proposed to clean up the rmtab a few months ago also eliminated this comma-delimited string. Neil had valid objections to it at the time, but if we switched to using the IP address as a cache key like Bruce describes then doing that becomes more reasonable.

The only downside I see is the one Bruce points out -- the size of the kernel export cache would increase. I don't have a feel for whether this is a show stopper, however.

Neil, do you have thoughts on what you'd consider a reasonable approach to fixing this?

I'm having troubling visualising the problematic setup. Is it possible to get a copy of the /etc/exports file, and some idea of what hosts are in which netgroups? Knowing that would help assess the appropriateness of various solutions.

Quoted 7 lines · On Thursday August 2, [email protected] wrote:

I started having a look at this today. The original patches that I proposed to clean up the rmtab a few months ago also eliminated this comma-delimited string. Neil had valid objections to it at the time, but if we switched to using the IP address as a cache key like Bruce describes then doing that becomes more reasonable.

Quoted 7 lines

The only downside I see is the one Bruce points out -- the size of the kernel export cache would increase. I don't have a feel for whether this is a show stopper, however.

Neil, do you have thoughts on what you'd consider a reasonable approach to fixing this?

I'm having troubling visualising the problematic setup. Is it possible to get a copy of the /etc/exports file, and some idea of what hosts are in which netgroups? Knowing that would help assess the appropriateness of various solutions.

The core issue is this:

We need to map IPADDRESS to THING, and THING + PATH to EXPORTPOINT.
(roughly).
So what do we choose for "THING"?

One obvious option is the dotted quad of the IP address. People tend to have large sets of similar clients, so doing this seems to be missing out on a valuable optimisation and adding bloat to the lookup tables in the kernel.

Another apparently obvious solution is to use FQDN, but we really don't want to do that, as a multihomed host might have one IP address that you want to trust, and another that you don't (as the subnet is less secure against forgery). It doesn't help must over dotted-quad anyway.

Another option is to use whatever strings are included in /etc/exports to identify things. e.g. *.my.domain or @mynetgroup.
This seems simple and elegant, but it can be messy in some situations. In particular, if one IP address matches several strings, which one do you use? We have to use them all.
One path might be exported to *.my.domain and a different path might be exported to @mynetgroup. If a.b.c.d is in both of those sets of machines, then the THING that a.b.c.d is mapped to must allow the correct

    THING + PATH -> EXPORTPOINT

mapping for both paths.
*.my.domain,@mynetgroup
works quite effectively.
But if you have lots and lots of different netgroups mentioned in /etc/exports, and one machine is in all of those netgroups, then you have to generate a very long string @a,@b,@c,@d,.....

I guess we could use a hash and keep a hash table in mountd. So mountd:
1/ Generates the comma-separated name as needed, being careful to
allocate enough space.
2a/ If this is small enough, write it to the kernel as is. 2b/ If it is too big, Find it in a table (adding if needed) and
write the address of the table entry to the kernel

When a THING + PATH request arrives:
If THING looks like an address in the table, do the lookup to get the string.

This would work to keep the strings in the kernel shorter, but is rather ugly - storing in the kernel addresses in a user-space program.

Also, if you have lots of netgroups mentioned, then finding out which ones contain a given IP address might be slow, and all the information isn't really needed. Once you get THING, it will be paired with a specific path, so you only really need to look up netgroups that are related to that path.

So maybe we want to combine the two workable approaches.

Sometimes IPADDRESS maps to DOTTED_QUAD
Sometimes IPADDRESS maps to LIST,OF,STRINGS,FROM,ETC,EXPORTS

Possibly the choice could be based on a command line switch. In the absence of such a switch, it could be based on the number of entries in /etc/exports.

	int i;
	int size = 0;
	nfs_client	*clp;

	for (i = 0 ; i < MCL_MAXTYPES; i++)
		for (clp = clientlist[i]; clp ; clp = clp->m_next)
			size += strlen(clp->m_hostname)+1;

	if (size > 1000)
		use DOTTED_QUAD;
	els
		don't.

The core issue is this:

We need to map IPADDRESS to THING, and THING + PATH to EXPORTPOINT.
(roughly).
So what do we choose for "THING"?

One obvious option is the dotted quad of the IP address. People tend to have large sets of similar clients, so doing this seems to be missing out on a valuable optimisation and adding bloat to the lookup tables in the kernel.

I guess we could use a hash and keep a hash table in mountd. So mountd:
1/ Generates the comma-separated name as needed, being careful to
allocate enough space.
2a/ If this is small enough, write it to the kernel as is. 2b/ If it is too big, Find it in a table (adding if needed) and
write the address of the table entry to the kernel

When a THING + PATH request arrives:
If THING looks like an address in the table, do the lookup to get the string.

This would work to keep the strings in the kernel shorter, but is rather ugly - storing in the kernel addresses in a user-space program.

So maybe we want to combine the two workable approaches.

Sometimes IPADDRESS maps to DOTTED_QUAD
Sometimes IPADDRESS maps to LIST,OF,STRINGS,FROM,ETC,EXPORTS

Possibly the choice could be based on a command line switch. In the absence of such a switch, it could be based on the number of entries in /etc/exports.