Page MenuHomeFreeBSD

[sshd] add wrapper function of login_getpwclass
Needs ReviewPublic

Authored by naito.yuichiro_gmail.com on Sep 6 2018, 1:10 AM.

Details

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"
  1. ssh login
$ ssh localhost
  1. expect successfully login

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

naito.yuichiro_gmail.com edited the test plan for this revision. (Show Details)Sep 6 2018, 1:21 AM
cem added a comment.Sep 6 2018, 1:30 AM

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 edited the test plan for this revision. (Show Details)Sep 6 2018, 1:30 AM

regenerate patch by `diff -U9999```

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

cem added a reviewer: des.Sep 6 2018, 4:38 PM

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 requested changes to this revision.Sep 6 2018, 5:03 PM

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.
  1. (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
75–76

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

This revision now requires changes to proceed.Sep 6 2018, 5:03 PM
emaste added a comment.Sep 6 2018, 9:08 PM

How do we coordinate with upstream on this?

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.

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'.

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 accepted this revision.Sep 8 2018, 1:37 AM

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.

This revision is now accepted and ready to land.Sep 8 2018, 1:37 AM
buffer.c
153

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

buffer.h
95

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.

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.).
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.

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

This revision now requires review to proceed.Sep 10 2018, 7:50 AM
cem accepted this revision.Sep 10 2018, 3:46 PM

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

This revision is now accepted and ready to land.Sep 10 2018, 3:46 PM

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.

This revision now requires review to proceed.Sep 11 2018, 10:01 AM

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 added a comment.Sep 11 2018, 3:14 PM

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.

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!