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.

Sign up for GitHub

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

Jump to bottom

[Proposal] Use runc-dmz to defeat CVE-2019-5736 #3983

Closed
wants to merge 4 commits into from
Closed

[Proposal] Use runc-dmz to defeat CVE-2019-5736 #3983

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
29fd9f6
add runc-dmz.c
lifubang Aug 15, 2023
48debd1
make runc & runc-dmz at the same time
lifubang Aug 15, 2023
7adf5a5
use runc-dmz to defeat CVE-2019-5736
lifubang Aug 15, 2023
7c7e825
let unit test work after importing runc-dmz
lifubang Aug 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions contrib/cmd/runc-dmz/runc-dmz.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#include <unistd.h>

extern char **environ;

int main(int argv, char **args)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int main(int argv, char **args)
int main(int argc, char **argv)

{
if (argv > 0) {
return execve(args[0], args, environ);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering about ensuring the first argument, i.e. the executed binary is runc.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

The first argument is not runc -- it's the container pid1 program.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh OK! Thank you for the precision! I still need to polish my runc knowledge.

Copy link
Contributor

Choose a reason for hiding this comment

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

The first argument is not runc -- it's the container pid1 program.

If you try to run the C binary directly, I think the first argument of args would be runc-dmz?

}
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

Probably this should be non-zero

}
Copy link
Member

Choose a reason for hiding this comment

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

I guess we can optimize the binary footprint by eliminating glibc and executing “syscall” instruction directly, but that may happen in follow-up PRs

Copy link
Member

Choose a reason for hiding this comment

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

If we build this binary with musl, you get a 13KB binary. With glibc it's 1.1MB. We will need to adjust our build system for this -- but there is a separate issue that distributions might find building a single binary with musl quite difficult. Speaking from the SUSE side, I don't think we even package musl for SLES so we would need to build with glibc (meaning the binary would be 1.1MB in size -- almost 100 times larger).

As you say, this can be done in a future PR.

Copy link
Member

Choose a reason for hiding this comment

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

Rewriting this in asm will be much easier than complicating libc dependency

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if there is a silver bullet here:

  • with glibc, you get a big binary.
  • with musl, you get a small binary but this can be painful to build against musl from some distribution.
  • with pure assembly, you will need as many files as supported architecture and choosing which file to build depending on the target architecture.

Maybe using syscall directly can be a middle ground, but you will need to defined the execve syscall number depending on the target architecture.

Copy link
Member

Choose a reason for hiding this comment

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

syscall doesn't reduce the binary size unfortunately (also, libcs provide the syscall numbers for all syscalls in <sys/syscall.h>).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sad :(.
Let me know regarding the assembly files, I can give a hand for amd64 and arm64!

Copy link
Contributor

Choose a reason for hiding this comment

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

https://gist.github.com/Zheaoli/f37bc1fb04917fdfac36d644ee69f7e9
I made a demo for amd64. The binary file size is 4.9k.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is my contribution for arm64:

From 95defc03b7ec526c9c966735ebeb5bac5a9a34a0 Mon Sep 17 00:00:00 2001
From: Francis Laniel <flaniel@linux.microsoft.com>
Date: Wed, 30 Aug 2023 13:06:27 +0200
Subject: [PATCH] runc-dmz: Add arm64 assembly code.

Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
---
 contrib/cmd/runc-dmz/runc-dmz-arm64.S | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 contrib/cmd/runc-dmz/runc-dmz-arm64.S

diff --git a/contrib/cmd/runc-dmz/runc-dmz-arm64.S b/contrib/cmd/runc-dmz/runc-dmz-arm64.S
new file mode 100644
index 00000000..25387e61
--- /dev/null
+++ b/contrib/cmd/runc-dmz/runc-dmz-arm64.S
@@ -0,0 +1,25 @@
+.equ EXECVE_SYSCALL_NUMBER, 221
+.equ EXIT_SYSCALL_NUMBER, 93
+
+.text
+.global _start
+_start:
+	ldr x3, [sp] // *sp contains argc, so x3 = argc
+	cmp x3, #0
+	bgt execve
+	mov x0, #0
+
+exit:
+	mov x8, EXIT_SYSCALL_NUMBER
+	svc #0
+
+execve:
+	add x1, sp, #16 // x1 = &argv[1]
+	ldr x0, [x1] // x0 = argv[1]
+	# argv[argc] is NULL and environment starts at argv[argc + 1].
+	mov x4, #8 // x4 = 8
+	mul x3, x3, x4 // x3 = (argc + 1) * 8
+	add x2, x1, x3 // x3 = &argv[argc + 1], i.e. environment.
+	mov x8, EXECVE_SYSCALL_NUMBER
+	svc #0
+	b exit
-- 
2.34.1

