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.
Details
Details
- Reviewers
oshogbo emaste des naito.yuichiro_gmail.com
Successfully login via ssh with sysctl kern.trap_enotcap=1
- set kern.trap_enotcap=1
$ su root -c "sysctl kern.trap_enotcap=1"- ssh login
$ ssh localhost- expect successfully login
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
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
Comment Actions
Hi Conrad.
Thanks for the advice. I regenerated patch file. It seems good differential.
Comment Actions
Thanks!
Including des@ on the review, as he has been most active in maintaining our base sshd.
Comment Actions
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.
| |
| 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 | ||
| 75–76 | Maybe spell as caph_cache_tzdata() instead. It does the same thing but shows intent without the comment. | |
| 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 | ||
| 75–76 | Indeed. I'll fix this. | |
Comment Actions
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'.
Comment Actions
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.
Comment Actions
Looks great to me! All of my comments below are just style or message suggestions, nothing functional.
| buffer.c | ||
|---|---|---|
| 153 | 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 | 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. | |
| 308–311 | 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. | |
| buffer.c | ||
|---|---|---|
| 153 | Thanks for the comment. I was little confused about this. | |
| buffer.h | ||
| 95 | Yes. sturct passwd * will never be changed in buffer_put_passwd. | |
| 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. | |
| 308–311 | I'll fix this too. | |
| sandbox-capsicum.c | ||
| 27–34 | I see capsicum_helpers.h refers to libc headers (time.h, unistd.h, etc.). | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Thanks!