Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Check preconditions for fromDistinctAscList #20

Closed
wants to merge 2 commits into from

3 participants

@roconnor

Check preconditions for fromDistinctAscList to prevent

http://justinleitgeb.com/haskell/mind-bending-behavior-for-deserialization-in-haskell/

and the security exploits that will surely follow from it.

@elliottt elliottt referenced this pull request from a commit
@elliottt elliottt Prevent parsing non-ordered containers
This comes from the discussion of this pull request:

#20

It was pointed out that fromList on the containers will do the same check
internally that the pull request implements, so it seemed sensible to just
use that instead of replicate its functionality here.
09dd4f4
@elliottt
Owner

Thanks for the patch.

glguy pointed out to me that fromList on the containers will do the ordered check automatically, and dispatch to fromDistinctAscList when appropriate. As such, I've pushed a change that uses fromList instead, removing additional logic from cereal.

@elliottt elliottt closed this
@roconnor
@elliottt
Owner

Excellent. I've pushed an updated version of cereal as version 0.4.0.1.

@mamash mamash referenced this pull request from a commit in joyent/pkgsrc-wip
szptvlfn Update to 0.4.0.1
changes:
0.4.0.1
 - Prevent parsing non-ordered containers
    This comes from the discussion of this pull request:
    GaloisInc/cereal#20

    It was pointed out that fromList on the containers will do the same check
    internally that the pull request implements, so it seemed sensible to just
    use that instead of replicate its functionality here.
20a96d7
@co-dan

I know that this has been closed and merged, but can someone please explain to me why is this actually a bug?

I can agree that it would be nice to get an error when de-serializing to a wrong type, but IMHO you shouldn't be able just to de-serialize a map from an arbitrary list of tuples.

@roconnor

The bug is that it is wrong to call a function, such as fromDistinctAscList, that has a precondition without either (a) ensuring that the preconditions is met or (b) propagating the precondition out to the documentation of decode. Generally speaking, (a) is preferred to (b). Even better than (a) is to not call such functions at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 19 additions and 4 deletions.
  1. +19 −4 src/Data/Serialize/Get.hs
View
23 src/Data/Serialize/Get.hs
@@ -622,19 +622,31 @@ getTreeOf m = liftM2 T.Node m (getListOf (getTreeOf m))
-- | Read as a list of pairs of key and element.
getMapOf :: Ord k => Get k -> Get a -> Get (Map.Map k a)
-getMapOf k m = Map.fromDistinctAscList `fmap` getListOf (getTwoOf k m)
+getMapOf k m = do
+ l <- getListOf (getTwoOf k m)
+ unless (sorted (map fst l)) (fail "invalid Data.Map")
+ return (Map.fromDistinctAscList l)
-- | Read as a list of pairs of int and element.
getIntMapOf :: Get Int -> Get a -> Get (IntMap.IntMap a)
-getIntMapOf i m = IntMap.fromDistinctAscList `fmap` getListOf (getTwoOf i m)
+getIntMapOf i m = do
+ l <- getListOf (getTwoOf i m)
+ unless (sorted (map fst l)) (fail "invalid Data.IntMap")
+ return (IntMap.fromDistinctAscList l)
-- | Read as a list of elements.
getSetOf :: Ord a => Get a -> Get (Set.Set a)
-getSetOf m = Set.fromDistinctAscList `fmap` getListOf m
+getSetOf m = do
+ l <- getListOf m
+ unless (sorted l) (fail "invalid Data.Set")
+ return (Set.fromDistinctAscList l)
-- | Read as a list of ints.
getIntSetOf :: Get Int -> Get IntSet.IntSet
-getIntSetOf m = IntSet.fromDistinctAscList `fmap` getListOf m
+getIntSetOf m = do
+ l <- getListOf m
+ unless (sorted l) (fail "invalid Data.IntSet")
+ return (IntSet.fromDistinctAscList l)
-- | Read in a Maybe in the following format:
-- Word8 (0 for Nothing, anything else for Just)
@@ -655,3 +667,6 @@ getEitherOf ma mb = do
case tag of
0 -> Left `fmap` ma
_ -> Right `fmap` mb
+
+sorted :: Ord a => [a] -> Bool
+sorted l = and (zipWith (<) l (tail l))
Something went wrong with that request. Please try again.