Update stackage to 0.19 to avoid people falling prey of getRandomBytes lack of randomness? #96

Closed
jsdw opened this Issue Aug 20, 2016 · 19 comments

Projects

None yet

6 participants

@jsdw
jsdw commented Aug 20, 2016 edited

The latest version of cryptonite available on stackage is 0.15 (as of LTS 6.12). However, I spotted a bug which seems to be resolved in 0.19 whereby getRandomBytes is not random (which seems quite serious to me, but please correct me if I'm holding it wrong!). As an example, in ghci I do:

import Crypto.Random (getRandomBytes)
ByteChars.unpack <$> getRandomBytes 300

and the response I get back is always something like:

"\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225\235J\219 \192C\179\225C\ENQ\137\187"

This no longer seems to be an issue in 0.19, but as 0.15 is in the latest LTS I'm worried by how many users might be falling prey of this presently. The next best thing might be to put a warning in the readme telling people to manually add cryptonite-0.19 to their stack.yamls extra-deps field or somesuch.

@vincenthz
Member

I think we can envision making point releases for security reasons for lts'es, unfortunately I dont have a way to make any until next monday. Otherwise I dont think there s much changes to move to 0.19, so if someone from stackage can upgrade lts that could go faster (pinging @snoyberg)

@vincenthz
Member

Otherwise pinging @centromere @tekul, anyone of you interested by making a ..1 ?

@vincenthz
Member

markdown ate the comment .lts-ver.1

@tekul
Member
tekul commented Aug 22, 2016

OK, I'm on it.

@snoyberg snoyberg referenced this issue in fpco/lts-haskell Aug 22, 2016
Closed

Upgrade to newer cryptonite #24

@snoyberg

I've opened fpco/lts-haskell#24

@tekul
Member
tekul commented Aug 22, 2016 edited

I made a 0.15.1 release, backporting rdrand and other security-related fixes. There is a problem in that my machine doesn't have the issue which means I can't check it. Could someone (e.g. @jsdw) please check that 0.15.1 does indeed resolve the issue?

As such I have a few observations/recommendations wrt the commit log and so on, as IMO the git commit log should be a super-useful resource for cutting releases like this. Ours is a bit lacking, so this was harder and more error prone than it could have been, since I couldn't test the fix:

  • If you are changing any kind of code or build behaviour, use detailed commit log messages to explain what was done and why.
  • Make it obvious when you are not changing code. If, for example, you are updating the CHANGELOG to say "Fixed X", don't use "Fixed X" as the commit message. Use "Updated CHANGELOG" or something similar.
  • Stick to git log conventions for readability
  • Code changes should preferably be linked to an issue (not essential if the log is detailed enough)
  • We should probably ask people to tidy up large feature branches with lots of commits before merging

Tim Pope's blog article on git logs and commit messages is worth a read.

@centromere
Member

@tekul Do you know what the precise cause of the problem was?

@vincenthz vincenthz added the security label Aug 22, 2016
@vincenthz
Member

Looks like the encoded instruction rdrand has become invalid maybe after the i386 fix

@tekul
Member
tekul commented Aug 22, 2016

@centromere No, I just know it was related to the rdrand changes. Any chance you can confirm whether you see a problem with 0.15 which is fixed with 0.15.1?

@centromere
Member

@tekul I am unable to reproduce the issue with 0.15.

$ ghci -package=cryptonite -package-db=./.cabal-sandbox/x86_64-linux-ghc-8.0.1.20160725-packages.conf.d/
GHCi, version 8.0.1.20160725: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/alex/.ghci
λ> import Crypto.Random (getRandomBytes)
λ> import qualified Data.ByteString.Char8 as ByteChars
λ> ByteChars.unpack <$> getRandomBytes 300
"\149w\EMj\152t\249\176(bx\216<\n\147\240\CAN\245\219\180\\\215q\145L\178\139\GSU\233Q\ns\153\ACK\250\141\223\163\EM5\172\&3\236\144\186\221\173\177L\238\195\&8\165R\139\tj\246\192v\USo\188\SOH8\202\212\196\&8x\235\149\SUB\148?\175MUu\224C\233\195\a\129Qb\240\203\&9p$\NUL\224\\\130\206K\EM\244#\RSoj#\221@\180\132\253\216\166\242O\156\ACK\228\DC2\182\147\173~#\169\&6?\192!\NAK\145\&7\STX\CAN2\143|\157\NUL\159$cT\157\153_\226\196\EOT\153\240\212\133\234\249\158\&8\185\186/\184\198\253\220z\138\204bq\224T\253\ACKgfJ\b\NUL_1#\184n-\ETB\FS\228a\DC3\147\158u\SI$\208\251:\174@l\235\134\176\219\162<\158\190\130Mq\246w\DC3\243\235M\148\191\235\245\216\135\166N\148>\197r\ETX\219\DC2\219\131\138\237\151\196\ag<\158\&7\201\207\SO\192\129R\209\&5\218\EM\230\252\193\149\182w^\230\246v\155\131\&1N\178\146U\167\US\152[\NAK\233\181\&8\EOTw\138\169oQ\167N*!\195\218\222\225n\169K\227\136\SOHL\183"
λ> ByteChars.unpack <$> getRandomBytes 300
"\235\&4R\t\216\"~\242\238 \197\189v\178rQ\239\197\191\200\191\160X\151\bEY\252h=a>\234H\199\200\187\211\133;\RS\170\231S\217\ETB\179\DLE\163/X\205\177\ESCF\NAK]\135N\148/\250\158K\175\145\SO\230Ua\220Zug\238\a\215\253\SOH\199\NUL\243\145X\179\151\181\217\169\ENQ>\234\147\US\CAN0\138\SO\253N|\204\200\227]#`7P\160\t>\191\169\244T<\204\fZ8ToA\NAK\250!W\155+f\159\189\161U\195\209\DC3\176h\FS6aB\165)\213\RSw\215\NUL\SYNV\253\240J\f\223\212\158y\DC4\129\238W\213m=\186J\222\187\175\&2C?\208\194_Y\220a#\144>\221X\150\143S;/\207\248\175\f\FS\199\DC3\188+\224a\STX\166YJzJQ\182\136MB\210 K\DLEK\201\136\141\154-\180D}\235\187\243\164\128oM\128\209JE\232\NAK\169\132J\196\142g\152\158&\243\&4'g\194\178\227\255\191|?\176\183\tj\243\196\161\SO\222\161\196N\DLEv\136M\218\SYN\USK\228\152\213\160\223\FS\220\222\141m\156T\194\&4W\222j*!\SOH"
@vincenthz
Member

Thanks @bergmark

@centromere it depends if you're using rdrand in the first place, and if you do, I think the operating system seems to be playing a part here. I'll check if I can reproduce on 0.15 and 0.15.1 later

@tekul
Member
tekul commented Aug 29, 2016

I'm back from Zurihac finally, so I'll try and reproduce on some other machines.

@centromere
Member

I installed like so and was still unable to reproduce:

cabal install cryptonite-0.15 --constraint="cryptonite -support_rdrand"
@tekul
Member
tekul commented Aug 29, 2016

@vincenthz Is there some way to check whether the "real" rdrand instructions are being invoked by a process?

I guess you can tell whether cryptonite is skipping rdrand altogether by using strace (or dtrace) to see if your data is coming from /dev/(u)random but curious to know if there's an equivalent way to monitor calls to rdrand.

@vincenthz
Member
vincenthz commented Aug 29, 2016 edited

@tekul It'ld would be useful to have a debug module in cryptonite to tell the configuration, version, and some system stuff (architecture, feature supported, etc); that would provide access to all this.

@tekul
Member
tekul commented Aug 29, 2016 edited

@vincenthz Maybe we should also add a startup self-test to the entropy source which would error out and dump this debug information if some data pulled out didn't match basic random tests? That would prevent odd errors like this occurring on different platforms.

@vincenthz
Member

It's a bit hard to do and not very reliable to do "random" tests though. I had some functions to calculate the probability of every possible choice and check if there's any skew. that would probably help in this case.

@vincenthz
Member

I think we can close this for now. I've created #100 to improve our tests

@vincenthz vincenthz closed this Sep 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment