wahern / luaossl

Most comprehensive OpenSSL module in the Lua universe.
http://25thandclement.com/~william/projects/luaossl.html
Other
140 stars 49 forks source link

Add support for getting/setting GCM authentication tag (fixes #115) #201

Closed mwild1 closed 2 years ago

mwild1 commented 2 years ago

See https://github.com/wahern/luaossl/issues/115#issuecomment-1138846788 for context.

mwild1 commented 2 years ago

The following code snippet can be used to test GCM encryption (it's based on the code originally posted as broken in #115):

local cipher = require "openssl.cipher"
local key = "abcdefghijklmnopabcdefghijklmnop"
local iv = "123456123456"
local message = "My secret message"

local c = cipher.new("aes-256-gcm"):encrypt(key, iv);
local encrypted = c:update(message)
assert(c:final());
local tag = assert(c:getTag(16));

print("Encrypted to "..#encrypted.." bytes with "..#tag.."-byte tag");

-- Now for the decryption
local aes = cipher.new("aes-256-gcm"):decrypt(key, iv)
-- Provide the expected authentication tag to OpenSSL
aes:setTag(tag);
local decrypted = aes:update(encrypted)
print(decrypted)
print(aes:final())

CCM can be tested by changing c:getTag(16) to c:getTag(12) and obviously changing -gcm to -ccm in the cipher name.

daurnimator commented 2 years ago

The following code snippet ...

Could you add this as a regression test?

mwild1 commented 2 years ago

Regression test added in https://github.com/wahern/luaossl/pull/201/commits/e402818ccc91f3ec97641d08d8c64f60fec55580

daurnimator commented 2 years ago

Merged with bed73b66dee1dde0bfa5a658cf109457c3f70b46

daurnimator commented 2 years ago

@mwild1 I'm only running your regression tests now, and I get:

$ lua 115-test-aead.lua 
lua: 115-test-aead.lua:12: bad argument #2 to 'encrypt' (12: invalid IV length (should be 7))
stack traceback:
    [C]: in method 'encrypt'
    115-test-aead.lua:12: in function 'test_aead'
    115-test-aead.lua:36: in main chunk
    [C]: in ?
mwild1 commented 2 years ago

Well that's interesting. The test passes fine for me as-is. If I change the IV length to 7, I get:

lua: 115-test-aead.lua:12: bad argument #2 to 'encrypt' (7: invalid IV length (should be 12))
stack traceback:
    [C]: in function 'encrypt'
    115-test-aead.lua:12: in function 'test_aead'
    115-test-aead.lua:37: in main chunk
    [C]: in ?

This is with OpenSSL 1.1.1.

Although I can't find it now, I vaguely recall some documentation that indicated the iv_length() function was not reliable for ciphers that accept IVs of variable/multiple lengths.

My guess is that maybe the default IV length has changed between versions. Out of curiosity, what version was your result with?

Some further reading:

So in 1.1.1+ if we explicitly set the IV length (which might be sensible anyway if the default varies between versions) then this issue should go away. But if we want backwards-compatibility with pre-1.1.1, I guess we need to skip this check for these ciphers.

daurnimator commented 2 years ago

If I change the IV length to 7, I get:

lua: 115-test-aead.lua:12: bad argument #2 to 'encrypt' (7: invalid IV length (should be 12))
stack traceback:
  [C]: in function 'encrypt'
  115-test-aead.lua:12: in function 'test_aead'
  115-test-aead.lua:37: in main chunk
  [C]: in ?

Me too:

$ git diff
diff --git a/regress/115-test-aead.lua b/regress/115-test-aead.lua
index ed0f7f8..5026ce1 100644
--- a/regress/115-test-aead.lua
+++ b/regress/115-test-aead.lua
@@ -5,7 +5,7 @@ local cipher = require "openssl.cipher"

 -- Test AES-256-GCM
 local key = "abcdefghijklmnopabcdefghijklmnop"
-local iv = "123456123456"
+local iv = "1234561"
 local message = "My secret message"

 function test_aead(params)
$ lua 115-test-aead.lua 
lua: 115-test-aead.lua:12: bad argument #2 to 'encrypt' (7: invalid IV length (should be 12))
stack traceback:
    [C]: in method 'encrypt'
    115-test-aead.lua:12: in function 'test_aead'
    115-test-aead.lua:31: in main chunk
    [C]: in ?

Out of curiosity, what version was your result with?

$ openssl version
OpenSSL 1.1.1p  21 Jun 2022

So in 1.1.1+ if we explicitly set the IV length (which might be sensible anyway if the default varies between versions) then this issue should go away.

I'm running new enough that the fix you mentioned should be in. I guess it's something else...

daurnimator commented 2 years ago

oh; I only just noticed the line number in the stack trace: the error moves from 36 to 31 when the IV changes length.

So I guess for aes-256-gcm it wants 12; but for aes-256-ccm it wants 7? or some variable length where it's currently initialised to 7?

mwild1 commented 2 years ago

Yeah, I made these changes for testing:

diff --git a/regress/115-test-aead.lua b/regress/115-test-aead.lua
index ed0f7f8..febea2d 100644
--- a/regress/115-test-aead.lua
+++ b/regress/115-test-aead.lua
@@ -9,7 +9,7 @@ local iv = "123456123456"
 local message = "My secret message"

 function test_aead(params)
-       local c = cipher.new(params.cipher):encrypt(key, iv)
+       local c = cipher.new(params.cipher):encrypt(key, params.iv)

        local encrypted = c:update(message)
        regress.check(encrypted)
@@ -31,9 +31,11 @@ end
 test_aead {
        cipher = "aes-256-gcm";
        tag_length = 16;
+       iv = iv;
 }

 test_aead {
        cipher = "aes-256-ccm";
        tag_length = 12;
+       iv = iv:sub(1, 7);
 }
daurnimator commented 2 years ago

@mwild1 how about https://github.com/wahern/luaossl/pull/202 ?