Hello,
I was looking for a nice IRC client and found catgirl which was
exactly was I looking for, thanks for that! I've prepared a port
for catgirl, waiting to be reviewed by peers [0]. I found about the
-R flag to switch to restricted mode and for me, it's the perfect
opportunity to add some OpenBSD specific syscalls to limit filesystem
access and restrict system calls, see pledge [1] and unveil [2].
In the past, I did a similar work on irssi but as you may read in
this mail thread [3], it was breaking irssi behavior and wasn't
acceptable. Forking irssi was a solution but it was too much work
to keep the fork in sync with the upstream project. catgirl -R
is a good opportunity to add these features without changing
expected behavior.
I have a work in progress patch (attached to this mail) which
work, at the exception of the absence of the configuration or
data directory. The directories must exists before using unveil()
on them.
I am not sure how to do it correctly without adding too much code,
I could add a recursive mkdir once I get the config path from
configPath() and the data path from dataPath().
I need to know:
1) you are willing to accept this kind of OS specific patch, if
not, I'd have to fork and keep it in sync. Given it's a small change
that would be a pity to keep a separate project for it.
2) what do you think would be a proper way to create the
directories at this stage of the program?
Regards
Solène
[0] https://marc.info/?l=openbsd-ports&m=160720917817020&w=2
[1] https://man.openbsd.org/pledge
[2] https://man.openbsd.org/unveil
[3] https://marc.info/?t=155992729400002&r=1&w=2
> On Dec 6, 2020, at 07:54, Solene Rapenne <solene@perso.pw> wrote:
>
> Hello,
>
> I was looking for a nice IRC client and found catgirl which was
> exactly was I looking for, thanks for that! I've prepared a port
> for catgirl, waiting to be reviewed by peers [0]. I found about the
Awesome, thanks for letting me know!
> I need to know:
>
> 1) you are willing to accept this kind of OS specific patch, if
> not, I'd have to fork and keep it in sync. Given it's a small change
> that would be a pity to keep a separate project for it.
I don’t mind it. My IRC bouncer project[1] contains code for FreeBSD
capsicum and OpenBSD pledge, with similar path usage.
> 2) what do you think would be a proper way to create the
> directories at this stage of the program?
I don’t think the configuration directories should be created, and
I think at the stage you’ve added the unveil calls, those paths
have already been read and won’t be needed again. However for data
paths, in pounce I iterate over them and unveil, ignoring the ones
that don’t exist. The default path can be created with the dataMkdir
function in xdg.c.
I have another concern with adding pledge/unveil calls though: the
reason I added -R is to run an ssh “kiosk” of catgirl, in which I
think it is important to be able to use the /help command to open
the catgirl(1) manual page. Allowing this would require the “proc”
and “exec” pledge promises, which I think somewhat limits the
usefulness of using pledge to begin with. Thoughts?
[1]: https://git.causal.agency/pounce
On Tue, 8 Dec 2020 20:14:15 -0500
June Bug <june@causal.agency>:
> > On Dec 6, 2020, at 07:54, Solene Rapenne <solene@perso.pw> wrote:
> >
> > Hello,
> >
> > I was looking for a nice IRC client and found catgirl which was
> > exactly was I looking for, thanks for that! I've prepared a port
> > for catgirl, waiting to be reviewed by peers [0]. I found about the
>
> Awesome, thanks for letting me know!
I've been able to import catgirl, reviewers were happy of catgirl.
> > I need to know:
> >
> > 1) you are willing to accept this kind of OS specific patch, if
> > not, I'd have to fork and keep it in sync. Given it's a small change
> > that would be a pity to keep a separate project for it.
>
> I don’t mind it. My IRC bouncer project[1] contains code for FreeBSD
> capsicum and OpenBSD pledge, with similar path usage.
good :)
> > 2) what do you think would be a proper way to create the
> > directories at this stage of the program?
>
> I don’t think the configuration directories should be created, and
> I think at the stage you’ve added the unveil calls, those paths
> have already been read and won’t be needed again. However for data
> paths, in pounce I iterate over them and unveil, ignoring the ones
> that don’t exist. The default path can be created with the dataMkdir
> function in xdg.c.
indeed, I could use dataMkdir.
I found that catgirl was crashing if ~/.local or ~/.local/share don't
exist. Is it something you would like to fix? I tried on a fresh user
which didn't have such directories.
> I have another concern with adding pledge/unveil calls though: the
> reason I added -R is to run an ssh “kiosk” of catgirl, in which I
> think it is important to be able to use the /help command to open
> the catgirl(1) manual page. Allowing this would require the “proc”
> and “exec” pledge promises, which I think somewhat limits the
> usefulness of using pledge to begin with. Thoughts?
Indeed, adding exec and proc promises + the man binary and man page
paths to unveil would add too much extra code and would also reduce
the benefits of such features.
Then I think there should be separate modes for the kiosk and the
restricted mode, kiosk mode could imply restricted mode. But I don't
like the idea to add this only to please my OpenBSD users needs. This
may be useful though if the kiosk requires some specific extra code
compared to restricted mode.
I think, as a first step, it would be ok to make /help work with
pledge and unveil. They are not supposed to change the behaviour
of programs, so if /help is expected to work with -R, I missed it
and I should fix it.
> On Dec 10, 2020, at 07:38, Solene Rapenne <solene@perso.pw> wrote:
>
> I found that catgirl was crashing if ~/.local or ~/.local/share don't
> exist. Is it something you would like to fix? I tried on a fresh user
> which didn't have such directories.
I don’t think I want to create those directories (though probably
some other programs must be doing so), since they’re potentially
user-configured through environment variables, and if they’re
misconfigured I’d rather fail than silently create a wrong path.
> Indeed, adding exec and proc promises + the man binary and man page
> paths to unveil would add too much extra code and would also reduce
> the benefits of such features.
I’ve been experimenting with this and came up with this patch[1].
In non-restricted mode, I just call pledge, which doesn’t accomplish
much, but I figure I may as well. In restricted mode, I further use
unveil on the required config and data paths, as well as /usr/bin/man.
What do you think?
[1]: https://git.causal.agency/catgirl/commit/?id=df280aa7d658163adae378b468db6fe884bbe02e
On Sun, 10 Jan 2021 21:54:39 -0500
June Bug <june@causal.agency>:
> > On Dec 10, 2020, at 07:38, Solene Rapenne <solene@perso.pw> wrote:
> >
> > I found that catgirl was crashing if ~/.local or ~/.local/share don't
> > exist. Is it something you would like to fix? I tried on a fresh user
> > which didn't have such directories.
>
> I don’t think I want to create those directories (though probably
> some other programs must be doing so), since they’re potentially
> user-configured through environment variables, and if they’re
> misconfigured I’d rather fail than silently create a wrong path.
>
> > Indeed, adding exec and proc promises + the man binary and man page
> > paths to unveil would add too much extra code and would also reduce
> > the benefits of such features.
>
> I’ve been experimenting with this and came up with this patch[1].
> In non-restricted mode, I just call pledge, which doesn’t accomplish
> much, but I figure I may as well. In restricted mode, I further use
> unveil on the required config and data paths, as well as /usr/bin/man.
> What do you think?
>
> [1]: https://git.causal.agency/catgirl/commit/?id=df280aa7d658163adae378b468db6fe884bbe02e
hello,
thank you very much to have worked on this. I really like your code
here, except you want to move pledge call under the unveil calls
so you can remove "unveil" from the pledge list. Otherwise that
looks fine to me, I didn't have the opportunity to test it yet.
> On Jan 11, 2021, at 03:19, Solene Rapenne <solene@perso.pw> wrote:
>
> thank you very much to have worked on this. I really like your code
> here, except you want to move pledge call under the unveil calls
> so you can remove "unveil" from the pledge list. Otherwise that
> looks fine to me, I didn't have the opportunity to test it yet.
I wrote it that way for the early return if not in restricted mode,
since as I understand it unveil(NULL, NULL) accomplishes the same
thing.
The patch is included in the latest release:
<https://git.causal.agency/catgirl/tag/?h=1.5>.