Page MenuHomeFreeBSD

NTPD Circumvent ASLR
ClosedPublic

Authored by cy on Nov 14 2019, 3:02 AM.

Details

Summary

ntpd fails again with ASLR stack gap != 0. This patch circumvents the issue by duplicating proccontrol within ntpd until a proper solution can be developed with nwtime.org.

This has been submitted upstream and may be included in the next release of ntp.

This is a resolution for PR/241421 and PR/241960.

A corresponding ports patch will be created.

Test Plan

Currently in use locally.

Submitted upstream.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cy created this revision.Nov 14 2019, 3:02 AM
imp accepted this revision.Nov 14 2019, 3:20 AM

Seems sane to me, but I'd feel better about getting it committed if others more deeply familiar with the aslr stuff give it their blessing.

This revision is now accepted and ready to land.Nov 14 2019, 3:20 AM
kib added a comment.Nov 14 2019, 7:24 PM

If you want to disable ASLR, it is enough to issue procctl(), checking global system settings gives no value and is in fact racy.

If the problem is only with the gap, then you can just disable the gap using procctl, I believe this is what jdk does.

That said, why the gap is the problem ? Wasn't the rlimit(2) call that created problems, removed from ntpd ?

contrib/ntp/ntpd/ntpd.c
427 ↗(On Diff #64299)

And if __FreeBSD_cc_version is undefined, this code is ending with the error.

cy added a comment.Nov 14 2019, 9:54 PM

The rlimit() for mlock() was removed. The rlimit() for stack wasn't. Removing the second rlimit() causes an an assertion callback when an IPv6 packet is received. Your suggestion for simply calling procctl() works as well.

cy updated this revision to Diff 64341.Nov 14 2019, 9:55 PM

Unconditionally call procctl() as suggested by kib@.

This revision now requires review to proceed.Nov 14 2019, 9:55 PM
kib added inline comments.Nov 14 2019, 10:25 PM
contrib/ntp/ntpd/ntpd.c
141 ↗(On Diff #64341)

Why not #ifdef __FreeBSD ?

415 ↗(On Diff #64341)

I suggest to disable stack gap instead of ASLR. Use PROC_STACKGAP_CTL with DISABLE when available. If not available, it is safe to assume that stackgap is not supported as well.

ian added inline comments.Nov 14 2019, 10:33 PM
contrib/ntp/ntpd/ntpd.c
409 ↗(On Diff #64341)

Is there a PR for this problem? I think this whole block needs a comment, something simple like

/* Must disable STACKGAP on FreeBSD to avoid <whatever the problem was>. See PR NNNN. */

cy updated this revision to Diff 64348.Nov 15 2019, 12:47 AM
cy edited the summary of this revision. (Show Details)

A better solution.

cy added inline comments.Nov 15 2019, 12:53 AM
contrib/ntp/ntpd/ntpd.c
141 ↗(On Diff #64341)

FreeBSD is not defined by llvm or gcc. FreeBSD_cc_version is defined by our cc, without including any headers. It's portable and can be shared with our ntp upline, nwtime.org.

imp added inline comments.Nov 15 2019, 4:21 AM
contrib/ntp/ntpd/ntpd.c
141 ↗(On Diff #64341)

Ummm, FreeBSD is defined to be the major version of FreeBSD without needing a header. FreeBSD_version needs sys/param.h....

imp added inline comments.Nov 15 2019, 4:33 AM
contrib/ntp/ntpd/ntpd.c
141 ↗(On Diff #64341)

FreeBSD is defined, I should have said

cy updated this revision to Diff 64360.Nov 15 2019, 5:40 AM

s/__FreeBSD_cc_version/__FreeBSD__/

kib added inline comments.Nov 15 2019, 8:46 AM
contrib/ntp/ntpd/ntpd.c
421 ↗(On Diff #64360)

I suggest to silently ignore the error from procctl(2). The only real situation where it can occur is when PROC_STACKGAP_CTL is not supported, because the kernel we running on is too old. Then there is no harm from the error, and running the daemon instead of abruptly terminating is much better outcome.

I might even go further with this suggestion and propose to do e.g.

#ifdef __FreeBSD__
#include <sys/procctl.h>
#ifndef PROC_STACKGAP_CTL
#define	PROC_STACKGAP_CTL	17
#define	PROC_STACKGAP_DISABLE	0x0002
#endif
...

So that even if we compile on the older system, we still can run on newer kernels.

That said, we probably need to allocate a bit for stack gap in image flags. Unfortunately, the image flag editor is still not installed.

kib added a subscriber: emaste.Nov 15 2019, 8:46 AM
cy updated this revision to Diff 64371.Nov 15 2019, 3:20 PM

Latest reviews incorporated.

kib accepted this revision.Nov 15 2019, 3:39 PM
This revision is now accepted and ready to land.Nov 15 2019, 3:39 PM