Fix mountd parsing of user/group info in /etc/exports

[yocalebo_gmail.com]

Summary

mountd incorrectly assumes that the first character of the username or groupname passed via the -maproot or -mapall is the uid or gid. This means, for example, a user named "1test" will be mapped to the uid of 1. The nfs clients will see that user owner of the export path is mapped to the user "daemon" (in this example).

[rmacklem]

I've made a few inline comments.

The only substantial one is the numeric group
case. The rest are stylistic and you can choose
to ignore them, if you prefer what you already have
written.

mountd.c

3538

I don't think the "end == NULL" check is needed and might? confuse
readers of the code. (But it doesn't break anything, of course.)

Also, the "{}" aren't needed for the if/else, which is just a minor
style nit.

3555

Since you've moved this check up. A good change, I think.
Then you can delete it here.

3607

There could be cases where the gid doesn't actually
exist in the group database or the exports line
was meant to create a different list of group numbers.

The old code just assigned the value, so I think the
new code should do the same as line#3588 did in the old code
for the case where it is numeric.

[yocalebo_gmail.com]

Uploading new diff addressing Rick Macklem's review.

[rmacklem]

Here's a couple more suggestions.

mountd.c

3592

I'd do this one as cr->cr_uid = name_ul; as well,
for the same reason as the gid one below.
(And maybe get rid of the "end == NULL" and "{}"
for consistency.)

3596

If you do the above change, this needs to be moved
to just after line #3586 (obvious, but thought I'd
mention it).

3607

You need to invert the conditional or interchange
the code blocks for the if/else.
--> This is the "not a number" case

3615

I'd write this as:

groups[cr->cr_ngroups++] = name_ul;

since you've already done the conversion.

[yocalebo_gmail.com]

mountd.c

3592

To be honest, I've tried to understand why this section is even here because it seems like the cr->cr_uid = pw->pw_uid; on lines 3552 and 3582 can be moved underneath the calls after we do the getpwnam or getpwuid calls on lines 3539 or 3541 respectively?

[yocalebo_gmail.com]

mountd.c

3592

Or is there a situation where a user doesn't exist in the local password database similar to the scenario you said exists for the groups?

[rmacklem]

Replied to question.

mountd.c

3592

Yes, I think one use of numeric uid/gids is that
they do not exist in the password/group database.
(Maybe the main use?)

Also, this code is very old. The first rendition of
it was written by Herb Hasler when he worked
with me in around 1986. Since then, many have
made changes to it.
--> It may not seem logical, but the semantics

 have been wired into it over the decades
 and it appears creating usernames that start
 with a digit is something only you have done
in all those years.

[yocalebo_gmail.com]

mountd.c

3592

Ha, thanks for the insight. I will admit, personally, I do not create users starting with digits. However, given the opportunity (as you're probably well-aware) end users will do whatever the system allows them to do 😄

[yocalebo_gmail.com]

Okay, I've tried to fix the style to be consistent with what freeBSD expects. I've removed the call to cr->cr_uid = atoi(name) because it's a no-op and could never be reached with or without my logic changes. Lastly, I've ensured that if strsep_quote returns NULL that we account for that so we don't crash by passing a NULL value to the various getpw* or getgr calls.

[rmacklem]

Ok, here's some more fixes. Basically, some of your changes need to
be reverted because they change the semantics.

Basically, the "if (names == NULL)" at line#3573 (patched code)
handles the single name/number case and the stuff that uses
the initial acquisition of "pw" all needs to be in there, including
the check for "pw" being NULL.
--> For the ':' list of numbers, it can be NULL because it is

allowed to not be in the password database for the list of
numbers case.

usr.sbin/mountd/mountd.c

3563 ↗

(On Diff #98570)

The code from here (line#3564) to line#3566 needs to
be in the "if (names == NULL)" block that
starts at 3564, It is for the case where there is just
a name/number for the user and not the case where
there is a ':' separated list.

Sorry, I didn't spot this on the first review cycle.

3569 ↗

(On Diff #98570)

Above line#3569 should not be here.
It was set to 1 above and should only
be set to 0 further down, after the
if (names == NULL) block starting
at line#3573.

In case the above is confusing...
delete line# 3569 and move line#3565-3568
to after line#3573 (putting it back the way
it is in the unpatched sources).

3602 ↗

(On Diff #98570)

The code that needs to be here is what is on the left
(old line#3595 to 3603), but you can replace ole line#3598 with

cr->cr_uid = name_ul;

since you've done the conversion, above.

In other words, put it back to the unpatched version,
except use "name_ul" in old line#3598 instead of atoi(name).

3604 ↗

(On Diff #98570)

I don't think the "if (name == NULL)" check is
needed here, since the "while" checks that
both "names has a string left in it.
--> strsep_quote() should always return

a non-NULL pointer.

This refers to line#3605->3608.

[freqlabs]

usr.sbin/mountd/mountd.c

3602 ↗

(On Diff #98570)

What was on the left doesn't make sense anymore because pw can't be NULL here now.

[freqlabs]

usr.sbin/mountd/mountd.c

3602 ↗

(On Diff #98570)

And that makes sense because you can't specify only groups, right? So there must be a user we've found by now.

[freqlabs]

usr.sbin/mountd/mountd.c

3602 ↗

(On Diff #98570)

Ah but then you can't specify a uid that doesn't exist on the server.

[freqlabs]

@rmacklem is it ok if we make parsecred return an int so we can fail to start when the creds are invalid? It feels wrong to merely log an error message and keep going when this is misconfigured.

[rmacklem]

With the one additional change I just commented inline,
I think that will do it.
(Isn't it fun to work with decades old code and still maintain
all the semantics it carries around. Personally, I live in fear
of mountd bugs because the code can be such a beast to
work with.;-)

mountd.c

3586–3597

Getting close. You still need a check here for
"not a numeric string" which generates the error.
You could add that test here by replacing the "else" below with:

    else if (*end != '\0' || end == name) {
                syslog(LOG_ERR, "unknown user: %s", name);
		return;
    } else

[yocalebo_gmail.com]

In D32993#745850, @rmacklem wrote:

With the one additional change I just commented inline,
I think that will do it.
(Isn't it fun to work with decades old code and still maintain
all the semantics it carries around. Personally, I live in fear
of mountd bugs because the code can be such a beast to
work with.;-)

I've made the change as requested. Thanks for being patient with me. I was naive at first, but now I realize why you live in fear of mountd breaking :)

[rmacklem]

It now looks good to me.

Do you have a src commit bit?

If not, I can commit it with "Submitted by: yocalebo_gmail.com".

Thanks for the patch, rick

[yocalebo_gmail.com]

In D32993#746038, @rmacklem wrote:

It now looks good to me.

Do you have a src commit bit?

If not, I can commit it with "Submitted by: yocalebo_gmail.com".

Thanks for the patch, rick

I do not have a source commit bit.