bugGNU roff - Bugs: bug #55557, [PATCH]...

 
 

bug #55557: [PATCH] gropdf,gpinyin,gperl,glilypond,hyphenex can execute arbitrary commands

Submitter:  Deri James <deri>
Submitted:  Wed 23 Jan 2019 03:59:31 PM UTC
   
 
Category:  General Severity:  6 - Security
Item Group:  Warning/Suspicious behaviour Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.23.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 06 Jan 2021 03:17:42 AM UTC, comment #7: 


commit bd37e19c1a69b553072c153dbae58ec6ff214297 (HEAD -> master, origin/master, origin/HEAD)
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Wed Jan 6 13:58:20 2021 +1100

    ChangeLog: Add entry for Savannah #55557 fix.

    Fixes <https://savannah.gnu.org/bugs/?55557>.

commit 27472b5ae548d3dbe933713d488d676708996253
Author: Colin Watson <cjwatson@debian.org>
Date:   Thu Jan 24 13:39:06 2019 +0000

    Avoid Perl's unsafe "<>" operator.

    The "<>" operator is implemented using the two-argument form of "open",
    which interprets magic such as pipe characters, allowing execution of
    arbitrary commands which is unlikely to be expected.  Perl >= 5.22 has a
    "<<>>" operator which avoids this, but also forbids the use of "-" to
    mean the standard input, which is a facility that the affected groff
    programs document.

    ARGV::readonly would probably also fix this, but I fundamentally dislike
    the approach of escaping data in preparation for a language facility to
    unescape it, especially when the required escaping is as non-obvious as
    it is here.  (For the same reason, I prefer to use subprocess invocation
    facilities that allow passing the argument list as a list rather than as
    a string to be interpreted by the shell.)  So I've abandoned this
    dubious convenience and changed the affected programs to iterate over
    command-line arguments manually using the three-argument form of open.

    This change involves an extra level of indentation, so it's a little
    awkward to review.  It consists of changing this form:

      while (<>) {  # or foreach, which is similar but less efficient
        ...
      }

    ... into this:

      unshift @ARGV, '-' unless @ARGV;
      foreach my $filename (@ARGV) {
        my $input;
        if ($filename eq '-') {
          $input = \*STDIN;
        } elsif (not open $input, '<', $filename) {
          warn $!;
          next;
        }
        while (<$input>) {
          ...
        }
      }

    Local variation: glilypond doesn't need the initial unshift since
    that's already handled in contrib/glilypond/args.pl.

    Fixes: https://bugs.debian.org/920269

    [Commit automerged but altered by GBR to omit changes to gropdf, already
    handled by Deri James in 2fc912f0751320a1fba0094dded38e2df46d1dbe.]


G. Branden Robinson <gbranden>
Group administrator
Thu 05 Nov 2020 12:45:30 PM UTC, comment #6: 

No objection here, so long as the patch to gropdf is excluded, for the reasons given in #4.

Deri James <deri>
Group Member
Thu 05 Nov 2020 07:32:13 AM UTC, comment #5: 

I propose we go ahead and incorporate Colin Watson's patch to his Debian package, which he's had in place for almost 2 years.

Do we need smoke test regressions for the affected programs?

If there's a concrete objection to it, can this bug please be updated with it, or a link to it?

G. Branden Robinson <gbranden>
Group administrator
Wed 21 Oct 2020 11:24:28 AM UTC, comment #4: 

Commit 2fc912f0751320a1fba0094dded38e2df46d1dbe fixed this for gropdf. I remember finding a case where the patch caused a problem with multiple input files.

Deri James <deri>
Group Member
Wed 21 Oct 2020 07:31:32 AM UTC, comment #3: 

Uhhhh, these are kind of important, aren't they?

hyphenex.pl isn't a tool that gets installed but it sure would suck for a groff developer to get pwn3d during build by hyphenation patterns of all things.

G. Branden Robinson <gbranden>
Group administrator
Thu 24 Jan 2019 04:12:37 PM UTC, comment #2: 

I think all the approaches involving careful @ARGV mangling are far too delicate and it's too hard to be certain that they're correct.  My preference would be to avoid <> entirely and use three-argument open, as in the attached patch.

Colin Watson <cjwatson>
Group Member
Wed 23 Jan 2019 11:22:35 PM UTC, comment #1: 

It looks like putting this line in the program, after any command line flags have been handled/removed from ARGV:-

map { $_="< ".$_."\0" } @ARGV;

This does a similar job to the code referenced by Colin at:-

https://metacpan.org/source/DAVIDNICO/ARGV-readonly-0.01/lib/ARGV/readonly.pm

Except that it does not add "./" before any space at the start of the filename, because that will fail if a space is given before a full pathname, it is only useful if a file in the current directory has embedded leading spaces in its filename. Even with the gropdf code as it is currently you would need to pass the filename as "./ filename" to avoid the spaces being ignored.

I believe this line can be added to gropdf, hyphenex, gpinyin, and gperl. I am not sure about glilypond yet, I need a bit more thought on that, since it does a lot of work with ARGV in the program args.pl.

Does anyone have a better idea?

Deri James <deri>
Group Member
Wed 23 Jan 2019 03:59:31 PM UTC, original submission:  

Vincent Lefevre has reported this security problem on the debian bug tracker:-


  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=920269

So I am opening this bug here. It has been discussed on the groff mailing list, here:-

http://lists.gnu.org/archive/html/groff/2019-01/msg00024.html

The problem is explained as:-

"... but providing a "filename" with a pipe character can yield an
arbitrary command execution:

$ touch foo
$ ls foo
foo
$ gropdf "rm foo|"
$ ls foo
ls: cannot access 'foo': No such file or directory
$

The reason is that gropdf is a Perl script that uses the insecure
null filehandle "<>". "

Colin Watson has suggested including code to "clean" the the arguments passed on the gropdf command line. He has also identified other perl scripts which may have a similar problem:-

  $ find -name \*.pl | xargs grep -- '<>'
  ./src/devices/gropdf/gropdf.pl:while (<>)
  ./src/devices/gropdf/gropdf.pl: my $lin=<>;
  ./tmac/hyphenex.pl:while (<>) {
  ./contrib/gpinyin/gpinyin.pl:foreach (<>) {     # get line from input
  ./contrib/gperl/gperl.pl:foreach (<>) {
  ./contrib/glilypond/glilypond.pl: LILYPOND: foreach (<>) {
  ./contrib/glilypond/glilypond.pl:  } # end foreach <>

I shall look at ways of blocking this behaviour.

Deri James <deri>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #46089:  0001-Avoid-Perl-s-unsafe-operator.patch added by cjwatson (20KiB - text/x-patch - Avoid Perl's unsafe "<>" operator)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by cjwatson (Updated the item)
  • -email is unavailable- added by deri (Submitted the item)
  •  

    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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-05-02 gbranden CategoryNone General
    2021-01-06 gbranden CategoryDriver gropdf None
        StatusConfirmed Fixed
        Assigned toderi gbranden
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.23.0
    2020-10-21 gbranden Severity4 - Important 6 - Security
        Summary[PATCH] gropdf can execute arbitrary commands [PATCH] gropdf,gpinyin,gperl,glilypond,hyphenex can execute arbitrary commands
    2020-10-13 barx Carbon-CopyRemoved 93119 -
    2020-10-13 barx Summarygropdf can execute arbitrary commands [PATCH] gropdf can execute arbitrary commands
    2019-01-24 cjwatson Attached File- Added 0001-Avoid-Perl-s-unsafe-operator.patch, #46089

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code