[tarantool-patches] Re: [PATCH 1/1] crypto: fix assertion on cipher reinitialization

["Георгий Кириченко" <georgy@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 8402 bytes --]

LGTM

On Friday, May 17, 2019 2:24:20 PM MSK Vladislav Shpilevoy wrote:
> Crypto provides API to create stream objects. These streams
> consume plain and return ecnrypted data. Steps:
> 
>     1 c = cipher.new([key, iv])
>     2 c:init(key, iv)
>     3 c:update(input)
>     4 c:result()
> 
> Step 2 is optional, if key and iv are specified in new(), but if
> it called without key or iv, then result() method crashes.
> 
> The commit allows to fill key and iv gradually, in several init()
> calls, and remembers previous results.
> 
> Closes #4223
> ---
> Branch:
> https://github.com/tarantool/tarantool/tree/gerold103/gh-4223-crypto-segfau
> lt
> 
>  src/lua/crypto.lua       | 37 +++++++++++++++----------
>  test/app/crypto.result   | 60 ++++++++++++++++++++++++++++++----------
>  test/app/crypto.test.lua | 21 ++++++++++++--
>  3 files changed, 86 insertions(+), 32 deletions(-)
> 
> diff --git a/src/lua/crypto.lua b/src/lua/crypto.lua
> index e76370517..9c5769bc7 100644
> --- a/src/lua/crypto.lua
> +++ b/src/lua/crypto.lua
> @@ -234,14 +234,6 @@ local function cipher_gc(ctx)
>  end
> 
>  local function cipher_new(cipher, key, iv, direction)
> -    if key == nil or key:len() ~= ffi.C.tnt_EVP_CIPHER_key_length(cipher)
> then -        return error('Key length should be equal to cipher key length
> (' -            .. tostring(ffi.C.tnt_EVP_CIPHER_key_length(cipher)) .. '
> bytes)') -    end
> -    if iv == nil or iv:len() ~= ffi.C.tnt_EVP_CIPHER_iv_length(cipher) then
> -        return error('Initial vector length should be equal to cipher iv
> length (' -            .. tostring(ffi.C.tnt_EVP_CIPHER_iv_length(cipher))
> .. ' bytes)') -    end
>      local ctx = ffi.C.EVP_CIPHER_CTX_new()
>      if ctx == nil then
>          return error('Can\'t create cipher ctx: ' .. openssl_err_str())
> @@ -264,11 +256,28 @@ local function cipher_init(self, key, iv)
>      if self.ctx == nil then
>          return error('Cipher context isn\'t usable')
>      end
> -    if ffi.C.EVP_CipherInit_ex(self.ctx, self.cipher, nil,
> -        key, iv, self.direction) ~= 1 then
> -        return error('Can\'t init cipher:' .. openssl_err_str())
> +    local cipher = self.cipher
> +    key = key or self.key
> +    iv = iv or self.iv
> +    local needed = ffi.C.tnt_EVP_CIPHER_key_length(cipher)
> +    if key ~= nil and key:len() ~= needed then
> +        return error('Key length should be equal to cipher key length ('..
> +                     tostring(needed)..' bytes)')
> +    end
> +    needed = ffi.C.tnt_EVP_CIPHER_iv_length(cipher)
> +    if iv ~= nil and iv:len() ~= needed then
> +        return error('Initial vector length should be equal to cipher iv
> '.. +                     'length ('..tostring(needed)..' bytes)')
> +    end
> +    self.key = key
> +    self.iv = iv
> +    if key and iv then
> +        if ffi.C.EVP_CipherInit_ex(self.ctx, cipher, nil, key, iv,
> +                                   self.direction) ~= 1 then
> +            return error('Can\'t init cipher:'..openssl_err_str())
> +        end
> +        self.initialized = true
>      end
> -    self.initialized = true
>  end
> 
>  local function cipher_update(self, input)
> @@ -289,12 +298,10 @@ local function cipher_final(self)
>      if not self.initialized then
>          return error('Cipher not initialized')
>      end
> -    self.initialized = false
> -    local wpos = self.buf:reserve(self.block_size - 1)
> +    local wpos = self.buf:reserve(self.block_size)
>      if ffi.C.EVP_CipherFinal_ex(self.ctx, wpos, self.outl) ~= 1 then
>          return error('Can\'t finalize cipher:' .. openssl_err_str())
>      end
> -    self.initialized = false
>      return ffi.string(wpos, self.outl[0])
>  end
> 
> diff --git a/test/app/crypto.result b/test/app/crypto.result
> index 2d939b6fc..300d8d5e0 100644
> --- a/test/app/crypto.result
> +++ b/test/app/crypto.result
> @@ -35,8 +35,7 @@ ciph.decrypt(enc, pass, iv)
>  --Failing scenaries
>  crypto.cipher.aes128.cbc.encrypt('a')
>  ---
> -- error: 'builtin/crypto.lua:<line>"]: Key length should be equal to cipher
> key length -    (16 bytes)'
> +- error: 'builtin/crypto.lua:<line>"]: Cipher not initialized'
>  ...
>  crypto.cipher.aes128.cbc.encrypt('a', '123456', '435')
>  ---
> @@ -45,8 +44,7 @@ crypto.cipher.aes128.cbc.encrypt('a', '123456', '435')
>  ...
>  crypto.cipher.aes128.cbc.encrypt('a', '1234567887654321')
>  ---
> -- error: 'builtin/crypto.lua:<line>"]: Initial vector length should be
> equal to cipher -    iv length (16 bytes)'
> +- error: 'builtin/crypto.lua:<line>"]: Cipher not initialized'
>  ...
>  crypto.cipher.aes128.cbc.encrypt('a', '1234567887654321', '12')
>  ---
> @@ -55,8 +53,7 @@ crypto.cipher.aes128.cbc.encrypt('a', '1234567887654321',
> '12') ...
>  crypto.cipher.aes256.cbc.decrypt('a')
>  ---
> -- error: 'builtin/crypto.lua:<line>"]: Key length should be equal to cipher
> key length -    (32 bytes)'
> +- error: 'builtin/crypto.lua:<line>"]: Cipher not initialized'
>  ...
>  crypto.cipher.aes256.cbc.decrypt('a', '123456', '435')
>  ---
> @@ -65,29 +62,62 @@ crypto.cipher.aes256.cbc.decrypt('a', '123456', '435')
>  ...
>  crypto.cipher.aes256.cbc.decrypt('a', '12345678876543211234567887654321')
>  ---
> +- error: 'builtin/crypto.lua:<line>"]: Cipher not initialized'
> +...
> +crypto.cipher.aes256.cbc.decrypt('12', '12345678876543211234567887654321',
> '12') +---
>  - error: 'builtin/crypto.lua:<line>"]: Initial vector length should be
> equal to cipher iv length (16 bytes)'
>  ...
> -crypto.cipher.aes256.cbc.decrypt('12', '12345678876543211234567887654321',
> '12') +crypto.cipher.aes192.cbc.encrypt.new('123321')
> +---
> +- error: 'builtin/crypto.lua:<line>"]: Key length should be equal to cipher
> key length +    (24 bytes)'
> +...
> +crypto.cipher.aes192.cbc.decrypt.new('123456788765432112345678', '12345')
>  ---
>  - error: 'builtin/crypto.lua:<line>"]: Initial vector length should be
> equal to cipher iv length (16 bytes)'
>  ...
> -crypto.cipher.aes192.cbc.encrypt.new()
> +--
> +-- It is allowed to fill a cipher gradually.
> +--
> +c = crypto.cipher.aes128.cbc.encrypt.new()
>  ---
> -- error: Key length should be equal to cipher key length (24 bytes)
>  ...
> -crypto.cipher.aes192.cbc.encrypt.new('123321')
> +c:init('1234567812345678')
>  ---
> -- error: Key length should be equal to cipher key length (24 bytes)
>  ...
> -crypto.cipher.aes192.cbc.decrypt.new('123456788765432112345678')
> +c:update('plain')
>  ---
> -- error: Initial vector length should be equal to cipher iv length (16
> bytes) +- error: Cipher not initialized
>  ...
> -crypto.cipher.aes192.cbc.decrypt.new('123456788765432112345678', '12345')
> +c:init(nil, '1234567812345678')
> +---
> +...
> +c:update('plain')
> +---
> +-
> +...
> +c:free()
> +---
> +...
> +--
> +-- gh-4223: cipher:init() could rewrite previously initialized
> +-- key and IV with nil values.
> +--
> +c = crypto.cipher.aes192.cbc.encrypt.new('123456789012345678901234',
> '1234567890123456') +---
> +...
> +c:init()
> +---
> +...
> +c:result()
> +---
> +- !!binary xzcDuRoi1sml7FB9IFf8EQ==
> +...
> +c:free()
>  ---
> -- error: Initial vector length should be equal to cipher iv length (16
> bytes) ...
>  crypto.cipher.aes100.efb
>  ---
> diff --git a/test/app/crypto.test.lua b/test/app/crypto.test.lua
> index 94f594bcc..2632346b9 100644
> --- a/test/app/crypto.test.lua
> +++ b/test/app/crypto.test.lua
> @@ -23,11 +23,28 @@ crypto.cipher.aes256.cbc.decrypt('a', '123456', '435')
>  crypto.cipher.aes256.cbc.decrypt('a', '12345678876543211234567887654321')
>  crypto.cipher.aes256.cbc.decrypt('12', '12345678876543211234567887654321',
> '12')
> 
> -crypto.cipher.aes192.cbc.encrypt.new()
>  crypto.cipher.aes192.cbc.encrypt.new('123321')
> -crypto.cipher.aes192.cbc.decrypt.new('123456788765432112345678')
>  crypto.cipher.aes192.cbc.decrypt.new('123456788765432112345678', '12345')
> 
> +--
> +-- It is allowed to fill a cipher gradually.
> +--
> +c = crypto.cipher.aes128.cbc.encrypt.new()
> +c:init('1234567812345678')
> +c:update('plain')
> +c:init(nil, '1234567812345678')
> +c:update('plain')
> +c:free()
> +
> +--
> +-- gh-4223: cipher:init() could rewrite previously initialized
> +-- key and IV with nil values.
> +--
> +c = crypto.cipher.aes192.cbc.encrypt.new('123456789012345678901234',
> '1234567890123456') +c:init()
> +c:result()
> +c:free()
> +
>  crypto.cipher.aes100.efb
>  crypto.cipher.aes256.nomode


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]