comment #7:

> comment #5:
> > > If the download file containing the path to the postscript
> > > font is only writeable by root I think we can trust the path.
> >
> > That seems unnecessarily restrictive to me.  A user might have
> > a "local" font directory under their $HOME.
>
> But a user maintaining a personal "download" file has to trust the entries they're putting into that file.  If they don't vet their fonts, and something nefarious slips in, no amount of groff restricting pathnames will block that malicious code.

Right.
 

> So as long as a user trusts root and herself, it seems that pathnames in "download" files are safe.  (This is a separate situation from ones within a document--but even there, enabling directory traversal with "-U" would seem to be within that switch's mission.)

That's the point I was trying to make.

$HOME/whatever/download is likely not going to be "only writeable by root".

Hi Phil,

comment #3:

> I think what you are saying is that the groff-specific font metric files (e.g. "desc/TR") should remain where they have always been.  Agreed -- they are specific to groff, and there is no need to change the current system.

I agree with this statement, given its scope.

> The Type 1 fonts (.pfa, and .ps) can be anywhere, and frequently shared with other applications.

I agree with this one too.

> The GROFF_INCLUDE_PATH is to be used to find the PostScript files when needed (which I'm guessing is only when they need to be included in the output of grops).

Deri is the author of gropdf, groff's PDF output driver.  It is written in Perl.  All of groff's other output drivers are written in C++, and moreover use some internal statically linked libraries to leverage code reuse.

> I'm a bit concerned by the statement "inserts the path to the system installed versions into the download file" because the prohibition of paths in the download file is precisely where this all started.

If there existed a system tool called "update-font-download" or similar that groff could rely upon to be run at appropriate times, and which kept a "download" file at some reliable location on the file system, some/all of the problems here would go away.

I reckon we'd trust such a file implicitly.

One of our problems, as I see it, is that we've relied upon such a file without undertaking the work to manage it on a dynamically configured system (meaning: one where PostScript fonts might come and go independently of groff's installation).

Ideally, there's be some common-practice location to find a "download" file for PostScript/TrueType/OpenType fonts, and grops and gropdf would look there by default.  Maybe we'd support a GROFF_FONT_DOWNLOAD environment variable of appropriately bikeshedded name for overriding--and automated testing at build time--this location, and then we could have done with it.

But that's not where we are now.  We "manage" these "download" files in an anemic fashion and search for them only in places that a user reasonably would conclude we have declared proprietary to our programs.  In my opinion this is historical inertia.  We do it this way because that's what James Clark implemented in 1990/1991, possibly before anyone else in the Unix world was generating PostScript files with embedded resources.  (I see that Ghostscript dates back to 1988, but I know nothing of its early history, and little else of it besides, apart from some interesting licensing history.)

I was not aware that a change proposed to forbid paths in .fp third parameter and the "name" written by afmtodit, had also (unintentionally) prevented paths in the grops download file, with no diagnostic message.

The proposed "fix" seems to be addition of a diagnostic message and a discussion on what to call a new environment variable! My preference would be to find a safe way to restore the previous behaviour re. download paths, given the change is unintentional in the first place. If the download file containing the path to the postscript font is only writeable by root I think we can trust the path. Further, for the users safety, it would make sense to always search for a groff font in the system directories first before directories provided by the user, this would prevent masking a system font with a nefarious font from somewhere else. Fortunately, the bug only affects grops, not gropdf, so paths are not forbidden in the its own download file.

Just checking if I am understanding this correctly. GROFF_FONT_PATH will still point to a directory containing groff font resources for each post-processor. Individual directories for each output driver contain groff stuff (DESC, download, groff fonts, i.e. TR) and possibly postscript fonts which are defined in the download file such as freeeuro.pfa.

GROFF_INCLUDE_PATH will hold directories to search when the postscript font resides outside the groff environment, e.g. /usr/share/fonts/Type1, and the entry in the download file only includes a filename, no path.

Normally, when wanting to give access to a system installed Type 1 font for groff, site-font would hold the groff font and download would contain the path to the system installed postscript font. The only time you need to duplicate real font data within the groff environs is when the font needs to be converted to be a type 1 font from .tth, .otf or .woff.

This is is exactly what BuildFoundries does, inserts the path to the system installed versions into the download file, not duplicating the font file. I see no objection to implementing GROFF_INCLUDE_PATH into the groff pantheon of environment variables, but for grops and gropdf it is only used for including files in the output, it is not needed for locating fonts, the download file is sufficient.

My personal road map for gropdf, after adding support for TrueType fonts and friends, includes a way of dynamically building a groff font cache so you can use any font installed on your system and it will build the groff font file on the fly if you have never used the font before, I've figured out all the hows, just the dos now!

Spawned off of bug #64577.

In that ticket, the submitter observed that a change in groff 1.23.0 to make font description file opening logic reject file names with slashes in them (to avoid directory traversal for security [confidentiality] purposes in the event of untrusted input), broke a user's workflow.

In my opinion the root cause of the problem is the grops driver's use of `font::open_file()` to open files that aren't font descriptions.

You might call it a...leaky abstraction (take a drink).

The problem is work-aroundable by performing some path-fu with the `-F` option or `GROFF_FONT_PATH` environment variable.

The downside of those is that they push the cognitive dissonance noted above from the API level (where it's gone unnoticed for decades) up to the user.  Not ideal.

Let's review the call sites at issue, and how they look in our master branch in Git.

src/devices/grops/ps.cpp:

   785  void ps_printer::define_encoding(const char *encoding, int encoding_index)
   786  {
   787    char *vec[256];
   788    int i;
   789    for (i = 0; i < 256; i++)
   790      vec[i] = 0;
   791    char *path;
   792    FILE *fp = font::open_file(encoding, &path);
   793    if (0 /* nullptr */ == fp) {
   794      // If errno not valid, assume file rejected due to '/'.
   795      if (errno <= 0)
   796        fatal("refusing to traverse directories to open PostScript"
   797              " encoding file '%1'");
   798      fatal("can't open encoding file '%1'", encoding);
   799    }
...

What's an encoding file?  grops(1):

    A font description file may also contain a directive
        encoding enc‐file
    which says that the PostScript font should be reencoded using the
    encoding described in enc‐file; this file should consist of a
    sequence of lines of the form
        pschar code
    where pschar is the PostScript name of the character, and code is
    its position in the encoding expressed as a decimal integer; valid
    values are in the range 0 to 255.
...

The encoding file is therefore tightly coupled to a groff font description file, and moreover is of no inherent utility as a resource to be inlined into device-specific output (in this case, PostScript), and I therefore propose no change here.

(Incidentally, I wonder if this is a model we might follow when pursuing an old idea of Werner's to decouple terminal output devices from their character set encodings.  Dave can maybe help locate the ticket we have about that.)

What are the other cases?

src/devices/grops/psrm.cpp:

   306  void resource_manager::output_prolog(ps_output &out)
   307  {
   308    FILE *outfp = out.get_file();
   309    out.end_line();
   310    char *path;
   311    if (!getenv("GROPS_PROLOGUE")) {
   312      string e = "GROPS_PROLOGUE";
   313      e += '=';
   314      e += GROPS_PROLOGUE;
   315      e += '\0';
   316      if (putenv(strsave(e.contents())))
   317        fatal("putenv failed");
   318    }
   319    char *prologue = getenv("GROPS_PROLOGUE");
   320    FILE *fp = font::open_file(prologue, &path);
   321    if (0 /* nullptr */ == fp) {
   322      // If errno not valid, assume file rejected due to '/'.
   323      if (errno <= 0)
   324        fatal("refusing to traverse directories to open PostScript"
   325              " prologue file '%1'");
   326      fatal("failed to open PostScript prologue file '%1': %2", prologue,
   327            strerror(errno));
   328    }
...

Salient facts here appear to be:

1.  A prologue file is required, not merely some optional resource.
2.  We already ship one.
3.  We already have an environment variable for the specific purpose of identifying a user-supplied alternate.

So this doesn't seem in urgent need of change either, even if it is a poor fit with other font description-related stuff.

   346  void resource_manager::supply_resource(resource *r, int rank,
   347                                         FILE *outfp, int is_document)
   348  {
   349    if (r->flags & resource::BUSY) {
   350      r->name += '\0';
   351      fatal("loop detected in dependency graph for %1 '%2'",
   352            resource_table[r->type],
   353            r->name.contents());
   354    }
   355    r->flags |= resource::BUSY;
   356    if (rank > r->rank)
   357      r->rank = rank;
   358    char *path = 0 /* nullptr */;
   359    FILE *fp = 0 /* nullptr */;
   360    if (r->filename != 0 /* nullptr */) {
   361      if (r->type == RESOURCE_FONT) {
   362        fp = font::open_file(r->filename, &path);
   363        if (0 /* nullptr */ == fp) {
   364          // If errno not valid, assume file rejected due to '/'.
   365          if (errno <= 0)
   366            error("refusing to traverse directories to open PostScript"
   367                  " resource file '%1'");
   368          else
   369            error("failed to open PostScript resource file '%1': %2",
   370                  r->filename, strerror(errno));
   371          delete[] r->filename;
   372          r->filename = 0 /* nullptr */;
   373        }
   374      }
   375      else {
   376        fp = include_search_path.open_file_cautious(r->filename);
   377        if (0 /* nullptr */ == fp) {
   378          error("can't open '%1': %2", r->filename, strerror(errno));
   379          delete[] r->filename;
   380          r->filename = 0 /* nullptr */;
   381        }
   382        else
   383          path = r->filename;
   384      }
   385    }
...

I wonder if we shouldn't just collapse the if-else branches on `(r->type == RESOURCE_FONT)` into one, using the latter, the `include_search_path`.

  1090  void resource_manager::read_download_file()
  1091  {
  1092    char *path = 0 /* nullptr */;
  1093    FILE *fp = font::open_file("download", &path);
  1094    if (0 /* nullptr */ == fp)
  1095      fatal("failed to open 'download' file: %1", strerror(errno));
  1096    char buf[512];
  1097    int lineno = 0;
  1098    while (fgets(buf, sizeof(buf), fp)) {
  1099      lineno++;
  1100      char *p = strtok(buf, " \t\r\n");
  1101      if (p == 0 /* nullptr */ || *p == '#')
  1102        continue;
  1103      char *q = strtok(0 /* nullptr */, " \t\r\n");
  1104      if (!q)
  1105        fatal_with_file_and_line(path, lineno, "file name missing for"
  1106                                 " font '%1'", p);
  1107      lookup_font(p)->filename = strsave(q);
  1108    }
  1109    free(path);
  1110    fclose(fp);
  1111  }

Ah, it's our old bedeviling friend the download file.

I think a good case can be made for treating this like the Type 1 font files themselves--in other words, from grops's perspective as an inclusion rather than a font description.  (The significance of this distinction is why I have been such a stickler for the terminological distinction between a "font file" and a "font description file) in groff 1.23.0 documentation.

1.  It needs to be updated to refer to those fonts when any are added/removed from the system.

2.  It can live in a place on the file system that has nothing to do per se with groff.  Phil Chadwick's system and those of countless Debian users keep Type 1 fonts elsewhere.

3.  In principle, it doesn't even need to be groff's job to update such download files when relevant fonts appear on and disappear from the system.

One may perceive that we're dovetailing into the install-font.sh problem, a long-standing bugaboo for groff users.

But that's just step one: reassigning some files from being resolved via `-F` paths to `-I` paths.

Step two is to decide if we want/need an environment variable analog for `-I`, as we do for `-F` in `GROFF_FONT_PATH`.  Bike shed time: I'm uncertain whether to call this `GROFF_INCLUDE_PATH` or `GROPS_INCLUDE_PATH`.

The case for `GROFF_INCLUDE_PATH` is that we don't need different names for different groff programs; the semantics of `-I` are similar or identical everywhere we support it at all.  Also, if gropdf is to behave the same as grops here--and I can't think of any reason it should not, a "generic" variable name is less confusing.  On the downside, this means more work to either (1) implement support for this environment variable in places we currently don't or (2) document the discrepancy.  On the gripping hand, if `include_search_path.open_file_cautious()` works the way I hope it does, and is diligently used everywhere it should be, there's only one place we'd need to make the change: there.

The case for `GROPS_INCLUDE_PATH` is simpler to state.  Quicker, easier, directly attacks the problem reported and only that.  The downside is that it becomes another piece of driver-specific weirdness, a wart of non-orthogonality that makes groff as a system harder to understand for developers and users alike.

I welcome feedback on this disquisition.