Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle untrusted input #122

Open
mgsloan opened this issue Feb 23, 2018 · 5 comments
Open

Handle untrusted input #122

mgsloan opened this issue Feb 23, 2018 · 5 comments

Comments

@mgsloan
Copy link
Owner

mgsloan commented Feb 23, 2018

I think that the Store class and Peek / Poke / Size should all have an added type parameter or two which customizes the details of serialization. In particular, it should specify whether the input is trusted or untrusted, and whether architecture independent serialization should be used #36. Let's just focus on Trusted vs Untrusted for now.

One tricky thing is handling boxed Vector (). This is tricky because the size of () is 0 when serialized, but when deserialized it takes up space. Haven't tried compiling this, but I think something roughly like the following could work alright (after enabling a zillion extensions):

import GHC.Exts
import GHC.TypeLits
import Data.Type.Bool

data Trusted = InputTrusted | InputUntrusted

type family IsUntrusted (t :: Trusted) :: Bool where
  IsUntrusted InputTrusted = 'False
  IsUntrusted InputUntrusted = 'True

class Store (t :: Trusted) a where
  type IsUnitLike a :: Bool
  -- NOTE: Size / Poke don't need the trusted parameter
  size :: Size a
  poke :: a -> Poke a
  peek :: Peek t a

-- | Disallow unit-like types (zero size) when input is untrusted.
type UnitIsUntrusted (t) (a) :: Constraint =
  If (IsUntrusted t && Not (IsUnitLike a))
     (TypeError (Text "Input is untrusted, but the type " :<>: ShowType a :<> Text " may be size 0 and is used in an unsafe context.")
     ()

instance (UnitIsUntrusted t a, Store a) => Store t (Vector a) where

Main issue is that this is some seriously fancy type level stuff. Not 100% sure if the custom type error would work.

For some other things, like having extra checks for Map invariants, this would be handled by reifying Trusted as a value and using a conditional.

@jm4games
Copy link

Maybe I missed this, but what does the trusted/untrusted get you. Are the wins great enough that it justifies breaking everything?

@mgsloan
Copy link
Owner Author

mgsloan commented Feb 23, 2018

@jm4games The reason for this is that I am potentially reverting some partial safety measures in #123 . The problem is that there is a tradeoff between performance and dealing with untrusted input gracefully. Not sure, but I think that full support for untrusted deserialization will inolve making Peek a bit more complicated to pass around state. The degree of inlining / optimization is fairly sensitive to this, so it'd be best to only pass around this extra state when necessary.

We could actually minimize breakage by having a different name for the class and exporting a constraint synonym type Store = Store' Untrusted a. Then it would only break users that have manual store instances. I suppose they might need to enable ConstraintKinds as well. Inclined to prefer a breaking change so that it encourages people to consider whether or not they trust the input.

@jm4games
Copy link

Ok, I will have to take a look at 123. This only peeked my interest since I happen to have a lot of manual store instances :(

@mgsloan
Copy link
Owner Author

mgsloan commented Feb 23, 2018

As proposed here, the migration would only require you to add a bit of boilerplate per instance

instance Store Thing where
  ...

would become

instance Store t Thing where
  type IsUnitLike Thing = False

An alternative approach to consider for () would actually be to allow it, but force it to be encoded as a single byte when the input is untrusted. This'd be ugly though, because then the trusted and untrusted serialization would differ.

@mgsloan
Copy link
Owner Author

mgsloan commented Jul 6, 2018

I ended up having a different reason for wanting a type parameter on every Store / Size / Peek / Poke, but it could be useful for this in the future - 00d77d5

For now this change is experimental. It breaks the whole API, so I imagine that a polished up version of this that could be merged to this repo would attempt to minimize such breakage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants