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

mwild1 · 2022-06-29T12:24:11Z

)

src/openssl.c

daurnimator · 2022-06-30T09:27:03Z

src/openssl.c

+
+	luaL_buffinit(L, &tag);
+
+	if(1 != EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_GET_TAG, tag_size, (void*)luaL_prepbuffsize(&tag, tag_size))) {


When was EVP_CIPHER_CTX_ctrl introduced? luaossl still tries to be compatible with old openssl versions

I believe it's been around a "long time". I don't know if it was in 0.9.8 (I can dig if really necessary), but it's certainly in 1.0.2. It's due to be retired post-3.0.

It's due to be retired post-3.0.

What is it going to be replaced with?

A new OSSL_PARAM API they've introduced. I haven't looked too much into it so far, as I haven't been working on any code that can be 3.0.x-only just yet.

daurnimator · 2022-06-30T09:27:32Z

src/openssl.c

@@ -11975,6 +11975,47 @@ static int cipher_final(lua_State *L) {
 } /* cipher_final() */


+static int cipher_get_tag(lua_State *L) {


Should this be cipher_gcm_get_tag or similar to follow the naming of the option?

Possibly. It's actually not unique to GCM, but to the family of AEAD modes - in OpenSSL this includes CCM. In 1.1.x there is now therefore an alias for EVP_CTRL_AEAD_GET_TAG but this constant is not defined in 1.0.x. The current code should work with both GCM and CCM.

If we change the name, we could call it e.g. :get_aead_tag(), but I don't know whether this level of disambiguation is necessary.

Would it be sensible to use EVP_CIPHER_type to get the type of the cipher and then use the relevant constant?

I don't think so. As I mentioned, modern versions of OpenSSL have both CCM_GET_TAG and GCM_GET_TAG aliased to AEAD_GET_TAG. The latter was added in 1.1, but before this the CCM variant was an alias of GCM_GET_TAG anyway.

In summary: in old versions, GCM_GET_TAG and CCM_GET_TAG are interchangeable identifiers for the same operation. In newer versions (1.1+) AEAD_GET_TAG was added as a generic identifier for this operation.

References:

openssl/openssl@e640fa0

Could you add a comment saying as much?

Added in 4a8b770

src/openssl.c

mwild1 · 2022-06-30T12:19:57Z

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 · 2022-06-30T12:36:04Z

The following code snippet ...

Could you add this as a regression test?

mwild1 · 2022-06-30T18:46:43Z

Regression test added in e402818

daurnimator

Couple of nits; otherwise LGTM.

daurnimator · 2022-07-01T04:06:25Z

src/openssl.c

+	{ "getTag", &cipher_get_tag },
+	{ "setTag", &cipher_set_tag },


These need whitespace for alignment

Fixed in 0dad2b3

daurnimator · 2022-07-01T04:06:32Z

src/openssl.c

+	auxL_pusherror(L, auxL_EOPENSSL, NULL);
+
+	return 2;
+} /* cipher_get_tag() */


Suggested change

} /* cipher_get_tag() */

} /* cipher_set_tag() */

Fixed in 0dad2b3

daurnimator · 2022-07-03T12:56:51Z

Merged with bed73b6

daurnimator · 2022-07-07T02:13:35Z

@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 · 2022-07-07T08:54:27Z

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:

AEAD: EVP_CIPHER_CTX_iv_length is oblivious to EVP_CTRL_AEAD_SET_IVLEN openssl/openssl#8330 has some discussion about this issue
openssl/openssl@728f944 may improve the situation, but is in 1.1.1+ only

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 · 2022-07-07T15:13:13Z

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 · 2022-07-07T15:16:44Z

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 · 2022-07-07T18:55:25Z

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 · 2022-07-08T00:19:36Z

@mwild1 how about #202 ?

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

6e70ca0

)

daurnimator reviewed Jun 30, 2022

View reviewed changes

mwild1 added 2 commits June 30, 2022 11:29

style: add extra blank line between function definitions

c77a7cf

Fix API to expose camelCase identifiers per convention

82a9a53

mwild1 added 2 commits June 30, 2022 19:41

Add regression test for wahern#115

e402818

Add comment about EVP_CTRL_GCM_GET_TAG

4a8b770

daurnimator requested changes Jul 1, 2022

View reviewed changes

Alignment and comment fixes

0dad2b3

daurnimator approved these changes Jul 1, 2022

View reviewed changes

daurnimator closed this Jul 3, 2022

daurnimator mentioned this pull request Jul 8, 2022

Allow for ciphers with adjustable IV lengths #202

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mwild1 commented Jun 29, 2022 •

edited

Loading

daurnimator Jun 30, 2022

mwild1 Jun 30, 2022

daurnimator Jun 30, 2022

mwild1 Jun 30, 2022

daurnimator Jun 30, 2022

mwild1 Jun 30, 2022

daurnimator Jun 30, 2022

mwild1 Jun 30, 2022

daurnimator Jun 30, 2022

mwild1 Jun 30, 2022

mwild1 commented Jun 30, 2022

daurnimator commented Jun 30, 2022

mwild1 commented Jun 30, 2022

daurnimator left a comment

daurnimator Jul 1, 2022

mwild1 Jul 1, 2022

daurnimator Jul 1, 2022

mwild1 Jul 1, 2022

daurnimator commented Jul 3, 2022

daurnimator commented Jul 7, 2022

mwild1 commented Jul 7, 2022

daurnimator commented Jul 7, 2022

daurnimator commented Jul 7, 2022 •

edited

Loading

mwild1 commented Jul 7, 2022

daurnimator commented Jul 8, 2022


		luaL_buffinit(L, &tag);

		if(1 != EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_GET_TAG, tag_size, (void*)luaL_prepbuffsize(&tag, tag_size))) {

		@@ -11975,6 +11975,47 @@ static int cipher_final(lua_State *L) {
		} /* cipher_final() */


		static int cipher_get_tag(lua_State *L) {

		{ "getTag", &cipher_get_tag },
		{ "setTag", &cipher_set_tag },

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

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

Conversation

mwild1 commented Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwild1 commented Jun 30, 2022

daurnimator commented Jun 30, 2022

mwild1 commented Jun 30, 2022

daurnimator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daurnimator commented Jul 3, 2022

daurnimator commented Jul 7, 2022

mwild1 commented Jul 7, 2022

daurnimator commented Jul 7, 2022

daurnimator commented Jul 7, 2022 • edited Loading

mwild1 commented Jul 7, 2022

daurnimator commented Jul 8, 2022

mwild1 commented Jun 29, 2022 •

edited

Loading

daurnimator commented Jul 7, 2022 •

edited

Loading