Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ubiquitous compartmentalisation #2012

Merged
merged 14 commits into from
Apr 15, 2024
Merged

Ubiquitous compartmentalisation #2012

merged 14 commits into from
Apr 15, 2024

Conversation

dpgao
Copy link
Contributor

@dpgao dpgao commented Feb 19, 2024

The default RTLD is modified to allow you to turn compartmentalisation on by setting the environment variable LD_COMPARTMENT_ENABLE.

Without setting the environment variable, RTLD behaves exactly as before.

@dpgao
Copy link
Contributor Author

dpgao commented Feb 20, 2024

@brooksdavis Commit 8bb8e5d removes a number of ifdef RTLD_SANDBOXs that you introduced in commit bcc7f63. Is this OK?

@jrtc27
Copy link
Member

jrtc27 commented Feb 20, 2024

@brooksdavis Commit 8bb8e5d removes a number of ifdef RTLD_SANDBOXs that you introduced in commit bcc7f63. Is this OK?

The point of those is so that, when building the various c18n libraries, duplicate config etc files are not installed. Given you still have separate c18n and non-c18n libc builds I don't know why you're removing the ifdefs? The definitions come from lib/libc_c18n etc.

@jrtc27
Copy link
Member

jrtc27 commented Feb 20, 2024

Can you please be more descriptive in your commit messages? I strongly suspect you are the only one to whom they have a precise meaning; to the rest of us it's not at all clear what specific things you're trying to achieve with each. We understand the high-level overview of what the goal is, but not the details for how that is being done.

@dpgao
Copy link
Contributor Author

dpgao commented Mar 8, 2024

@jrtc27 This PR is ready for review now. Thanks!

@dpgao dpgao changed the title Ubiquitous compartmentalisation [WIP] Ubiquitous compartmentalisation Mar 9, 2024
@dpgao
Copy link
Contributor Author

dpgao commented Mar 9, 2024

@jrtc27 I've adapted procctl to allow enabling/disabling compartmentalisation. We still need to add ifdefs around these code but I wonder what the name should be and where to set them.

The man page also needs to be updated.

@dpgao dpgao requested review from brooksdavis and bsdjhb March 9, 2024 20:02
@dpgao dpgao changed the title [WIP] Ubiquitous compartmentalisation Ubiquitous compartmentalisation Mar 11, 2024
@dpgao
Copy link
Contributor Author

dpgao commented Mar 11, 2024

ifdefs have been added to the code and a new kernel option CHERI_C18N has been added.

libexec/rtld-elf-c18n/Makefile Outdated Show resolved Hide resolved
libexec/rtld-elf/aarch64/rtld_start.S Outdated Show resolved Hide resolved
libexec/rtld-elf/rtld_c18n.c Show resolved Hide resolved
libexec/rtld-elf/rtld.c Show resolved Hide resolved
libexec/rtld-elf/rtld.h Outdated Show resolved Hide resolved
libexec/rtld-elf/rtld_c18n.c Outdated Show resolved Hide resolved
libexec/rtld-elf/rtld_c18n.c Outdated Show resolved Hide resolved
libexec/rtld-elf/rtld_c18n.h Outdated Show resolved Hide resolved
@@ -45,16 +45,22 @@
#endif
#endif

#ifndef _RTLD_C18N_FILE_SUFFIX
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment for all of these:

What is the intended set of files used by each linker in the various different scenarios? I foresee issues with ld-elf being both c18n and non-c18n when it comes to ldconfig, since you need the c18n libc+libthr for c18n but the non-c18n one for non-c18n, yet it's the same rtld. What's making sure the right one gets used? Similarly what's making sure the files are configured correctly by /etc/rc.d/ldconfig? Note that these days it asks rtld itself for the system lib path, which may give different output based on what the default c18n configuration is, which sounds like something that you don't want.

sys/arm64/conf/GENERIC-MORELLO Outdated Show resolved Hide resolved
Copy link
Member

@brooksdavis brooksdavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have general concerns about the policy file format and the related data structures, but I don't think they matter much for this release as long as we have a migration path that includes the possibility of significant changes. In particular, I think there would be a lot of value in something existing tools can manipulate (e.g., JSON or UCL) since I expect policies to be large and machine generated.

libexec/rtld-elf/rtld_c18n.c Outdated Show resolved Hide resolved
struct string_base *symbols;
char buf[C18N_POLICY_IDENT_MAX];

