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 8696A259FE for ; Tue, 21 May 2019 04:40:50 -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 2vD4rTaPgGug for ; Tue, 21 May 2019 04:40:50 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 9574529302 for ; Tue, 21 May 2019 04:40:49 -0400 (EDT) From: =?utf-8?B?0JPQtdC+0YDQs9C40Lkg0JrQuNGA0LjRh9C10L3QutC+?= Subject: [tarantool-patches] Re: [PATCH 1/1] crypto: fix assertion on cipher reinitialization Date: Mon, 20 May 2019 16:53:11 +0300 Message-ID: <1718500.beJYYlpZKy@localhost> In-Reply-To: <73be47571ecdce078ad0a9d155653034c338c161.1558092222.git.v.shpilevoy@tarantool.org> References: <73be47571ecdce078ad0a9d155653034c338c161.1558092222.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart2190473.bnpqfGCoKH"; micalg="pgp-sha256"; protocol="application/pgp-signature" 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: Vladislav Shpilevoy --nextPart2190473.bnpqfGCoKH Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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:"]: 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 --nextPart2190473.bnpqfGCoKH Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEFZT35EtIMRTDS5hJnoTdFFzh6LUFAlzisUcACgkQnoTdFFzh 6LU+jAf+JLzzmSOZS+eueCj5pOn97aLYfpOROozWuBDy0Nj25i53VmEDSTU9paNl e5gw2u5P6jqp9XDudJu4vDksGQDfrg5MSLBfdiNMhhrrShYDT/VqEDpYJrXXBDgh grGWG7J2JDh4jrz1lhLhoO8zHLJRlSRVzk24FjyATFzxuOBDvszurwHro2y9EQhV i3FtEVSw4O/cwbhCL8eQpTo5ZlQZh1dBcWpIJVvl5D22w91HHJiyrqorrKOM9c5w RK856acgYON+4eSo/KPl0QHcx405RIQWmIowPMmPbGEdskLlSqH1NHhPvUydedVD zfj8V0Jbh8uHsZn9CFSMdXNHnQ11LA== =bMic -----END PGP SIGNATURE----- --nextPart2190473.bnpqfGCoKH--