I tested it both through emulation and virtualization:

francis@pwmachine:~/Codes/kinvolk/runc/contrib/cmd/runc-dmz$ aarch64-linux-gnu-as -g runc-dmz-arm64.S -o runc-dmz-arm64.o                                  (95defc03...) %
francis@pwmachine:~/Codes/kinvolk/runc/contrib/cmd/runc-dmz$ aarch64-linux-gnu-ld -static runc-dmz-arm64.o                                                 (95defc03...) %
francis@pwmachine:~/Codes/kinvolk/runc/contrib/cmd/runc-dmz$ du -sh a.out                                                                                  (95defc03...) %
4,0K    a.out
francis@pwmachine:~/Codes/kinvolk/runc/contrib/cmd/runc-dmz$ file a.out                                                                                    (95defc03...) %
a.out: ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), statically linked, with debug_info, not stripped
francis@pwmachine:~/Codes/kinvolk/runc/contrib/cmd/runc-dmz$ qemu-aarch64-static ./a.out /bin/ls -al                                                       (95defc03...) %
total 32
drwxrwxr-x 2 francis francis 4096 août  30 13:09 .
drwxrwxr-x 7 francis francis 4096 août  29 17:23 ..
-rw-rw-r-- 1 francis francis 1213 août  30 13:06 0001-runc-dmz-Add-arm64-assembly-code.patch
-rwxrwxr-x 1 francis francis 1832 août  30 13:09 a.out
-rw-rw-r-- 1 francis francis 2168 août  30 13:09 runc-dmz-arm64.o
-rw-rw-r-- 1 francis francis  524 août  30 13:06 runc-dmz-arm64.S
-rw-rw-r-- 1 francis francis  168 août  29 17:23 runc-dmz.c
-rw-rw-r-- 1 francis francis  700 août  29 18:59 runc-dmz.s
# Inside arm64 VM:
root@vm-arm64:~/share/kinvolk/runc/contrib/cmd/runc-dmz# lscpu | head -1
Architecture:                       aarch64
root@vm-arm64:~/share/kinvolk/runc/contrib/cmd/runc-dmz# ./a.out /bin/ls -al
total 32
drwxrwxr-x 2 1000 1000 4096 Aug 30 11:09 .
drwxrwxr-x 7 1000 1000 4096 Aug 29 15:23 ..
-rw-rw-r-- 1 1000 1000 1213 Aug 30 11:06 0001-runc-dmz-Add-arm64-assembly-code.patch
-rwxrwxr-x 1 1000 1000 1832 Aug 30 11:09 a.out
-rw-rw-r-- 1 1000 1000  524 Aug 30 11:06 runc-dmz-arm64.S
-rw-rw-r-- 1 1000 1000 2168 Aug 30 11:09 runc-dmz-arm64.o
-rw-rw-r-- 1 1000 1000  168 Aug 29 15:23 runc-dmz.c
-rw-rw-r-- 1 1000 1000  700 Aug 29 16:59 runc-dmz.s

Feel free to apply the patch! In any case, it was funny to write arm64 assembly from scratch as it was a long time I did not do that.
If you see any problem with the code, share your feedback and I will address your comments!

Copy link
Member Author

Choose a reason for hiding this comment

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

I think maybe there still have some design problems for runc-dmz proposal.
Please see: #3987 (comment)
But I can't confirm, if you can check there is no problem for runc-dmz propsal, it will be appreciated. Because I don't want to waste contributor's time for a wrong proposal.