if (!eat_token(&cur, "Version 1\n"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear the tabs are going to lead to a great deal of frustration.

sys/arm64/conf/GENERIC-MORELLO Outdated Show resolved Hide resolved
@dpgao
Copy link
Contributor Author

dpgao commented Mar 13, 2024

I think you don't need this define. Just enable if the kernel is CHERI enabled and on Morello since it's harmless if rtld doesn't know about them. We'll want to get to the point where it's always on fairly quickly in DEC.

@brooksdavis How do I test if we are building on CHERI and Morello? Something like defined(__ELF_CHERI) && defined(__aarch64__) in kernel and __has_feature(capabilities) && defined(__aarch64__) for proccontrol?

@brooksdavis
Copy link
Member

I think you don't need this define. Just enable if the kernel is CHERI enabled and on Morello since it's harmless if rtld doesn't know about them. We'll want to get to the point where it's always on fairly quickly in DEC.

@brooksdavis How do I test if we are building on CHERI and Morello? Something like defined(__ELF_CHERI) && defined(__aarch64__) in kernel and __has_feature(capabilities) && defined(__aarch64__) for proccontrol?

That would work. To centralize things, you could conceivably only define PROC_CHERI_C18N_CTL/STATUS on morello using the second expression and then key off their presence elsewhere.

@dpgao
Copy link
Contributor Author

dpgao commented Mar 15, 2024

@jrtc27 I agree with your thoughts about elfctl and ldconfig. The current PR is certainly not the ideal setup of 'ubiquitous compartmentalisation', but more of an intermediate step that merely adds a hidden flag to ld-elf.so.1 that enables compartmentalisation. Users (including ldconfig) unaware of this will not perceive any difference.

With this in mind, can we merge this PR as-is so that people can start experimenting with the feature first? In a future PR, I will make the breaking changes of:

  1. Remove ld-elf*-c18n.so.1 and add elfctl support
  2. Change cheribsdtest-*-c18n to use ELF flags
  3. Change ldconfig to call ld-elf.so.1 -v twice with different env vars
  4. Add a sysctl flag to enable c18n by default
  5. Documenting all of the above

@dpgao dpgao force-pushed the c18n-ubiq branch 6 times, most recently from e54a477 to ba3a9f1 Compare March 16, 2024 23:55
@dpgao dpgao requested a review from brooksdavis April 15, 2024 15:19
*/
extern bool ld_compartment_enable;

#define C18N_ENABLED ld_compartment_enable
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move the extern Obj_Entry obj_rtld here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, if I do that, then an error will appear in reloc.c:reloc_non_plt because one of its arguments also has the name obj_rtld...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, reading rtld, it seems to be quite careful to avoid exporting obj_rtld. Maybe pass it as an argument to c18n_init instead of using it as a global? Then you can leave it static in rtld.c.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added an XXX to the extern Obj_Entry obj_rtld. It turns out that I don't need obj_rtld to be global after interrupt-safety is implemented. So this change is only a temporary hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, reading rtld, it seems to be quite careful to avoid exporting obj_rtld. Maybe pass it as an argument to c18n_init instead of using it as a global? Then you can leave it static in rtld.c.

I hadn't refreshed my page to see your last comment when I posted mine. Yes, passing obj_rtld as an argument is a better idea. Fix now pushed.

ObsoleteFiles.inc Show resolved Hide resolved
sys/sys/proc.h Show resolved Hide resolved
@dpgao dpgao force-pushed the c18n-ubiq branch 2 times, most recently from bd12598 to c8ccdd4 Compare April 15, 2024 19:04
{
extern Obj_Entry obj_rtld;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you no longer need the extern if you are passing it in now? (Presumably this won't compile?)

return (tframe->pc);
_rtld_sighandler = tramp_intern(NULL, &(struct tramp_data) {
.target = _rtld_sighandler_unsafe,
.defobj = &obj_rtld,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.defobj = &obj_rtld,
.defobj = obj_rtld,

Would seem to be needed now as well (and below)?

dpgao added 14 commits April 15, 2024 21:40
Undefined weak symbols are represented by a dummy sym_zero definition in
RTLD. Correctly handle this special case when creating a trampoline.
Non-PLT functions must have a canonical address so that different
libraries see the same address for the same function.

Previously, some but not all non-PLT functions are wrapped in
trampolines, breaking function pointer equality. For example, relative
relocations of function pointers are not wrapped in trampolines at all.

This commit completely disables trampolines for non-PLT relocations
until a complete solution for wrapping all function pointers is
implemented.
Otherwise this causes cheribsdtest's 'initial value of restricted stack
capability' test to fail with the default RTLD when ubiquitous
compartmentalisation is implemented.
Compartmentalisation can be enabled on the ld-elf.so.1 linker by setting
the environment variable LD_COMPARTMENT_ENABLE (unless
LD_COMPARTMENT_DISABLE also set, which prevails).

The existing ld-elf*-c18n.so.1 linkers are removed.
This reverts commit c56fe83 now that
the default RTLD supports compartmentalisation.
This makes functions such as dladdr work even when given a trampoline.
Some software like bmake calls sigaction after calling vfork which
corrupts RTLD's signal dispatch table. Securely supporting vfork under
c18n has proven a difficult task and this commit redirects vfork to fork
when c18n is enabled.

rfork may be used when the user intentionally wants vfork-like memory
sharing behaviour. Thus, only calling it with flags deemed to be 'safe'
is allowed under c18n, unless the call is made from libc (as part of the
implementation for posix_spawn).

This commit also partially reverts
50865cd by removing the execve(2) family
of functions from the list of trusted symbols.

Also redirect _thr_exit and __sys_thr_exit to _rtld_thr_exit.
@dpgao
Copy link
Contributor Author

dpgao commented Apr 15, 2024

Seems like CI had a bogus failure earlier. Have now rebased to latest dev and will merge once CI passes.

@dpgao dpgao merged commit 169a5f0 into dev Apr 15, 2024
11 of 16 checks passed
@dpgao dpgao deleted the c18n-ubiq branch April 15, 2024 21:17
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Aug 9, 2024
New features:
  CTSRD-CHERI#1941 uudecode filter: support file name and file mode in raw mode
  CTSRD-CHERI#1943 7-zip reader: translate Windows permissions into UNIX
        permissions
  CTSRD-CHERI#1962 zstd filter now supports the "long" write option
  CTSRD-CHERI#2012 add trailing letter b to bsdtar(1) substitute pattern
  CTSRD-CHERI#2031 PCRE2 support
  CTSRD-CHERI#2054 add support for long options "--group" and "--owner" to tar(1)

Security fixes:
  CTSRD-CHERI#2101 Fix possible vulnerability in tar error reporting introduced
        in f27c173

Important bugfixes:
  CTSRD-CHERI#1974 ISO9660: preserve the natural order of links
  CTSRD-CHERI#2105 rar5: fix infinite loop if during rar5 decompression the last
        block produced no data
  CTSRD-CHERI#2027 xz filter: fix incorrect eof at the end of an lzip member
  CTSRD-CHERI#2043 zip: fix end-of-data marker processing when decompressing zip
        archives

Obtained from:		libarchive
Libarchive commit:	4fcc02d906cca4b9e21a78a833f1142a2689ec52
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Aug 9, 2024
Libarchive 3.7.3

New features:
  CTSRD-CHERI#1941 uudecode filter: support file name and file mode in raw mode
  CTSRD-CHERI#1943 7-zip reader: translate Windows permissions into UNIX
        permissions
  CTSRD-CHERI#1962 zstd filter now supports the "long" write option
  CTSRD-CHERI#2012 add trailing letter b to bsdtar(1) substitute pattern
  CTSRD-CHERI#2031 PCRE2 support
  CTSRD-CHERI#2054 add support for long options "--group" and "--owner" to tar(1)

Security fixes:
  CTSRD-CHERI#2101 Fix possible vulnerability in tar error reporting introduced
        in f27c173

Important bugfixes:
  CTSRD-CHERI#1974 ISO9660: preserve the natural order of links
  CTSRD-CHERI#2105 rar5: fix infinite loop if during rar5 decompression the last
        block produced no data
  CTSRD-CHERI#2027 xz filter: fix incorrect eof at the end of an lzip member
  CTSRD-CHERI#2043 zip: fix end-of-data marker processing when decompressing zip
        archives

PR:		278315 (exp-run)
MFC after:	1 week
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Aug 9, 2024
Libarchive 3.7.3

New features:
  CTSRD-CHERI#1941 uudecode filter: support file name and file mode in raw mode
  CTSRD-CHERI#1943 7-zip reader: translate Windows permissions into UNIX
        permissions
  CTSRD-CHERI#1962 zstd filter now supports the "long" write option
  CTSRD-CHERI#2012 add trailing letter b to bsdtar(1) substitute pattern
  CTSRD-CHERI#2031 PCRE2 support
  CTSRD-CHERI#2054 add support for long options "--group" and "--owner" to tar(1)

Security fixes:
  CTSRD-CHERI#2101 Fix possible vulnerability in tar error reporting introduced
        in f27c173

Important bugfixes:
  CTSRD-CHERI#1974 ISO9660: preserve the natural order of links
  CTSRD-CHERI#2105 rar5: fix infinite loop if during rar5 decompression the last
        block produced no data
  CTSRD-CHERI#2027 xz filter: fix incorrect eof at the end of an lzip member
  CTSRD-CHERI#2043 zip: fix end-of-data marker processing when decompressing zip
        archives

PR:		278315 (exp-run)
MFC after:	1 week
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants