unveil and renameat
Today I finally got to try unveil(2) and retrofit it into one of
my applications. I really like it.
But there was one thing that tripped me up for a bit.
When trying to move a file from one directory into a subdirectory
I kept getting an ENOENT when trying to accomplish this with
renameat(2). Attached is a little sample program that shows the
behavior.
You can see the problem when trying to move the source file from
the current dir into the test dir:
$ cc unveilrenameat.c && ./a.out . unveilrenameat.c test
a.out: moving ./unveilrenameat.c to test/unveilrenameat.c
a.out: renameat: No such file or directory
Instead I would have expected it to have moved the source file into
the test directory, this is what it does when using rename(2)
instead.
Besides this nitpick, thanks for unveil!
-Tim
#include
#include
#include
#include
#include
#include
#include
int
main(int argc, char **argv)
{
const char *basedir, *file, *subdir;
int fd1, fd2;
if (argc != 4)
errx(1, "usage: %s basedir file subdir", getprogname());
basedir = argv[1];
file = argv[2];
subdir = argv[3];
warnx("moving %s/%s to %s/%s", basedir, file, subdir, file);
if (unveil(basedir, "rwc") == -1)
err(1, "unveil");
if (unveil(NULL, NULL) == -1)
err(1, "unveil");
if ((fd1 = open(basedir, O_RDONLY)) == -1)
err(1, "open");
if (mkdirat(fd1, subdir, 0755) == -1 && errno != EEXIST)
err(1, "mkdirat");
if ((fd2 = openat(fd1, subdir, O_DIRECTORY|O_RDONLY)) == -1)
err(1, "openat");
if (renameat(fd1, file, fd2, file) == -1)
err(1, "renameat");
return 0;
}
unveil prototypes
Hi,
I have found more unveil functions that are used in separate C
files. They should have common prototypes in the namei.h header.
It is #ifdef _KERNEL and survived a make build.
ok?
bluhm
Index: kern/kern_unveil.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_unveil.c,v
retrieving revision 1.30
diff -u -p -r1.30 kern_unveil.c
--- kern/kern_unveil.c 1 Aug 2019 15:09:25 - 1.30
+++ kern/kern_unveil.c 1 Aug 2019 16:55:14 -
@@ -43,9 +43,6 @@
#define UNVEIL_MAX_VNODES 128
#define UNVEIL_MAX_NAMES 128
-struct unveil *unveil_lookup(struct vnode *vp, struct proc *p,
-ssize_t *position);
-
static inline int
unvname_compare(const struct unvname *n1, const struct unvname *n2)
{
Index: kern/vfs_syscalls.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/vfs_syscalls.c,v
retrieving revision 1.327
diff -u -p -r1.327 vfs_syscalls.c
--- kern/vfs_syscalls.c 25 Jul 2019 01:43:21 - 1.327
+++ kern/vfs_syscalls.c 1 Aug 2019 16:53:35 -
@@ -90,11 +90,6 @@ int doutimensat(struct proc *, int, cons
int dovutimens(struct proc *, struct vnode *, struct timespec [2]);
int dofutimens(struct proc *, int, struct timespec [2]);
int dounmount_leaf(struct mount *, int, struct proc *);
-int unveil_add(struct proc *, struct nameidata *, const char *);
-void unveil_removevnode(struct vnode *vp);
-void unveil_free_traversed_vnodes(struct nameidata *);
-ssize_t unveil_find_cover(struct vnode *, struct proc *);
-struct unveil *unveil_lookup(struct vnode *, struct proc *, ssize_t *);
/*
* Virtual File System System Calls
Index: sys/namei.h
===
RCS file: /data/mirror/openbsd/cvs/src/sys/sys/namei.h,v
retrieving revision 1.41
diff -u -p -r1.41 namei.h
--- sys/namei.h 27 Jul 2019 21:15:36 - 1.41
+++ sys/namei.h 1 Aug 2019 16:55:35 -
@@ -204,6 +204,11 @@ void nchinit(void);
struct mount;
void cache_purgevfs(struct mount *);
+int unveil_add(struct proc *, struct nameidata *, const char *);
+void unveil_removevnode(struct vnode *);
+void unveil_free_traversed_vnodes(struct nameidata *);
+ssize_t unveil_find_cover(struct vnode *, struct proc *);
+struct unveil *unveil_lookup(struct vnode *, struct proc *, ssize_t *);
void unveil_start_relative(struct proc *, struct nameidata *);
void unveil_check_component(struct proc *, struct nameidata *, struct vnode *);
int unveil_check_final(struct proc *, struct nameidata *);
Re: printf(1) man page has a small omission
Hi Andras, please do not cross-post on OpenBSD lists, choose whatever list fits best. I trimmed bugs@ for this followup. On Mon, Jun 3, 2019 at 2:12 PM Andras Farkas wrote: > https://man.openbsd.org/man1/printf.1 > The section on the b format (%b) neglects to mention that for that > format, it's \0num rather than \num > Because of the way OpenBSD's printf is made, both \0num and \num work > for %b, but, \0num is more correct and portable when using printf(1)'s > %b format. > https://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html I agree that deserves describing, just in case some system ever implements the standard and only the standard. Andras Farkas wrote on Mon, Jul 29, 2019 at 11:54:18PM -0400: > I have a diff attached which fixes the man page. I don't quite agree with your patch. In practice, both \0num and \num work; i inspected the code of FreeBSD and NetBSD which both appear to support \num, too, even though they don't document it, and i tested on Linux, and on Solaris 9, 10, and 11, and both forms work everywhere: $ printf '%b\n' '\0176x' ~x $ printf '%b\n' '\176x' ~x So here is an alternative patch. OK? Ingo Index: printf.1 === RCS file: /cvs/src/usr.bin/printf/printf.1,v retrieving revision 1.32 diff -u -r1.32 printf.1 --- printf.12 Jun 2019 06:16:37 - 1.32 +++ printf.11 Aug 2019 21:53:11 - @@ -319,6 +319,15 @@ Characters from the string .Ar argument are printed with backslash-escape sequences expanded. +In the +.Ar argument , +ASCII characters can be octally encoded either as +.Cm \e0 Ns Ar num +or as +.Cm \e Ns Ar num +like in the +.Ar format +string. If the .Ar argument contains the special escape sequence @@ -373,7 +382,17 @@ .Ev LC_ALL Ns =C were set. .Pp -The escape sequences \ee and \e' are extensions to that specification. +The escape sequences +.Cm \ee +and +.Cm \e' , +as well as omitting the leading digit +.Cm 0 +from +.Cm \e0 Ns Ar num +octal escape sequences in +.Cm %b +arguments, are extensions to that specification. .Sh HISTORY The .Nm
Re: Grammar and style edits to installation guide
On Mon, Jul 29, 2019 at 09:52:48PM -0700, Evan Silberman wrote:
> "T.J. Townsend" wrote:
> > > + You may individually select distribution sets to install
> > > + by entering their names or wildcards (e.g. `*.tgz' or
> > > + `base*|comp*'), or you may enter `all' to select all the
> > > + sets (which is what most users will want to do).
> >
> > > +You should create yourself an account, if you skipped this step during
> > > +installation, and protect it and the "root" account with good passwords.
> >
> > We're mixing these `these' quotes and "normal" ones. It would be great if
> > we could rid ourselves of `these' and ``these'' everywhere.
>
> In the updated patch attached, INSTALL and m4.common are standardized to
> use "ascii straight double quotes" in running text wherever any other
> convention was used. Double quotes are conventional in American English
> style guides.
>
> I haven't done this same pass on the arch-specific content blocks, but
> it should get done.
>
> As an aside, the file "packages" contains guidance that feels outdated.
> It doesn't place the same emphasis that
> https://www.openbsd.org/faq/faq15.html does on preferring the packages
> whenever possible, and the example given suggests the usually
> unnecessary approach of passing the complete path to a fixed version of
> a package to pkg_add. It's not even the current version of the emacs
> package!
>
> > > The installer runs dhclient(8) on the network interface the system
> > > booted from, or in case of multiple interfaces it will ask which one
> > > to use. Upon success it retrieves a response file via HTTP. If that
> >
> > Should there be a comma after success?
>
> There needn't be, but there certainly may be.
>
it;s in, tweaked a bit more. thanks for persevering evan!
jmc
> Index: INSTALL
> ===
> RCS file: /cvs/src/distrib/notes/INSTALL,v
> retrieving revision 1.53
> diff -u -p -r1.53 INSTALL
> --- INSTALL 24 Jun 2019 01:21:46 - 1.53
> +++ INSTALL 30 Jul 2019 04:54:35 -
> @@ -11,10 +11,10 @@ OpenBSD is a fully functional, multi-pla
> System based on Berkeley Networking Release 2 (Net/2) and 4.4BSD-Lite.
> There are several operating systems in this family, but OpenBSD
> differentiates itself by putting security and correctness first. The
> -OpenBSD team strives to achieve what is called a 'secure by default'
> +OpenBSD team strives to achieve what is called a "secure by default"
> status. This means that an OpenBSD user should feel safe that their
> -newly installed machine will not be compromised. This 'secure by
> -default' goal is achieved by taking a proactive stance on security.
> +newly installed machine will not be compromised. This "secure by
> +default" goal is achieved by taking a proactive stance on security.
>
> Since security flaws are essentially mistakes in design or implement-
> ation, the OpenBSD team puts as much importance on finding and fixing
> @@ -138,7 +138,7 @@ Using online OpenBSD documentation:
>
> Documentation is available if you first install the manual pages
> distribution set. Traditionally, the UN*X "man pages" (documentation)
> -are denoted by 'name(section)'. Some examples of this are
> +are denoted by "name(section)". Some examples of this are
>
> intro(1),
> man(1),
> @@ -151,8 +151,8 @@ The section numbers group the topics int
> are of primary interest: user commands are in section 1, file formats
> are in section 5, and administrative information is in section 8.
>
> -The 'man' command is used to view the documentation on a topic, and is
> -started by entering 'man [section] topic'. The brackets [] around the
> +The "man" command is used to view the documentation on a topic, and is
> +started by entering "man [section] topic". The brackets [] around the
> section should not be entered, but rather indicate that the section is
> optional. If you don't ask for a particular section, the topic with the
> least-numbered section name will be displayed. For instance, after
> @@ -175,7 +175,7 @@ where "subject-word" is your topic of in
> related man pages will be displayed.
>
>
> -Adding third party software; ``packages'' and ``ports'':
> +Adding third party software; "packages" and "ports":
>
>
> includeit(packages)
> @@ -194,7 +194,7 @@ netiquette is available at
>
> https://www.OpenBSD.org/mail.html
>
> -To report bugs, use the 'sendbug' command shipped with OpenBSD,
> +To report bugs, use the "sendbug" command shipped with OpenBSD,
> and fill in as much information about the problem as you can. Good
> bug reports {:-include-:} lots of details. Additionally, bug reports can
> be sent by mail to:
> Index: m4.common
> ===
> RCS file: /cvs/src/distrib/notes/m4.common,v
> retrieving revision
iked(8): fix lost childsa after rekey issues
Two iked(8) instances sometimes get of sync after rekeying their SAs,
which breaks the IPsec tunnel until the daemons are restarted.
One reason for this seems to be that when one peer succeeds
rekeying the IKE SA while the other is waiting for a response to a Child SA
rekey exchange, the first can no longer decrypt and answer the
Child SA request (because it is encrypted for the old SA).
The diff prevents iked(8) from replying to IKE SA rekey request
while it is waiting for a Child SA response.
Another symptom of this behaviour is that sometimes an iked tries to rekey a
Child SA that the other has lost track of, resulting in a
CHILD_SA_NOT_FOUND notify. The second diff adds a handler for this notify type
so that it no longer waits for a Child SA rekeying reply after receiving
CHILD_SA_NOT_FOUND.
Ok?
Index: ikev2.c
===
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.168
diff -u -p -u -r1.168 ikev2.c
--- ikev2.c 27 Feb 2019 06:33:56 - 1.168
+++ ikev2.c 1 Aug 2019 08:42:35 -
@@ -3637,6 +3637,15 @@ ikev2_resp_create_child_sa(struct iked *
print_map(protoid, ikev2_saproto_map));
if (protoid == IKEV2_SAPROTO_IKE) {
+ if ((sa->sa_stateflags & IKED_REQ_CHILDSA)
+ && !(sa->sa_nexti)) {
+ log_debug("%s: Ignore IKE SA rekey: waiting for Child "
+ "SA response.", __func__);
+ /* Ignore, don't send error */
+ msg->msg_valid = 0;
+ return (0);
+ }
+
/* IKE SA rekeying */
spi = >msg_prop->prop_peerspi;
Index: ikev2_pld.c
===
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2_pld.c,v
retrieving revision 1.70
diff -u -p -u -r1.70 ikev2_pld.c
--- ikev2_pld.c 22 Mar 2018 21:11:49 - 1.70
+++ ikev2_pld.c 1 Aug 2019 08:39:25 -
@@ -1170,6 +1170,15 @@ ikev2_pld_notify(struct iked *env, struc
msg->msg_sa->sa_cpi_out = betoh16(cpi);
}
break;
+ case IKEV2_N_CHILD_SA_NOT_FOUND:
+ if (!msg->msg_e) {
+ log_debug("%s: N_CHILD_SA_NOT_FOUND not encrypted",
+ __func__);
+ return (-1);
+ }
+ /* Stop expecting response */
+ msg->msg_sa->sa_stateflags &= ~IKED_REQ_CHILDSA;
+ break;
case IKEV2_N_MOBIKE_SUPPORTED:
if (!msg->msg_e) {
log_debug("%s: N_MOBIKE_SUPPORTED not encrypted",
Re: SIGSEGV in libedit
> So i think for committing to OpenBSD, each function changed needs > to be inspected and it needs to be confirmed that zeroing each > buffer in question does not cause new problems or hide other > bugs. Good grief. That's very much like saying some of the strcpy functions in the tree were fine. New problems caused by deterministic initialized input? Shocking.
Re: SIGSEGV in libedit
Hello Yasuoka-san, YASUOKA Masahiko wrote on Thu, Aug 01, 2019 at 08:42:35PM +0900: > I noticed the upstream NetBSD recently replaced almost all malloc(3)s > by calloc(3) in libedit. > > https://github.com/NetBSD/src/commit/b91b3c48e0edb116bd797586430cb426b575d717 > > This also fixes the problem. I'll create a diff which does the same > thing. Might i suggest that you first send a diff changing only the one place required to fix the bug asou@ reported? That can easily be reviewed, and i expect we can get it in quickly, and it allows a useful commit message explaining why exactly it is needed. Regarding all the other places in the library: it is true that in many situations, zeroing buffers when allocating them provides additional safety. But there may be exceptions; in some cases, it may hide or rarely even cause bugs. In any case, i hate it when people go "i have no idea whether it is needed and what it does, but let's just change this globally anyway". Even if it's about something as seemingly unconspicious as zeroing buffers. The NetBSD commit message provides no indication that the changed code was inspected for consequences of the change, and as far as i know Christos (admittedly, i only met him two or three times and didn't talk all *that* much to him) i guess it might indeed be his style to commit changes without actually inspecting the code. So i think for committing to OpenBSD, each function changed needs to be inspected and it needs to be confirmed that zeroing each buffer in question does not cause new problems or hide other bugs. That may be somewhat painful work, libedit code is not very audit friendly, but i don't think cutting corners is a good idea. In general, code quality in libedit is somewhat below OpenBSD quality standards, so the time spent auditing it is certainly not wasted but spent at a place needing it. Also, the code quality implies that zeroing some additional buffers likely improves security at least at some of the places - if it is checked properly. Yours, Ingo
Re: SIGSEGV in libedit
Hi,
I noticed the upstream NetBSD recently replaced almost all malloc(3)s
by calloc(3) in libedit.
https://github.com/NetBSD/src/commit/b91b3c48e0edb116bd797586430cb426b575d717
This also fixes the problem. I'll create a diff which does the same
thing.
On Thu, 01 Aug 2019 14:54:20 +0900 (JST)
YASUOKA Masahiko wrote:
> Hi,
>
> Programs using libedit(3) crashe after the program's window size is
> changed. For example,
>
> 1. Invoke ftp
> $ ftp
> ftp>
> 2. Resize its window
> 3. Enter "deb" +
>
> => When the problem occurs, it crashes with a segmentation fault
> => The problem doesn't occur, it displays "debug"
>
> The problem happens once in 5-30 times.
>
> See the problem by gdb.
>
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0 0x162ce7bd3ff2 in re_update_line (el=0x162cfa59d800,
> old=0x162c3e478e00 L"deb", '\xdfdfdfdf' Cannot access memory at address 0x162c3e479000>, new=0x162c3854ae00 L"ftp>
> debug", i=0)
> at /home/yasuoka/src/lib/libedit/refresh.c:518
> 518 while (*o)
> (gdb) bt
> #0 0x162ce7bd3ff2 in re_update_line (el=0x162cfa59d800,
> old=0x162c3e478e00 L"deb", '\xdfdfdfdf' Cannot access memory at address 0x162c3e479000>, new=0x162c3854ae00 L"ftp>
> debug", i=0)
> at /home/yasuoka/src/lib/libedit/refresh.c:518
> #1 0x162ce7bd3c99 in re_refresh (el=0x162cfa59d800) at
> /home/yasuoka/src/lib/libedit/refresh.c:298
> #2 0x162ce7beba1c in el_wgets (el=0x162cfa59d800,
> nread=0x7f7e1c6c) at /home/yasuoka/src/lib/libedit/read.c:577
> #3 0x162ce7be700a in el_gets (el=0x162cfa59d800, nread=0x7f7e1c6c)
> at /home/yasuoka/src/lib/libedit/eln.c:74
> #4 0x162a32a19c60 in ?? ()
> #5 0x162a32a19817 in ?? ()
> #6 0x162a32a0b142 in ?? ()
> #7 0x in ?? ()
> (gdb) p el->el_display[0]
> $1 = 0x162c3e478e00 L"deb", '\xdfdfdfdf' access memory at address 0x162c3e479000>
> (gdb)
>
> When is entered, as the result of the completion, libedit calls
> re_update_line() <= re_reresh() to refresh the line. In that
> function, it tries to find a NUL char in the line buffer, but the line
> buffer doesn't include any NUL, then the crash happens.
>
> After the window size change, the line buffer is freeed and allocated
> again at terminal_alloc_display(). (These are done by signal
> handler. This is a separated problem)
>
> 409 static int
> 410 terminal_alloc_display(EditLine *el)
> 411 {
> 412 int i;
> 413 wchar_t **b;
> 414 coord_t *c = >el_terminal.t_size;
> 415
> 416 b = reallocarray(NULL, c->v + 1, sizeof(*b));
> 417 if (b == NULL)
> 418 goto done;
> 419 for (i = 0; i < c->v; i++) {
> 420 b[i] = reallocarray(NULL, c->h + 1, sizeof(**b));
> 421 if (b[i] == NULL) {
> 422 while (--i >= 0)
> 423 free(b[i]);
> 424 free(b);
> 425 goto done;
> 426 }
> 427 }
> 428 b[c->v] = NULL;
> 429 el->el_display = b;
>
> The line buffers are el->el_display[] and they are allocated by
> reallocarray(). Then they are initialized at re_clear_display().
>
> 1149 protected void
> 1150 re_clear_display(EditLine *el)
> 1151 {
> 1152 int i;
> 1153
> 1154 el->el_cursor.v = 0;
> 1155 el->el_cursor.h = 0;
> 1156 for (i = 0; i < el->el_terminal.t_size.v; i++)
> 1157 el->el_display[i][0] = '\0';
> 1158 el->el_refresh.r_oldcv = 0;
> 1159 }
>
> Remark that only first char is set NUL, but the rest part is kept
> uninitialized. Then user inputs "deb". re_fastputc() is called for
> each char.
>
> 1053 re_fastputc(EditLine *el, wint_t c)
> 1054 {
> 1055 wchar_t *lastline;
> 1056 int w;
> 1057
> 1058 w = wcwidth(c);
> 1059 while (w > 1 && el->el_cursor.h + w > el->el_terminal.t_size.h)
> 1060 re_fastputc(el, ' ');
> 1061
> 1062 terminal__putc(el, c);
> 1063 el->el_display[el->el_cursor.v][el->el_cursor.h++] = c;
>
> the function just put the char like #1063, then the buffer becomes it
> doesn't include any NUL char like below.
>
> deb +
>
> After this, by calling re_reresh() the crash happens.
>
> I had looked into the code deeply, but it didn't become clear how
> should we fix this problem. But I suppose the code assumes the entire
> buffer is initialized, so the following diff replaces the code which
> initializes only first char by the code which calls
> re__copy_and_pad().
>
> comment?
> ok?
>
> Index: lib/libedit/refresh.c
> ===
> RCS file: /var/cvs/openbsd/src/lib/libedit/refresh.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 refresh.c
> --- lib/libedit/refresh.c 11 Oct 2018 15:19:09
