[sshd] add wrapper function of login_getpwclass

[cem]

Summary

This is a fix for PR #231172.
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=231172
login_getpwclass(3) is not allowed in capability mode.
We add a wrapper function to proxy the function.

Test Plan

Successfully login via ssh with sysctl kern.trap_enotcap=1

1. set kern.trap_enotcap=1

$ su root  -c "sysctl kern.trap_enotcap=1"

2. ssh login

$ ssh localhost

3. expect successfully login

[cem]

Hi Yuichiro NAITO,

Thanks for submitting the patch! Would it be possible to regenerate the patch with diff -U9999 and upload that? (-U9999 emits full context for Phabricator.)

Thank you,
Conrad

[naito.yuichiro_gmail.com]

regenerate patch by `diff -U9999```

[naito.yuichiro_gmail.com]

Hi Conrad.
Thanks for the advice. I regenerated patch file. It seems good differential.

[cem]

In D17056#363447, @naito.yuichiro_gmail.com wrote:

Hi Conrad.
Thanks for the advice. I regenerated patch file. It seems good differential.

Thanks!

Including des@ on the review, as he has been most active in maintaining our base sshd.

[cem]

Mostly looks good to me! I had a couple concerns and suggestions, see below. Thanks for the patch.

auth2.c

294

I don't think it is valid to login_close() our PRIVSEP-allocated login_cap_t*.

FreeBSD login_close() function does not only free the strings and struct. (And even if it only did that, the fact that it may change means we must not use it for the privsep-allocated login_cap_t anyway.)

monitor.c

722

Can be sizeof(*pw)

742

Can be sizeof(*lc)

But also I don't think we need to send the struct. See below.

747

Hm. I think it should be after out:, except for lc_class will only be valid here. Is that string critical to the log message?

monitor_wrap.c

251

This can be sizeof(*pwent)

251–261

I have two comments about serialization of struct passwd.

1. It could be moved to a subroutine shared with e.g. mm_answer_pwnamallow. I'd suggest a buffer_put_passwd, but I haven't thought about that name/location very much.

2. (Not introduced in your change.) In general the serialization/deserialization of struct passwd scares me. We send the pointer values across the socket and don't have any seatbelt against e.g. new pointers being added to the struct. I think a more conservative approach might copy the non-pointer fields out to a zeroed struct and send that instead (followed by the serialized pointers, like this). I do not expect you to make this change in this revision.

273–275

login_cap_t is composed only of string pointers — I don't think it makes sense to send the struct at all.

274

Can be sizeof(*lc)

sandbox-capsicum.c

74–75

Maybe spell as caph_cache_tzdata() instead. It does the same thing but shows intent without the comment.

[emaste]

How do we coordinate with upstream on this?

[naito.yuichiro_gmail.com]

auth2.c

294

I see FreeBSD login_close() tries to free globally allocated memory and file descriptor. I will make mm_login_close() that frees just allocated memory by mm_login_getpwclass().

monitor.c

722

I'll fix this.

742

I'll fix this and comment blow.

747

No, lc_class is not critical. I think log the message is more important even if log_getpwsclass(3) failed. I will move the debug3() function after out: and remove lc_class from the message.

monitor_wrap.c

251

I'll fix this.

251–261

I will make new functions to share all struct passwd operations. In these functions, I will get avoid to send pointer values by sending all member values. If a new pointer member is added, it will be NULL in the receive function.

Because mm_answer_pwnamallow has same risk. We can fix both of them.

273–275

Ah, there is no meaning sending pointers. I'll remove the sending login_cap_t code.

274

I'll fix this.

sandbox-capsicum.c

74–75

Indeed. I'll fix this.

[naito.yuichiro_gmail.com]

Fixed commented issues.
If you try this code, please be aware that you need to update libprivatessh.so.
Because I fixed buffer.c to implement functions for operating struct passwd.
Buffer related functions are written in 'buffer.c'.

