haskell-cryptography / HsOpenSSL

OpenSSL binding for Haskell
http://hackage.haskell.org/package/HsOpenSSL
Creative Commons Zero v1.0 Universal
21 stars 30 forks source link

Assertions not triggering with -O and test suite failure #44

Open nh2 opened 5 years ago

nh2 commented 5 years ago

Hi,

version 0.11.4.16.

When you test with ./Setup build test:test-evp-base64 --ghc-option=-O0 or in ghci, then a test failure occurs (I've inserted some more stack traces and prints for your convenience):

in assertFunction: print (n, x, y, r)
("decodeBase64BS","YWJjZGVmZ2hpams=\n","abcdefghijk","decodeBlock "YWJjZGVmZ2hpams=\n"
*** Exception: Assertion failed
CallStack (from HasCallStack):
  assert, called at ./OpenSSL/EVP/Base64.hs:111:45 in main:OpenSSL.EVP.Base64
  decodeBlock, called at ./OpenSSL/EVP/Base64.hs:130:18 in main:OpenSSL.EVP.Base64
  decodeBase64BS, called at Test/OpenSSL/EVP/Base64.hs:51:37 in main:Main

This assertion triggers: https://github.com/vshabanov/HsOpenSSL/blob/a9efae0b598b9499443721a1989055ca017fd01f/OpenSSL/EVP/Base64.hs#L109-L111

This is because in the test https://github.com/vshabanov/HsOpenSSL/blob/master/Test/OpenSSL/EVP/Base64.hs#L49-L58

the last test case, "YWJjZGVmZ2hpams=\n" has 17 chars, and 17 mod 4 is not 0.

This assertion failure usually goes completely unnoticed, because assertions are compiled away when -O is used (which is the default).

Are these assertions relevant for security or correctness?

If yes, then they should not be assertions, because assertions should not be used for control flow and input validation. Alternatively, -fno-ignore-asserts can be used.

vshabanov commented 5 years ago

Bad, bad code. The more I look at this code the more I want to deprecate it in favor of https://hackage.haskell.org/package/base64-bytestring

decodeBase64 "YWJjZGVmZ2hpams=\n" leads to segmentation fault (and decodeBase64 "YWJj\n" too)

decodeBase64BS fails with exception on 1,2,3 character inputs or any other invalid input (due to _DecodeBlock returning negative value)

decodeBase64LBS has segmentation fault on any input not divisible by 4 (due to infinite loop which must be prevented by assert).

Counting '=' is bad as well: https://github.com/vshabanov/HsOpenSSL/blob/a9efae0b598b9499443721a1989055ca017fd01f/OpenSSL/EVP/Base64.hs#L119

OpenSSL seems to just decode 4-byte blocks (I can't understand what it does with newlines but it seems to stop on them). It can decode "AB==CD==EF==" and removing 6 bytes from output makes no sense.

Current plan:

nh2 commented 5 years ago

@vshabanov That plan sounds good to me!