HsOpenSSL icon indicating copy to clipboard operation
HsOpenSSL copied to clipboard

Assertions not triggering with -O and test suite failure

Open nh2 opened this issue 5 years ago • 2 comments

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.

nh2 avatar May 19 '19 16:05 nh2

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:

  • these asserts must be converted to error
  • count '=' should be replaced to checking whether input ends on "=" or "=="
  • negative results of _DecodeBlockmust be handled with error
  • note about decoding of "AB==CD==" should be added
  • module should be deprecated in favor of base64-bytestring

vshabanov avatar May 23 '19 13:05 vshabanov

@vshabanov That plan sounds good to me!

nh2 avatar Jun 17 '19 14:06 nh2