On Fri, Aug 30, 2024 at 09:11:18AM GMT, Stuart Henderson wrote:
> On 2024/08/29 23:07, A Tammy wrote:
> >
> > On 8/19/24 3:24 PM, Chaz Kettleson wrote:
> > > On Mon, Aug 19, 2024 at 03:48:20PM GMT, Omar Polo wrote:
> > >> On 2024/08/17 16:28:35 +0100, Stuart Henderson <s...@spacehopper.org>
> > >> wrote:
> > >>> ok
> > >> Imported
> > >>
> > >> Thank you,
> > >>
> > >> Omar Polo
> > >>
> > > Thanks everyone! Great feedback.
> > >
> > > Below are patches for pledge/unveil for feedback/discussion.
> > >
> > > Here is the approach that was taken:
> > >
> > > - Start with minimal set of promises that did not crash and from review
> > > stdio
> > > rpath - hopm config file, firedns config
> > > wpath - pid file, log file, scanlog file
> > > cpath - pid file, log file, scanlog file
> > > inet
> > > dns
> > > proc - fork (maybe we can remove fork and rc_bg?)
> > > exec - execv on restart
> > > unveil
> > > - Initially unveil nothing
> > > - Remove unneeded chdir (locations are no longer relative)
> > > - Unveil only what is needed if it's needed before main loop
> > > LOGFILE, wc
> > > CONFFILE, r
> > > SCANLOG, wc (only if the option is enabled)
> > > HOPM_BINPATH, x (for execv on restart)
> > > - Reduce promises before main loop
> > > stdio
> > > inet
> > > dns
> > > exec
> > >
> >
> > committed, thanks!
>
> That feels premature
>
> The initial pledge("stdio rpath wpath cpath inet dns proc exec unveil"
> and unveil("/", "") at the start of main are not how pledge/unveil are
> normally used
I'll attribute this to my lack of experience/ignorance. Do you mind
elaborating a bit so I can understand?
My general thought here was since I only needed wpath/cpath for pid/log
files, and I was not going to patch for syslog (still need to write pid
anyway), I would at least unveil for only those files. The idea of
unveil("/", "") just seemed like a sane default from other domains where
a "block all, explicitly allow" makes sense.
>
> What reason is there for removing the chdir?
Again this might be misguided. From the documentation:
https://github.com/ircd-hybrid/hopm/blob/1.1.x/INSTALL.md
By default bin/etc/var are put under prefix which makes it nice to
install on a shell server you do not have root to. The chdir to the
HOPM_PREFIX is then needed since the default configuration files use
relative paths like "var/run/hopm.pid" This was patched to include the
full paths.
Now for the misguided part. Since I was trying to "block all, explicitly
allow" with unveil, the idea of unveiling a whole directory soley to
satisfy a chdir that was unneeded anyway seemed excessive. So I opted to
just remove it.
>
> Why go to the trouble of setting up dynamic unveils when filesystem
> access is disabled with pledge("stdio inet dns exec" anyway?
I think I may have misunderstood the value here in the config stage
before the mainloop of using unveil.
Further, I just noticed looking at the code again that the SSL_CTX_*
will fail without the unveils for keys/certs.
>
> Is there any analysis to show that it doesn't use any syscalls which
> it has pledged not to use other than "it didn't crash"? I think nm -s
> and looking for the library functions used is a good start at that
See below. I did look at this from your original suggestion, though I
may have missed something.
Thanks for the feedback. I'm learning and appreciate the patience.
>
> Also this was committed without bumping REVISION.
>
--
Chaz
$ nm -s hopm | awk '/^ *U / { print $2 }' | column
ERR_error_string gai_strerror
ERR_get_error getaddrinfo
SSL_CTX_check_private_key getnameinfo
SSL_CTX_new getopt
SSL_CTX_set_default_verify_paths getpid
SSL_CTX_set_min_proto_version getrlimit
SSL_CTX_use_PrivateKey_file inet_ntop
SSL_CTX_use_certificate_chain_file inet_pton
SSL_connect localtime
SSL_free malloc
SSL_get0_param memcmp
SSL_get_error memcpy
SSL_library_init memset
SSL_new open
SSL_read optarg
SSL_set_fd pclose
SSL_set_shutdown perror
SSL_set_verify poll
SSL_shutdown popen
SSL_state rand
SSL_write read
SSLv23_client_method realloc
TLS_client_method recv
X509_VERIFY_PARAM_set1_host regcomp
X509_VERIFY_PARAM_set_hostflags regerror
__assert2 regexec
__errno select
__sF send
_csu_finish sendto
_exit setpgid
alarm setrlimit
atexit sigaction
atoi signal
bind sleep
calloc snprintf
chdir socket
clock_gettime sprintf
close srand
connect strcasecmp
dup strchr
dup2 strcmp
execv strcpy
exit strerror
fclose strftime
fcntl strlcat
fflush strlcpy
fgets strlen
fopen strncasecmp
fork strrchr
fprintf strstr
fputs time
free umask
freeaddrinfo vsnprintf
fwrite
