Page MenuHomeFreeBSD
Differential D45567

wall(1): cappsicumizing wall
Needs ReviewPublic
Actions

Authored by hanslu952_gmail.com on Jun 12 2024, 7:53 AM.
Tags
None
Referenced Files
F151206002: D45567.id140510.diff
Mon, Apr 6, 8:04 PM
Unknown Object (File)
Sun, Apr 5, 4:38 PM
Unknown Object (File)
Sun, Apr 5, 3:04 PM
Unknown Object (File)
Sun, Apr 5, 1:05 PM
Unknown Object (File)
Sun, Apr 5, 8:19 AM
Unknown Object (File)
Sun, Apr 5, 7:32 AM
Unknown Object (File)
Sat, Apr 4, 4:12 AM
Unknown Object (File)
Sat, Apr 4, 4:08 AM
View All Files
Subscribers
imp
jonathan

Details

Reviewers
oshogbo
lwhsu
Summary

The first problem is to use pdfork.
because fork() is disallowed in capability mode,
we use pdfork() to replace ,the semantic is same as rwhod
The second problem is to limit the file descriptor used by open.
it sets up a specific right (write) for a file descriptor and then
attempts to enforce the limitation. If the operation fails,
the program will terminate with a specific error message

Sponsored by: Google, Inc. (GSoC 2024)

Test Plan

Nonblocking test

setup a c program to fill up the pipe of /dev/pts/$any num
the program code is as follows
https://hackmd.io/@black13524/ryClhYgnA

$ cc -o nonblocking nonblocking.c
$ ./nonblocking
$ make 
$ ktrace ./wall-test msg
$ kdump -f ktrace.out

normal test

$ make 
$ ktrace ./wall-test msg
$ kdump -f ktrace.out

Set up enotcapable to get coredump

$ sysctl kern.trap_enotcap=1
$ sysctl kern.corefile= /tmp/coredumps/%N.core
$ gdb executable corefile

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 59111
Build 55998: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a subscriber: jonathan. · View Herald TranscriptJun 12 2024, 7:53 AM
hanslu952_gmail.com requested review of this revision.Jun 12 2024, 7:53 AM
Harbormaster completed remote builds in B58150: Diff 139743.Jun 12 2024, 7:53 AM
oshogbo requested changes to this revision.Jun 13 2024, 4:30 PM

In the "Test plan" please provide an example of testing blocking (forked version) and unblocked version.

usr.bin/wall/ttymsg.c
58

Why this has to be global?

66–67

I guess we don't need foked and fdp.

125–128

Does this code complies? I think you have a typo in variable name.

usr.bin/wall/wall.c
123

No need for extra line.

187

I think we have to enter capability mode a little bit earlier then before exit.
The cap_enter/caph_enter function is a barrier which says here starts a "untrusted part", know you are protecting nothing.

This revision now requires changes to proceed.Jun 13 2024, 4:30 PM
hanslu952_gmail.com updated this revision to Diff 139972.Jun 19 2024, 8:27 AM
  • Fix pdfork and take off unneccessary code
  • remove line break
Harbormaster completed remote builds in B58230: Diff 139972.Jun 19 2024, 8:27 AM
hanslu952_gmail.com edited the test plan for this revision. (Show Details)Jun 19 2024, 8:29 AM
hanslu952_gmail.com updated this revision to Diff 139973.Jun 19 2024, 8:38 AM
  • fix format of ttymsg
Harbormaster completed remote builds in B58231: Diff 139973.Jun 19 2024, 8:38 AM
oshogbo added a comment.Jun 19 2024, 11:41 AM

Hym I don't think this is complete patch.
Can you try to regenerate git diff -U99999 first_commit^ (the ^ is important)

hanslu952_gmail.com updated this revision to Diff 139980.Jun 19 2024, 1:53 PM

Reupload the patch

Harbormaster completed remote builds in B58233: Diff 139980.Jun 19 2024, 1:53 PM
oshogbo added a comment.Jun 19 2024, 2:05 PM

It still doesn't seem right.

hanslu952_gmail.com updated this revision to Diff 139981.Jun 19 2024, 2:17 PM

Trying on more earlier version

Harbormaster completed remote builds in B58234: Diff 139981.Jun 19 2024, 2:18 PM
hanslu952_gmail.com updated this revision to Diff 139982.Jun 19 2024, 2:23 PM

Sorry I found it

Harbormaster completed remote builds in B58235: Diff 139982.Jun 19 2024, 2:23 PM
lwhsu added a comment.Jun 19 2024, 3:37 PM

There are many whitespace changes, if possible, please don't change the original ones (even they are wrong) in a feature addition change, or it would cause unnecessary review complexity.

oshogbo added inline comments.Jun 19 2024, 3:43 PM
usr.bin/wall/ttymsg.c
121

Why we are entering capability mode here?

125–128

How can this work?
You are reassigning fd (which was the open file) to a process descriptor.

hanslu952_gmail.com updated this revision to Diff 140509.Jul 3 2024, 2:21 PM
This comment was removed by hanslu952_gmail.com.
Harbormaster completed remote builds in B58476: Diff 140509.Jul 3 2024, 2:21 PM
hanslu952_gmail.com updated this revision to Diff 140510.Jul 3 2024, 2:30 PM

