Follow-up Comment #8, bug #66419 (group groff): [comment #7 comment #7:] > I agree that the basic problem is the re-use of code to access completely > different font files. .fp references groff font files which will be parsed > (and only "belong" to groff), so there is a potential attack vector,
Right--that is what I attempted to illustrate in bug #61424.
> but entries in the download file point to actual real postscript fonts
> (available to all programs on the system) which will either be valid and
> work, or invalid and fail, but never in a way which could compromise the
> system. You can of course have valid and malicious postscript fonts which
> when downloaded to a printer cause it to calculate pi to the nth degree as an
> actual postscript program, but checking for "/" in the path would not stop
> that!
Yes. My solution to bug #61424 was not an attempt to prevent such
shenangians, and in fact didn't even consider them, since at that time I
didn't know that _grops_ sneakily re-used `font::open_file()` to get at
PostScript prologue and resource files.
> The download file for devpdf continues to allow full pathnames,
Just so it's on the record, the reason for this is that Deri James wrote
_gropdf_ in Perl, and so it does not have recourse to the facilities of the
internal _libgroff_ or _libdriver_ static libraries. (It _could_, but no
one's ever bothered to construct a Perl XS interface for them, demand for such
appears to be nonexistent, and it might not be worth the effort complicate our
build system sufficiently to support doing so.)
[...]
> If you fear a security hole in the .fp request by all means plug it,
I believe the exhibit in bug #61424 is surprising, dismaying, and demands
highly specialized knowledge of device-independent *roff conventions to
diagnose.
> but it is a poor choice if it prevents paths being included in the grops
> download file
I reiterate: that was not the intention.
I'll be pushing the following changes shortly.
commit d5d90be62656ddaffaa1404cd49dd8aef2b8f52a
Author: G. Branden Robinson <g.branden.robin...@gmail.com>
Date: Thu Nov 7 16:38:41 2024 -0600
[grops]: Improve file-related diagnostics.
* src/devices/grops/ps.cpp (ps_printer::define_encoding):
* src/devices/grops/psrm.cpp (resource_manager::output_prolog)
(resource_manager::supply_resource)
(resource_manager::read_download_file): Align diagnostic message
wording with recent revisions to GNU troff.
* src/devices/grops/ps.cpp (ps_printer::define_encoding): Substantially
improve fatal diagnostic thrown when failing to parse a grops encoding
file, replacing "bad second field" with information a user is more
likely to be able to act on.
Also wrap long source line.
Exhibit:
Let's say we have a text.enc file that is damaged like this.
-Scaron 2
+Scaron AA
Before:
$ echo x | groff -F ./font > /dev/null
grops:./font/devps/text.enc:7: fatal error: bad second field
Now:
$ echo foo | ./build/test-groff -F ./font > /dev/null
grops:./font/devps/text.enc:7: fatal error: invalid encoding file:
expected integer in range 0-255 as second word on line, got 'AA'
Lengthy, yes, but much more helpful to the user.
commit e1077decc0e01bd4e1f308409fa4572317fc8200 (HEAD -> master)
Author: G. Branden Robinson <g.branden.robin...@gmail.com>
Date: Thu Nov 7 17:18:56 2024 -0600
[grops]: Further revise fix for Savannah #61424.
Instead of checking for a nonpositive `errno` after calling
`font::open_file()` and assuming that that means the function rejected
the file name for having a slash character in it, check the file name at
the call site and throw a fatal error there if it contains one.
* src/devices/grops/ps.cpp (ps_printer::define_encoding):
* src/devices/grops/psrm.cpp (resource_manager::output_prolog)
(resource_manager::supply_resource): Do it.
Begins to address <https://savannah.gnu.org/bugs/?66419>. Thanks to Rob
Kolstad for the report and the suggestion.
Exhibit:
Consider the following damaged files.
$ git diff
diff --git a/font/devps/TR b/font/devps/TR
index 91581dfd1..01f2fcb8e 100644
--- a/font/devps/TR
+++ b/font/devps/TR
@@ -15,7 +15,7 @@
name TR
internalname Times-Roman
spacewidth 250
-encoding text.enc
+encoding ./text.enc
ligatures fi fl 0
kernpairs
$ git diff
diff --git a/font/devps/download b/font/devps/download
index 3f77716b6..62d3c012b 100644
--- a/font/devps/download
+++ b/font/devps/download
@@ -2,5 +2,5 @@
# PostScript-name Filename
Symbol-Slanted symbolsl.pfa
-ZapfDingbats-Reverse zapfdr.pfa
+ZapfDingbats-Reverse ./zapfdr.pfa
FreeEuro freeeuro.pfa
Before:
$ echo a | groff -F ./font > /dev/null
grops:<standard input>: fatal error: can't open encoding file
'./text.enc'
$ echo a | GROPS_PROLOGUE=./build/font/devps/prologue groff -F ./font >
/dev/null
grops:<standard input>: fatal error: refusing to traverse directories to
open PostScript prologue file 'grops: ../src/libs/libgroff/errarg.cpp:112:
void errprint(const char*, const errarg&, const errarg&, const errarg&):
Assertion `!arg1.empty()' failed.
groff: error: grops: Aborted (core dumped)
$ printf '\\[lh]\n' | groff -F ./font >/dev/null
grops:<standard input>: error: refusing to traverse directories to open
PostScript resource file 'grops: ../src/libs/libgroff/errarg.cpp:112: void
errprint(const char*, const errarg&, const errarg&, const errarg&): Assertion
`!arg1.empty()' failed.
groff: error: grops: Aborted (core dumped)
(Urp.)
Now:
$ echo a | ./build/test-groff -F ./font > /dev/null
grops:<standard input>: fatal error: a '/' is not allowed in encoding file
name: './text.enc'
$ echo a | GROPS_PROLOGUE=./build/font/devps/prologue ./build/test-groff
-F ./font > /dev/null
grops:<standard input>: fatal error: a '/' is not allowed in PostScript
prologue file name: './build/font/devps/prologue'
$ printf '\\[lh]\n' | ./build/test-groff -F ./font >/dev/null
grops:<standard input>: fatal error: a '/' is not allowed in PostScript
font file name: './zapfdr.pfa'
_______________________________________________________
Reply to this item at:
<https://savannah.gnu.org/bugs/?66419>
_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/
signature.asc
Description: PGP signature
