Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#10826 closed bug (fixed)

[Security] Safe Haskell can be bypassed via annotations

Reported by: spinda Owned by: kanetw
Priority: highest Milestone: 7.10.3
Component: Compiler Version: 7.10.2
Keywords: SafeHaskell Cc: dterei
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: GHC accepts invalid program Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): phab:D1226
Wiki Page:

Description (last modified by spinda)

module Test (hook) where

import System.IO.Unsafe

{-# ANN hook (unsafePerformIO (putStrLn "Woops.")) #-}
hook = undefined
➜  Test ghc -fpackage-trust -XSafe Test_simple.hs 
[1 of 1] Compiling Test_simple      ( Test_simple.hs, Test_simple.o ) [flags changed]
Woops.

Test_simple.hs:4:1:
    System.IO.Unsafe: Can't be safely imported!
    The module itself isn't safe.

GHC ultimately rejects the program due to the System.IO.Unsafe import, but this check doesn't occur until GHC has compiled and run the annotation expression, allowing arbitrary IO operations via unsafePerformIO.

The solution is probably to move the import check from the end of renaming/typechecking to the start.

Change History (15)

comment:1 Changed 4 years ago by spinda

Description: modified (diff)

comment:2 Changed 4 years ago by thomie

Cc: dterei added

comment:3 Changed 4 years ago by spinda

I should note that checking imports after renaming/typechecking, instead of before, also opens up nasty possibilities with QuasiQuoters, since Safe Haskell leaves them enabled (despite disabling the rest of Template Haskell). I have a more involved proof of concept that uses these two in conjunction to both execute arbitrary IO operations and delve into the GHC internals through a QuasiQuoter to mark arbitrary modules as safe.

comment:4 Changed 4 years ago by goldfire

Milestone: 7.10.3
Priority: normalhighest

comment:5 Changed 4 years ago by kanetw

I don't think it's possible to run the import check before typechecking/renaming as it requires a TcGblEnv (see checkSafeImports). You could run rnImports/maybe tcRnImports first, then check safety based on those imports, then do the rest. But I'm not sure whether that'll be sufficient for Safe Haskell. I'll take a closer look tomorrow.

I have a patch that just plain disables annotations under Safe Haskell; if that's acceptable (as a workaround at least) I can post it on Phab.

comment:6 Changed 4 years ago by goldfire

No reason to install a workaround if we can just fix the problem.

But, I must ask: is it possible ever to inspect annotations from Safe Haskell? I think probably not within Haskell, but perhaps someone wants to read .hi files (do these have annotations?) after they're compiled via Safe Haskell. Is this correct?

comment:7 Changed 4 years ago by kanetw

Not as far as I know. They're only accessible via the GHC API or TH and that's unsafe Haskell, never mind that you have to explicitly unhide ghc for the first way.

.hi files do have annotations stored, so you could read them with an external tool.

comment:8 Changed 4 years ago by kanetw

Is there a reason why checkSafeImports etc. are in Hsc and not TcM?

E: looks like package trust is the reason. So moving Safe Haskell checks into TcM so you can run it before running splices/tcAnnotations seems quite annoying.

Last edited 4 years ago by kanetw (previous) (diff)

comment:9 Changed 4 years ago by goldfire

Sounds to me like we should just disable annotations entirely in Safe Haskell. I'm sure someone can be clever enough to sort out the monads, but no one is requesting this feature, to my knowledge. And it would be peculiar (but certainly conceivable) to use Safe Haskell and then inspect .hi files manually (or through --show-iface).

So, unless there are objections: disable annotations in Safe Haskell.

Do please add a note in the release notes about this. Given that it's a safety issue, I think it's reasonable to mark this "merge" so that the fix goes into 7.10.3. But others may differ here, as the change could kill existing non-malicious code.

Also, in the error message that happens when a user tries an annotation in Safe Haskell, I think it would be best to include a link to this ticket, so that users who do want the feature have a nice place to make themselves known.

Many thanks!

comment:10 Changed 4 years ago by kanetw

Owner: set to kanetw

I'll submit my patch in a bit then.

comment:11 Changed 4 years ago by kanetw

Differential Rev(s): phab:D1226
Status: newpatch

comment:12 Changed 4 years ago by Austin Seipp <austin@…>

In 4356dac/ghc:

Forbid annotations when Safe Haskell safe mode is enabled.

For now, this fails compliation immediately with an error. If desired, this
can be a warning that annotations in Safe Haskell are ignored.

Signed-off-by: David Kraeutmann <kane@kane.cx>

Reviewed By: goldfire, austin

Differential Revision: https://phabricator.haskell.org/D1226

GHC Trac Issues: #10826

comment:13 Changed 4 years ago by thomie

Status: patchmerge

comment:14 Changed 4 years ago by bgamari

Resolution: fixed
Status: mergeclosed

Committed to ghc-7.10.

Last edited 4 years ago by bgamari (previous) (diff)

comment:15 Changed 4 years ago by thomie

Keywords: SafeHaskell added
Note: See TracTickets for help on using tickets.