path sandboxing and enter capability mode earlier

  • Reupload due to including wrong commit
Harbormaster completed remote builds in B58477: Diff 140510.Jul 3 2024, 2:30 PM
hanslu952_gmail.com updated this revision to Diff 140726.Jul 9 2024, 2:47 PM
  • rearrangement and timecache
Harbormaster completed remote builds in B58587: Diff 140726.Jul 9 2024, 2:47 PM
oshogbo added inline comments.Jul 10 2024, 6:06 AM
usr.bin/wall/ttymsg.c
59
67

We can combine 3 int variables to a single line.

69

Why we need ./ ?

89

So WRONLY or RDONLY or maybe O_RDWR?

93
123–125

How removed forked change the process menagment?
How many child process will have this process before and after this change.

usr.bin/wall/wall.c
74
83

Why?

87–88
159
221
hanslu952_gmail.com added inline comments.Jul 12 2024, 10:38 AM
usr.bin/wall/ttymsg.c
123–125

each parent still creates one child,but the number of parent process grows exponentially when pdfork is called more than once.
like:
once is the same
when pdfork is called twice
we will have five parent ,each parent create one child

oshogbo added inline comments.Jul 12 2024, 11:41 AM
usr.bin/wall/ttymsg.c
123–125

I don't think this what we want, no?

hanslu952_gmail.com added inline comments.Jul 12 2024, 1:09 PM
usr.bin/wall/ttymsg.c
69

to get the needed file without using absolute path

oshogbo added inline comments.Jul 12 2024, 1:17 PM
usr.bin/wall/ttymsg.c
69

But file and ./file are both relative.

hanslu952_gmail.com updated this revision to Diff 140858.Jul 13 2024, 7:52 AM
  • add back forked and fix format
Harbormaster completed remote builds in B58630: Diff 140858.Jul 13 2024, 7:52 AM
hanslu952_gmail.com retitled this revision from Capsicumizing wall to wall(1): cappsicumizing wall.Jul 17 2024, 3:53 PM
hanslu952_gmail.com edited the test plan for this revision. (Show Details)Aug 5 2024, 5:53 PM
hanslu952_gmail.com edited the summary of this revision. (Show Details)Aug 15 2024, 5:31 PM
hanslu952_gmail.com edited the test plan for this revision. (Show Details)
oshogbo added inline comments.Aug 20 2024, 12:49 PM
usr.bin/wall/ttymsg.c
66–67

Why fd2? Where is fd1?

125–128

Why close dfd?

132

Why dfd?
Where do we clean fd2?

144

Why dfd?

154–155

Why dfd?

usr.bin/wall/wall.c
78

Why fd2? Where is fd1?

86

Why we need these transformations?

186

Wasn't devfd was closed in ttymsg?

hanslu952_gmail.com updated this revision to Diff 142272.Aug 20 2024, 3:55 PM
hanslu952_gmail.com edited the test plan for this revision. (Show Details)
  • fix fd problem
Harbormaster completed remote builds in B59111: Diff 142272.Aug 20 2024, 3:55 PM
hanslu952_gmail.com edited the test plan for this revision. (Show Details)Aug 20 2024, 3:56 PM
oshogbo added inline comments.Aug 22 2024, 7:30 PM
usr.bin/wall/ttymsg.c
86

With got we cannot get these errors?

86

sorry...

With "openat" we cannot get these errors?

90

Why have we changed the way we return errors?

127

Why have you changed identation?

128

Where are we closing the process descriptor?

133

Why we removed this?

159

Why we remove this part?

usr.bin/wall/wall.c
119

I guess /dev/pts?

hanslu952_gmail.com added inline comments.Aug 23 2024, 11:52 AM
usr.bin/wall/ttymsg.c
128

I think _exit() will close the process?

oshogbo added inline comments.Aug 23 2024, 12:35 PM
usr.bin/wall/ttymsg.c
128

Which one?
Which one has a process descriptor?

hanslu952_gmail.com added inline comments.Aug 23 2024, 1:55 PM
usr.bin/wall/ttymsg.c
128

It seems not in tty,in main after closing devfd?

hanslu952_gmail.com added inline comments.Aug 24 2024, 6:00 PM
usr.bin/wall/ttymsg.c
128

Oh sorry,I used gdb wrong ,It wont exit.I found that it should exit at 160 - 161,so I will add back it

hanslu952_gmail.com updated this revision to Diff 142410.Aug 24 2024, 6:26 PM
  • add back forked and fix returned errors
Harbormaster completed remote builds in B59172: Diff 142410.Aug 24 2024, 6:26 PM
hanslu952_gmail.com edited the test plan for this revision. (Show Details)Aug 31 2024, 12:34 PM

Revision Contents
Changeset List

PathSize
usr.bin/
wall/
2 lines
41 lines
56 lines

Diff 142272

View Options

usr.bin/wall/ttymsg.h

Loading...
View Options

usr.bin/wall/ttymsg.c

Loading...
View Options

usr.bin/wall/wall.c

Loading...