implement .so

[Ingo Schwarze <schwarze@usta.de>]

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 <schwarze@usta.de> -----

From: Ingo Schwarze <schwarze@usta.de>
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