bugGNU roff - Bugs: bug #64612, consider an environment variable...

 
 

bug #64612: consider an environment variable for general resources/inclusions

Submitter:  G. Branden Robinson <gbranden>
Submitted:  Wed 30 Aug 2023 02:25:04 PM UTC
   
 
Category:  General Severity:  3 - Normal
Item Group:  Feature change Status:  Need Info
Privacy:  Public Assigned to:  None
Open/Closed:  Open Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 01 Sep 2023 12:50:49 PM UTC, comment #8: 


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".

G. Branden Robinson <gbranden>
Group administrator
Fri 01 Sep 2023 06:22:54 AM UTC, 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.

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.)

Dave <barx>
Group Member
Thu 31 Aug 2023 01:46:33 PM UTC, comment #6: 

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.)

G. Branden Robinson <gbranden>
Group administrator
Thu 31 Aug 2023 01:32:44 PM UTC, comment #5: 

Hi Deri,

comment #4:

> 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,


Right.  No one was aware of that.  As implementor of the change, I bear the chief blame for overlooking it; I should have checked all call sites of font::open_file(), but either didn't, or did but failed to completely understand the consequences.

> with no diagnostic message.


That part's not true.  A diagnostic was generated, albeit not a very helpful one.  See bug #64577, reported by Phil.


grops:<standard input>:(e.nr):20: error: failed to open PostScript resource 'Type1Fonts/Dot-Matrix.ps': No error: 0

 

> The proposed "fix" seems to be addition of a diagnostic message


...the replacement of an semi-scrutable diagnostic message with a comprehensible one...

> and a discussion on what to call a new environment variable!


Well, we could do without the environment variable and rely solely upon people passing `-I` options to the preprocessor, or requiring the user to stick symbolic links in various places on the file system.  The last is the solution Phil adopted while we work out a long-term fix.

Let's recall what `font::open_file()` is for.  It is for opening files relative to the device-specific directory housing device and font description files: "DESC", "TR", "ZCMI", "EURO", and so on.

If a file lives in, say, /usr/local/share/groff/1.23.0/font/devps/, then `font::open_file()` is what should be used to open it.  We can quibble about the PostScript "prologue" file and/or "text.enc" but I don't see a reason to as long as the default versions of these are adequate to most users' purposes, and this seems to be the case.
 

> 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.


Is the "download" file materially different than "DESC", font description files, "prologue", and "text.enc"?

I submit that it is materially different, because a normal, expected use case is for people to have Type 1 fonts in arbitrary places on the file system and for the contents of "download" to (ideally) be updated synchronously with changes in the populations of those locations.

> 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.  Consider:

/home/branden/share/font/type1/mince.pfa
/home/branden/share/groff/font/devps/MINCE

Many programs I run might use the Type 1 PostScript font "mince.pfa", but only groff programs (really just troff, grops, and gropdf) are going to care about its description in groff_font(5) format.

> 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.


I think this is a separate issue.  Yes, the present issue arose because of security concerns, but I am dubious of accepting a file specification containing "../../../../../../../../" just because it's resolved relative to a directory that is writable only by root (or the invoking user, who might have downloaded an untrusted attachment).  That is the concern of bug #61424.

> Fortunately, the bug only affects grops, not gropdf, so paths are not forbidden in the its own download file.


I think grops and gropdf should behave similarly.  I admit that grops's lack of a "foundry" feature and supported extension to the Foundry file issue is a defect, bug #58969.

It seems to me that this issue is entangled like the dickens with the long-standing and notorious bug #60930.  Perhaps one reason that issue has proven challenging to resolve by volunteers is the amount of conceptual murk and quantity of implementation gaps surrounding it.  (1) People confuse groff font description files with PostScript font files.  (2) People confuse $GROFF_FONT_PATH with a search path for arbitrary PostScript resources.  (3) Whatever best practices exist for storage locations of PostScript font files are insufficiently socialized.  (4) Apparently no one has ever written a tool for management of "download" files with awareness of the preceding point.  If I understand the role of "download" correctly, there's no reason at all for it to be groff's bailiwick.  One might have PostScript fonts one wants to manage (and embed in PostScript documents) without having groff installed at all.  I would have thought that some distribution of TeX solved this problem, but maybe not given the popularity of gigantic, batteries-included, everything-you'll-ever-need distributions of it.  Still, it's a point worth looking into.

So my questions for Deri (and any developer who wants to speak up) are (a) if you regard the "download" file as materially different than "DESC", font description files, "prologue", and "text.enc"; (b) if you regard the PostScript font files as materially different from their corresponding groff font descriptions; (c) whether it is likely that any of these files will be found in different locations; and (d) whether it is desirable for any of them to be, given groff's role in a larger operating system.

Putting into "Need Info" status.

While we're mulling these issues, I notice that `search_path::open_file_cautious()` also seems to have no protection against directory traversal.  This should be tested.

https://git.savannah.gnu.org/cgit/groff.git/tree/src/libs/libgroff/searchpath.cpp?h=1.23.0#n147

G. Branden Robinson <gbranden>
Group administrator
Thu 31 Aug 2023 11:58:25 AM UTC, comment #4: 

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.

Deri James <deri>
Group Member
Thu 31 Aug 2023 01:18:24 AM UTC, 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.
 
The Type 1 fonts (.pfa, and .ps) can be anywhere, and frequently shared with other applications.

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).
 
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.  But maybe the scope of my understanding is just too limited.

Phil Chadwick <gpw928>
Wed 30 Aug 2023 11:48:01 PM UTC, comment #2: 

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!

Deri James <deri>
Group Member
Wed 30 Aug 2023 06:18:54 PM UTC, comment #1: 

original submission:

> 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.


Bug #60536, but it merely states the idea, and contains hardly more information than what you've said here.

Dave <barx>
Group Member
Wed 30 Aug 2023 02:25:04 PM UTC, original submission:  

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.

G. Branden Robinson <gbranden>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gpw928 (Posted a comment)
  • -email is unavailable- added by deri (Posted a comment)
  • -email is unavailable- added by barx (Posted a comment)
  • -email is unavailable- added by gbranden (Submitted the item)
  • -email is unavailable- added by gbranden (reporter of parent issue, gropdf architect)
  • -email is unavailable- added by gbranden (reporter of parent issue, gropdf architect)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-08-31 gbranden StatusNone Need Info
    2023-08-30 gbranden Carbon-Copy- Added -email is unavailable-
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code