[naito.yuichiro_gmail.com]

How do we coordinate with upstream on this?

In my opinion, fixes of buffer.[c|h] can be merged to upstream.
Maybe upstream has an interest at sending pointer value issue.
I think mm_login_getpwclass() related fixes are FreeBSD specific.
caph_cache_tzdata() seems to be FreeBSD issue.

[cem]

Looks great to me! All of my comments below are just style or message suggestions, nothing functional.

buffer.c

153 ↗

(On Diff #47788)

The cast of calloc's return is not needed in C (unlike perhaps C++). General C style is to not have explicit cast.

buffer.h

95 ↗

(On Diff #47788)

Probably second argument can be const struct passwd *, right?

monitor.c

723

This message is maybe too specific now, because we may also just be out of memory :-)

monitor_wrap.c

262

Cast is not needed in C :-)

278

I don't know about openssh style. But FreeBSD style suggests comparing pointers against NULL instead of treating them as boolean values. So e.g. if (lc != NULL) {. I don't feel strongly about this — current code is quite clear to me.

311–321

Message may be too specific now.

sandbox-capsicum.c

27–34

Probably sort capsicum_helpers.h with or even after libc headers, rather than before.

[naito.yuichiro_gmail.com]

buffer.c

153 ↗

(On Diff #47788)

Thanks for the comment. I was little confused about this.
I'll fix this.

buffer.h

95 ↗

(On Diff #47788)

Yes. sturct passwd * will never be changed in buffer_put_passwd.
I'll fix this.

monitor.c

723

Ah, yes. I will fix log message.

monitor_wrap.c

262

Yeah. I'll fix too.

278

I referred to source code of login_close(3).

https://svnweb.freebsd.org/base/head/lib/libutil/login_cap.c?view=markup#l155

In this function, if (lc) { is written for checking pointer.
But I trust you and will fix this.

311–321

I'll fix this too.

sandbox-capsicum.c

27–34

I see capsicum_helpers.h refers to libc headers (time.h, unistd.h, etc.).
It seems that capsicum_helpers.h depends on libc headers.
I think it's reasonable sort capsicum_helpers.h after libc headers.
I will fix this.

[naito.yuichiro_gmail.com]

Fixed code style issues and log messages.
No functional change.

[cem]

Looks great to me, thanks. Any other reviewers want to take a pass?

[naito.yuichiro_gmail.com]

I found one problem that sshd fails to reverse resolve hostname if server is set UseDNS yes .

After login_getpwclass(), auth_get_canonical_hostname() is called.
This function calls remote_hostname() to reverse resolve hostname from client address.
It is not allowed in capability mode because of /etc/nsswich.conf access.

I fixed this problem to cache the hostname before entering capability mode.
The client hostname will never change after accepting the connection.
We don't need to update the hostname, so caching will work for this problem.

remote_hostname() function caches the reverse resolved hostname.
So my fix is simple to call auth_get_canonical_hostname() before entering capability mode.

[naito.yuichiro_gmail.com]

I see that r338561 commit updates openssh to 7.8p1.
I'm going to update my patch for this version and create a new differential.

This differential is useful for stable/11 branch.
I will keep this differential remained.

[cem]

In D17056#364904, @naito.yuichiro_gmail.com wrote:

I found one problem that sshd fails to reverse resolve hostname if server is set UseDNS yes .

Good find!

remote_hostname() function caches the reverse resolved hostname.
So my fix is simple to call auth_get_canonical_hostname() before entering capability mode.

That seems reasonable to me.

In D17056#364908, @naito.yuichiro_gmail.com wrote:

I see that r338561 commit updates openssh to 7.8p1.
I'm going to update my patch for this version and create a new differential.

Thanks!

[naito.yuichiro_gmail.com]

Created D17128 for OpenSSH-7.8p1.

[cem]

Superceded by D17128, committed in rS339216.