From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp1.rz.uni-karlsruhe.de (Debian-exim@smtp1.rz.uni-karlsruhe.de [129.13.185.217]) by krisdoz.my.domain (8.14.3/8.14.3) with ESMTP id o9OGexLW023224 for ; Sun, 24 Oct 2010 12:41:01 -0400 (EDT) Received: from hekate.usta.de (asta-nat.asta.uni-karlsruhe.de [172.22.63.82]) by smtp1.rz.uni-karlsruhe.de with esmtp (Exim 4.63 #1) id 1PA3cv-00019S-Mo; Sun, 24 Oct 2010 18:40:57 +0200 Received: from donnerwolke.usta.de ([172.24.96.3]) by hekate.usta.de with esmtp (Exim 4.71) (envelope-from ) id 1PA3cv-0004a7-Lq for tech@mdocml.bsd.lv; Sun, 24 Oct 2010 18:40:57 +0200 Received: from iris.usta.de ([172.24.96.5] helo=usta.de) by donnerwolke.usta.de with esmtp (Exim 4.69) (envelope-from ) id 1PA3cv-000285-Kk for tech@mdocml.bsd.lv; Sun, 24 Oct 2010 18:40:57 +0200 Received: from schwarze by usta.de with local (Exim 4.71) (envelope-from ) id 1PA3cv-0003nd-Jn for tech@mdocml.bsd.lv; Sun, 24 Oct 2010 18:40:57 +0200 Date: Sun, 24 Oct 2010 18:40:57 +0200 From: Ingo Schwarze To: tech@mdocml.bsd.lv Subject: implement .so Message-ID: <20101024164057.GF20876@iris.usta.de> X-Mailinglist: mdocml-tech Reply-To: tech@mdocml.bsd.lv MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.20 (2009-06-14) Hi, not only Xenocara (i.e. OpenBSD X.org), but various ports as well use .so. I'm not thrilled about that, i think .so is useful for general-purpose typesetting, but rather dumb in a manual formatter. Yet, being even less thrilled about the prospect to rewrite the X.org build system or about patching lots of ports, i made up my mind to implement .so. As a first step, here is a patch to split pdesc (p = partial as opposed to f = full) out of fdesc in main.c, such that it can later be called when encountering .so. Yes, i am aware that Kristaps is about to refactor fdesc as well - but i consider that all the more a reason to first push in what we need in terms of featuritis... ;-) The patch looks bigger than it is, most of the code is just moved, not changed. Somehow, i *feel* like .so is a bad idea from a security perspective, so i'm considering to add some additional checks, see below. The man(1), and thus, mandoc(1) utilities are meant to be called by privileged users, even on untrusted input data - no admin would check the manuals of software installed on a server for malicious content. Even though root is allowed to read each file on the system, he may not want a sensitive file displayed on the screen just because a manual contains a malicious .so directive referring to it. Besides, some operating systems install man(1) SGID or even SUID in order to do caching. I hope we will never do this in OpenBSD, but still, mandoc(1) is not OpenBSD-only, so i don't want to expose other systems to any avoidable risks, either - like symlink attacks. I admit that, discussing this with naddy@, sthen@ and phessler@, they all call me paranoid, and i am unable to provide really compelling exploits based on .so. Besides, OpenBSD groff always supported .so, and it was called from man.conf, though not SGID. Also, naddy@ has a point that people choosing to make man(1) SUID will probably have worse problems than merely .so... Still, an uneasy feeling remains about it. Thoughts - both about the refactoring patch below and about .so security implications? Yours, Ingo ----- Forwarded message from Ingo Schwarze ----- From: Ingo Schwarze Date: Sun, 24 Oct 2010 13:48:41 +0200 To: matthieu@ Cc: todd@, espie@, naddy@ Subject: Re: mandoc and x, we good? Hi Matthieu, > - For the .so, it seems to me that adding .so support to mandoc should > not be too hard to implement. After thinking a bit more and discussing with espie@ and a bit with naddy@, i'm now starting to implement .so for mandoc, very probably taking the following precautions: 1. In case the given path does not resolve to a regular file, skip the .so directive and throw a non-fatal ERROR. 2. In case you are running issetugid(2), skip the .so directive and throw a non-fatal ERROR. 3. In case you are running geteuid(2) root, drop privilege to a new _mandoc user before reading the file, and resume privilege after reading the file, such that you can process additional private files specified on the command line. Thus, i hope we can avoid useless effort reorganizing xenocara manuals. Additionally, various ports appear to use .so, too. Yours, Ingo ----- End forwarded message ----- Index: main.c =================================================================== RCS file: /cvs/src/usr.bin/mandoc/main.c,v retrieving revision 1.49 diff -u -p -r1.49 main.c --- main.c 16 Oct 2010 20:49:37 -0000 1.49 +++ main.c 24 Oct 2010 16:06:18 -0000 @@ -179,6 +179,7 @@ static const char * const mandocerrs[MAN "static buffer exhausted", }; +static void pdesc(struct curparse *); static void fdesc(struct curparse *); static void ffile(const char *, struct curparse *); static int moptions(enum intt *, char *); @@ -392,6 +393,124 @@ read_whole_file(struct curparse *curp, s static void fdesc(struct curparse *curp) { + struct man *man; + struct mdoc *mdoc; + struct roff *roff; + + pdesc(curp); + + man = curp->man; + mdoc = curp->mdoc; + roff = curp->roff; + + if (MANDOCLEVEL_FATAL <= exit_status) + goto cleanup; + + /* NOTE a parser may not have been assigned, yet. */ + + if ( ! (man || mdoc)) { + fprintf(stderr, "%s: Not a manual\n", curp->file); + exit_status = MANDOCLEVEL_FATAL; + goto cleanup; + } + + /* Clean up the parse routine ASTs. */ + + if (mdoc && ! mdoc_endparse(mdoc)) { + assert(MANDOCLEVEL_FATAL <= exit_status); + goto cleanup; + } + if (man && ! man_endparse(man)) { + assert(MANDOCLEVEL_FATAL <= exit_status); + goto cleanup; + } + if (roff && ! roff_endparse(roff)) { + assert(MANDOCLEVEL_FATAL <= exit_status); + goto cleanup; + } + + /* + * With -Wstop and warnings or errors of at least + * the requested level, do not produce output. + */ + + if (MANDOCLEVEL_OK != exit_status && curp->wstop) + goto cleanup; + + /* If unset, allocate output dev now (if applicable). */ + + if ( ! (curp->outman && curp->outmdoc)) { + switch (curp->outtype) { + case (OUTT_XHTML): + curp->outdata = xhtml_alloc(curp->outopts); + break; + case (OUTT_HTML): + curp->outdata = html_alloc(curp->outopts); + break; + case (OUTT_ASCII): + curp->outdata = ascii_alloc(curp->outopts); + curp->outfree = ascii_free; + break; + case (OUTT_PDF): + curp->outdata = pdf_alloc(curp->outopts); + curp->outfree = pspdf_free; + break; + case (OUTT_PS): + curp->outdata = ps_alloc(curp->outopts); + curp->outfree = pspdf_free; + break; + default: + break; + } + + switch (curp->outtype) { + case (OUTT_HTML): + /* FALLTHROUGH */ + case (OUTT_XHTML): + curp->outman = html_man; + curp->outmdoc = html_mdoc; + curp->outfree = html_free; + break; + case (OUTT_TREE): + curp->outman = tree_man; + curp->outmdoc = tree_mdoc; + break; + case (OUTT_PDF): + /* FALLTHROUGH */ + case (OUTT_ASCII): + /* FALLTHROUGH */ + case (OUTT_PS): + curp->outman = terminal_man; + curp->outmdoc = terminal_mdoc; + break; + default: + break; + } + } + + /* Execute the out device, if it exists. */ + + if (man && curp->outman) + (*curp->outman)(curp->outdata, man); + if (mdoc && curp->outmdoc) + (*curp->outmdoc)(curp->outdata, mdoc); + + cleanup: + memset(&curp->regs, 0, sizeof(struct regset)); + if (mdoc) + mdoc_reset(mdoc); + if (man) + man_reset(man); + if (roff) + roff_reset(roff); + + return; +} + + +static void +pdesc(struct curparse *curp) +{ struct buf ln, blk; int i, pos, lnn, lnn_start, with_mmap, of; enum rofferr re; @@ -400,10 +519,6 @@ fdesc(struct curparse *curp) struct mdoc *mdoc; struct roff *roff; - man = NULL; - mdoc = NULL; - roff = NULL; - memset(&ln, 0, sizeof(struct buf)); /* @@ -420,6 +535,8 @@ fdesc(struct curparse *curp) curp->roff = roff_alloc(&curp->regs, curp, mmsg); assert(curp->roff); roff = curp->roff; + mdoc = curp->mdoc; + man = curp->man; for (i = 0, lnn = 1; i < (int)blk.sz;) { pos = 0; @@ -512,7 +629,7 @@ fdesc(struct curparse *curp) continue; } else if (ROFF_ERR == re) { assert(MANDOCLEVEL_FATAL <= exit_status); - goto cleanup; + break; } /* @@ -529,119 +646,19 @@ fdesc(struct curparse *curp) if (man && ! man_parseln(man, lnn_start, ln.buf, of)) { assert(MANDOCLEVEL_FATAL <= exit_status); - goto cleanup; + break; } if (mdoc && ! mdoc_parseln(mdoc, lnn_start, ln.buf, of)) { assert(MANDOCLEVEL_FATAL <= exit_status); - goto cleanup; - } - } - - /* NOTE a parser may not have been assigned, yet. */ - - if ( ! (man || mdoc)) { - fprintf(stderr, "%s: Not a manual\n", curp->file); - exit_status = MANDOCLEVEL_FATAL; - goto cleanup; - } - - /* Clean up the parse routine ASTs. */ - - if (mdoc && ! mdoc_endparse(mdoc)) { - assert(MANDOCLEVEL_FATAL <= exit_status); - goto cleanup; - } - if (man && ! man_endparse(man)) { - assert(MANDOCLEVEL_FATAL <= exit_status); - goto cleanup; - } - if (roff && ! roff_endparse(roff)) { - assert(MANDOCLEVEL_FATAL <= exit_status); - goto cleanup; - } - - /* - * With -Wstop and warnings or errors of at least - * the requested level, do not produce output. - */ - - if (MANDOCLEVEL_OK != exit_status && curp->wstop) - goto cleanup; - - /* If unset, allocate output dev now (if applicable). */ - - if ( ! (curp->outman && curp->outmdoc)) { - switch (curp->outtype) { - case (OUTT_XHTML): - curp->outdata = xhtml_alloc(curp->outopts); - break; - case (OUTT_HTML): - curp->outdata = html_alloc(curp->outopts); - break; - case (OUTT_ASCII): - curp->outdata = ascii_alloc(curp->outopts); - curp->outfree = ascii_free; - break; - case (OUTT_PDF): - curp->outdata = pdf_alloc(curp->outopts); - curp->outfree = pspdf_free; - break; - case (OUTT_PS): - curp->outdata = ps_alloc(curp->outopts); - curp->outfree = pspdf_free; - break; - default: - break; - } - - switch (curp->outtype) { - case (OUTT_HTML): - /* FALLTHROUGH */ - case (OUTT_XHTML): - curp->outman = html_man; - curp->outmdoc = html_mdoc; - curp->outfree = html_free; - break; - case (OUTT_TREE): - curp->outman = tree_man; - curp->outmdoc = tree_mdoc; - break; - case (OUTT_PDF): - /* FALLTHROUGH */ - case (OUTT_ASCII): - /* FALLTHROUGH */ - case (OUTT_PS): - curp->outman = terminal_man; - curp->outmdoc = terminal_mdoc; - break; - default: break; } } - /* Execute the out device, if it exists. */ - - if (man && curp->outman) - (*curp->outman)(curp->outdata, man); - if (mdoc && curp->outmdoc) - (*curp->outmdoc)(curp->outdata, mdoc); - - cleanup: - memset(&curp->regs, 0, sizeof(struct regset)); - if (mdoc) - mdoc_reset(mdoc); - if (man) - man_reset(man); - if (roff) - roff_reset(roff); - if (ln.buf) - free(ln.buf); + free(ln.buf); if (with_mmap) munmap(blk.buf, blk.sz); else free(blk.buf); - - return; } -- To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv