Opened 3 years ago

Last modified 3 weeks ago

#6017 new task

Reading ./.ghci files raises security issues

Reported by: nomeata Owned by:
Priority: normal Milestone: 8.0.1
Component: GHCi Version: 7.4.1
Keywords: Cc: peter.minten@…, hvr
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Other Test Case:
Blocked By: Blocking:
Related Tickets: #8248 Differential Rev(s):


GHCi will execute .ghci files in the current directory, and this can be used to run arbitrary shell commands.

It seems to me that most people would not expect that running "ghci" in a directory can cause arbitrary commands to be executed. This could be a security issue, e.g. running ghci in a just downloaded software package with a rouge .ghci file.

Also it affects invocations "ghc -e", which conceivably could be used in aliases or scripts for some action unrelated to running a ghci session, as in

I just noticed that it will not read files in directories not owned by you and warn you about it (e.g. in /tmp), which is a good start. But this does not help against files in packaged repositories.

Maybe ghci could keep a white-list of files somewhere in ~/.ghci and ask if it should execute a .ghci file that has not been encountered before.

Alternatively (and more work) a safe subset of options (such as inclusion paths) could be identified and only those would be allowed in ./.ghci, while ~/.ghci allows all commands.

Attachments (2)

Change History (19)

comment:1 Changed 3 years ago by igloo

  • difficulty set to Unknown
  • Milestone set to 7.8.1
  • Priority changed from normal to high

comment:2 Changed 3 years ago by pminten

  • Owner set to pminten

comment:3 Changed 3 years ago by pminten

  • Status changed from new to patch

Here's a patch that adds a whitelist/blacklist mechanism. When ghci encounters a .ghci file in the current directory and the current directory is not the home dir (which is trusted) and the file does not match one of the -ghci-script arguments ghci will look in ~/.ghc/ghci_blacklist and then in ~/.ghc/ghci_whitelist. If it finds the .ghci file's path there it will respectively not load and load the file. If the .ghci file is in neither list the user will be asked.

If "ghc -e" is used no messages will be printed and no questions asked. Unknown .ghci files are treated as blacklisted.

I don't have a testsuite update for this patch, wouldn't know how to test this automatically.

comment:4 Changed 3 years ago by pminten

  • Cc peter.minten@… added

comment:5 Changed 3 years ago by nomeata

Thanks for taking this issue serious; the approach you describe sounds sane.

comment:6 Changed 3 years ago by igloo

Hmm, I think there are 5 things we might want to do with a .ghci file:

  1. Silently ignore it
  2. Silently execute it
  3. Warn the user that we are ignoring it
  4. Warn the user that we are executing it
  5. Ask the user what to do

although I'm not sure (4) is useful. Presumably "ghci -v" would tell you anyway.

Perhaps we should have a ~/.ghc/ghci.config and ~/.ghc/ghc-e.config in which you can say one of

evaluate-dot-ghci: no
evaluate-dot-ghci: yes
evaluate-dot-ghci: warn-no
evaluate-dot-ghci: warn-yes
evaluate-dot-ghci: ask

and likewise a way to white/blacklist particular paths in those files? Default should probably be warn-no, and perhaps when creating the file initially we should by default add an entry whitelisting ~/.ghci?

If we have a way to whitelist filenames, it would be straightforward to allow filenames other than .ghci to be whitelisted, so for example you could whitelist "/foo/bar/ghci-config" if you wanted to have ghci commands with a non-dotfile filename in a project.

comment:7 Changed 3 years ago by pminten

