Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org
Cc: georgy@tarantool.org
Subject: [tarantool-patches] [PATCH 1/1] crypto: fix assertion on cipher reinitialization
Date: Fri, 17 May 2019 14:24:20 +0300	[thread overview]
Message-ID: <73be47571ecdce078ad0a9d155653034c338c161.1558092222.git.v.shpilevoy@tarantool.org> (raw)

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:<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
 
-- 
2.20.1 (Apple Git-117)

             reply	other threads:[~2019-05-17 11:24 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-17 11:24 Vladislav Shpilevoy [this message]
2019-05-20 13:53 ` [tarantool-patches] " Георгий Кириченко
2019-05-21 15:25 ` Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=73be47571ecdce078ad0a9d155653034c338c161.1558092222.git.v.shpilevoy@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=georgy@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [tarantool-patches] [PATCH 1/1] crypto: fix assertion on cipher reinitialization' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox