From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3D9F52F160 for ; Tue, 21 May 2019 11:25:18 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 54EhqFD2Fsw2 for ; Tue, 21 May 2019 11:25:18 -0400 (EDT) Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6E5F52F0AA for ; Tue, 21 May 2019 11:25:17 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 1/1] crypto: fix assertion on cipher reinitialization From: Vladislav Shpilevoy References: <73be47571ecdce078ad0a9d155653034c338c161.1558092222.git.v.shpilevoy@tarantool.org> Message-ID: Date: Tue, 21 May 2019 18:25:12 +0300 MIME-Version: 1.0 In-Reply-To: <73be47571ecdce078ad0a9d155653034c338c161.1558092222.git.v.shpilevoy@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: georgy@tarantool.org Pushed to 2.1 and 1.10. On 17/05/2019 14:24, 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-segfault > > 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:"]: Key length should be equal to cipher key length > - (16 bytes)' > +- error: 'builtin/crypto.lua:"]: 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:"]: Initial vector length should be equal to cipher > - iv length (16 bytes)' > +- error: 'builtin/crypto.lua:"]: 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:"]: Key length should be equal to cipher key length > - (32 bytes)' > +- error: 'builtin/crypto.lua:"]: 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:"]: Cipher not initialized' > +... > +crypto.cipher.aes256.cbc.decrypt('12', '12345678876543211234567887654321', '12') > +--- > - error: 'builtin/crypto.lua:"]: 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:"]: Key length should be equal to cipher key length > + (24 bytes)' > +... > +crypto.cipher.aes192.cbc.decrypt.new('123456788765432112345678', '12345') > --- > - error: 'builtin/crypto.lua:"]: 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 > >