There is also the question what you want to check with the blacklist/whitelist mechanism. The .ghci files can be divided into three categories: explicitely passed (-ghci-script), standard location (~/.ghci, ~/.ghc/ghci.conf) and current directory. Also .ghci files can source other .ghci files using :script (with arbitrary names, don't need to be called .ghci obviously).

The patch simply says that if only files in the current directory are a threat and that if the user approves such a file the trust in that file cascades to whatever files are sourced.

With the patch the "/foo/bar/ghci-config" in your example wouldn't need to be whitelisted because the only way for it to be loaded is through -ghci-script (in which case it would be trusted) or by another file (in which case it would inherit the trustedness).

Having a way to configure the default blacklist approach is a good idea but if the blacklist is expanded as you seem to suggest a single knob probably won't suffice. There's a very good chance the user doesn't want the blacklist mechanism to ask for files (s)he explicitly requests to be loaded. So you'd get at least two settings. But you may also want to have a knob for the files loaded by .ghci files, the user may not appreciate being asked for every included file.

There would be at least 4 knobs. But I suspect all but one would have a default that nobody changes. For files the user explicitly passes and files included by those the default would be allow. For files included by not automatically trusted files the default would be to allow them if the including file is allowed (if that file can be nasty you already have the security problem). So only the not automatically trusted files (the .ghci files) in the current directory would need a knob.

One could imagine that files on a blacklist are rejected even if the user asks for them, this could be a knob too.

With this reasoning you'd get at most 2 knobs and a lot of hardcoded behaviour. Of course if there are situations where you'd want something different than the defaults indicated above the reasoning doesn't apply.

comment:8 Changed 3 years ago by simonmar

Let's not go overboard here. Even if we do a whitelist, someone will point out that we should be adding hashes of the .ghci file to the whitelist and failing if the hash doesn't match.

If people think that this is really a security problem (and I'm not convinced it is, e.g. gdb reads .gdbinit unconditionally), then we can just switch the default to not read .ghci files in the current directory, and add a flag to enable it (-ignore-dot-ghci ignores all, but we want a way to just ignore the one in the current directory). If you want to read a specific .ghci by default then there are lots of ways to do it: add some code to your ~/.ghci to implement an explicit whitelist, or invoke ghci via a script or a shell alias.

comment:9 Changed 3 years ago by igloo

  • Owner pminten deleted
  • Status changed from patch to new

How do you conditionally source a script from the ~/.ghci file?

But that makes me think: why don't we make ghci never execute anything other than ~/.ghci? People who want that functionality can just put something like :source .ghci_cmds in their ~/.ghci, and then the security implications are not our problem.

comment:10 Changed 19 months ago by Simon Marlow <marlowsd@…>

In a6f2c852d49313fa8acea2deb3741ab86c6ef995/ghc:

Don't perform permission checks for scripts named with -ghci-script (#6017)

The user explicitly requested this script on the command-line, so it's
unnecessary to require that the script is also owned by the user.
Also, it is currently impossible to make a GHCi wrapper that invokes a
custom script without first making a copy of the script to circumvent
the permissions check, which seems wrong.

comment:11 Changed 18 months ago by thoughtpolice

  • Cc hvr added
  • Resolution set to fixed
  • Status changed from new to closed

This is now fixed, and also merged to the 7.8 branch.

comment:12 Changed 18 months ago by thoughtpolice

  • Resolution fixed deleted
  • Status changed from closed to new

Whoops, sorry. I misread the intention of the ticket, here.

comment:13 Changed 17 months ago by thoughtpolice

  • Milestone changed from 7.8.3 to 7.10.1

Bumping priority down (these tickets haven't been closely followed or fixed in 7.4), and moving out to 7.10 and out of 7.8.3.

comment:14 Changed 17 months ago by thoughtpolice

  • Priority changed from high to normal

Actually dropping priority. :)

comment:15 Changed 10 months ago by thomie

comment:16 Changed 10 months ago by thoughtpolice

  • Milestone changed from 7.10.1 to 7.12.1

Moving to 7.12.1 milestone; if you feel this is an error and should be addressed sooner, please move it back to the 7.10.1 milestone.

comment:17 Changed 3 weeks ago by thoughtpolice

  • Milestone changed from 7.12.1 to 8.0.1

Milestone renamed

Note: See TracTickets for help on using tickets.