[gnutls-devel] GnuTLS | Allow registering ciphers with higher priority (!1404)

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[gnutls-devel] GnuTLS | Allow registering ciphers with higher priority (!1404)

Read-only notification of GnuTLS library development activities
GitLab

František Krenželok created a merge request:

Project:Branches: FrantisekKrenzelok/gnutls:af_alg to gnutls/gnutls:master
Author: František Krenželok
Assignees:
Reviewers:

Add a description of the new feature/bug fix. Reference any relevant bugs.

Checklist

  • Commits have Signed-off-by: with name/author being identical to the commit author
  • Code modified for feature
  • Test suite updated with functionality tests
  • Test suite updated with negative tests
  • Documentation updated / NEWS entry present (for non-trivial changes)
  • CI timeout is 2h or higher (see Settings/CICD/General pipelines/Timeout)

Reviewer's checklist:

  • Any issues marked for closing are addressed
  • There is a test suite reasonably covering new functionality or modifications
  • Function naming, parameters, return values, types, etc., are consistent and according to CONTRIBUTION.md
  • This feature/change has adequate documentation added
  • No obvious mistakes in the code

_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Allow registering ciphers with higher priority (!1404)

Read-only notification of GnuTLS library development activities
GitLab
Merge request !1404 was reviewed by Daiki Ueno

Daiki Ueno started a new discussion on lib/accelerated/afalg.c:

321
+	/* Set PT buffer to be filled by kernel */
322
+	iov[1].iov_base = plain;
323
+	iov[1].iov_len = plain_size;
	iov[1].iov_len = kcapi_aead_outbuflen_dec(ctx->handle,
						  encr_size - tag_size,
						  auth_size,
						  tag_size) - auth_size;

Daiki Ueno started a new discussion on lib/accelerated/afalg.c:

427
+	/* Set CT buffer to be filled by kernel */
428
+	iov[1].iov_base = encr;
429
+	iov[1].iov_len = plain_size + tag_size;
	iov[1].iov_len = kcapi_aead_outbuflen_enc(ctx->handle,
						  plain_size,
						  auth_size,
						  tag_size) - auth_size;

Daiki Ueno started a new discussion on lib/accelerated/afalg.c:

223
+	}
224
+
225
+	ctx->ccm = !strncmp(gnutls_aead_map[algorithm], "ccm", 3);

As we only support a couple of CCM ciphers, I'd suggest just use a switch rather than string comparison, something like:

switch (algorithm) {
case GNUTLS_CIPHER_AES_128_CCM:
case GNUTLS_CIPHER_AES_256_CCM:
...
}

Daiki Ueno started a new discussion on lib/accelerated/afalg.c:

102
+	struct kcapi_ctx *ctx = _ctx;
103
+
104
+	if (kcapi_cipher_encrypt(ctx->handle, src, src_size, ctx->iv,

From my experiment, src_size needs to be exactly the block size for CBC (or perhaps there might be an alignment issue). I haven't tried but maybe you could rewrite this (and afalg_cipher_decrypt) either in the following ways:

@smuellerDD thoughts?


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Allow registering ciphers with higher priority (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Daiki Ueno commented:

I'm skeptical about the "Allow registering ciphers with higher priority" commit, because it would reverse the current logic of cipher lookup. Perhaps you could just drop this commit and adjust the priority at registration (currently it's 90, but it can be 80 or lower). Maybe also change the title of MR.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Allow registering ciphers with higher priority (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Stephan Mueller commented on a discussion on lib/accelerated/afalg.c:

89
+	struct kcapi_ctx *ctx = _ctx;
90
+
91
+	if (iv_size > kcapi_cipher_ivsize(ctx->handle))
92
+		return gnutls_assert_val(GNUTLS_E_INVALID_REQUEST);
93
+
94
+	memcpy(ctx->iv, iv, iv_size);
95
+
96
+	return 0;
97
+}
98
+
99
+static int afalg_cipher_encrypt(void *_ctx, const void *src, size_t src_size,
100
+				void *dst, size_t dst_size)
101
+{
102
+	struct kcapi_ctx *ctx = _ctx;
103
+
104
+	if (kcapi_cipher_encrypt(ctx->handle, src, src_size, ctx->iv,

Are you saying that src_size could be not a multiple of the block size? Also, there should not be an alignment issue because the data would either be spliced by the kernel or copied into the kernel. And when using splice the kernel implementations should not require any specific alignment.

What is the error your see?


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Allow registering ciphers with higher priority (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Stephan Mueller commented on a discussion on lib/accelerated/afalg.c:

308
+	 * read-only
309
+	 */
310
+	iov[1].iov_base = (void *)encr;
311
+	iov[1].iov_len = encr_size;
312
+
313
+	if (kcapi_aead_stream_update_last(ctx->handle, iov, 2) < 0) {
314
+		gnutls_assert();
315
+		return GNUTLS_E_ENCRYPTION_FAILED;
316
+	}
317
+
318
+	/* The kernel may set the AAD, avoid modification of auth */
319
+	iov[0].iov_base = authtmp;
320
+
321
+	/* Set PT buffer to be filled by kernel */
322
+	iov[1].iov_base = plain;
323
+	iov[1].iov_len = plain_size;

May I ask for the motivation to use kcapi_aead_outbuflen_dec? I fear that you can easily introduce a buffer overflow this way as kcapi_aead_outbuflen_dec returns the size needed in user space to fulfill the request. Are you sure that plain is of sufficient size?


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Allow registering ciphers with higher priority (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Stephan Mueller commented on a discussion on lib/accelerated/afalg.c:

414
+		iov[2].iov_base = encr;
415
+		iov[2].iov_len = tag_size;
416
+		iovlen = 3;
417
+	}
418
+
419
+	if (kcapi_aead_stream_update_last(ctx->handle, iov, iovlen) < 0) {
420
+		gnutls_assert();
421
+		return GNUTLS_E_ENCRYPTION_FAILED;
422
+	}
423
+
424
+	/* The kernel may set the AAD, avoid modification of auth */
425
+	iov[0].iov_base = authtmp;
426
+
427
+	/* Set CT buffer to be filled by kernel */
428
+	iov[1].iov_base = encr;
429
+	iov[1].iov_len = plain_size + tag_size;

Same concerns here.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Allow registering ciphers with higher priority (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Stephan Mueller commented on a discussion on lib/accelerated/afalg.c:

210
+	struct kcapi_aead_ctx *ctx;
211
+
212
+	if (kcapi_aead_init(&handle, gnutls_aead_map[algorithm], 0) < 0) {
213
+		gnutls_assert();
214
+		return GNUTLS_E_MEMORY_ERROR;
215
+	}
216
+
217
+	ctx = (struct kcapi_aead_ctx *)gnutls_calloc(1,
218
+					sizeof(struct kcapi_aead_ctx));
219
+	if (ctx == NULL) {
220
+		gnutls_assert();
221
+		kcapi_aead_destroy(handle);
222
+		return GNUTLS_E_MEMORY_ERROR;
223
+	}
224
+
225
+	ctx->ccm = !strncmp(gnutls_aead_map[algorithm], "ccm", 3);

Yes, that is much more sane, thanks.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Allow registering ciphers with higher priority (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Stephan Mueller started a new discussion on lib/accelerated/afalg.c:

239
+	}
240
+
241
+	return 0;
242
+}
243
+
244
+static int afalg_aead_decrypt(void *_ctx,
245
+			      const void *nonce, size_t nonce_size,
246
+			      const void *auth, size_t auth_size,
247
+			      size_t tag_size,
248
+			      const void *encr, size_t encr_size,
249
+			      void *plain, size_t plain_size)
250
+{
251
+	struct kcapi_aead_ctx *ctx = _ctx;
252
+	struct iovec iov[3];
253
+	uint32_t iovlen = 2;
254
+	uint8_t authtmp[auth_size];

I recommend changing that - VLAs are not good. E.g. use the maximum auth_size for the stack buffer.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Allow registering ciphers with higher priority (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Stephan Mueller started a new discussion on lib/accelerated/afalg.c:

240
+
241
+	return 0;
242
+}
243
+
244
+static int afalg_aead_decrypt(void *_ctx,
245
+			      const void *nonce, size_t nonce_size,
246
+			      const void *auth, size_t auth_size,
247
+			      size_t tag_size,
248
+			      const void *encr, size_t encr_size,
249
+			      void *plain, size_t plain_size)
250
+{
251
+	struct kcapi_aead_ctx *ctx = _ctx;
252
+	struct iovec iov[3];
253
+	uint32_t iovlen = 2;
254
+	uint8_t authtmp[auth_size];
255
+	uint8_t tagtmp[tag_size];

Dto, use the max tag size and not a VLA.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Allow registering ciphers with higher priority (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Stephan Mueller started a new discussion on lib/accelerated/afalg.c:

334
+	if (kcapi_aead_stream_op(ctx->handle, iov, iovlen) < 0)
335
+		return gnutls_assert_val(GNUTLS_E_DECRYPTION_FAILED);
336
+
337
+	return 0;
338
+}
339
+
340
+static int afalg_aead_encrypt(void *_ctx, const void *nonce, size_t nonce_size,
341
+			      const void *auth, size_t auth_size,
342
+			      size_t tag_size,
343
+			      const void *plain, size_t plain_size,
344
+			      void *encr, size_t encr_size)
345
+{
346
+	struct kcapi_aead_ctx *ctx = _ctx;
347
+	struct iovec iov[3];
348
+	uint32_t iovlen = 2;
349
+	uint8_t authtmp[auth_size];

Same issue as above.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | Allow registering ciphers with higher priority (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Daiki Ueno commented on a discussion on lib/accelerated/afalg.c:

89
+	struct kcapi_ctx *ctx = _ctx;
90
+
91
+	if (iv_size > kcapi_cipher_ivsize(ctx->handle))
92
+		return gnutls_assert_val(GNUTLS_E_INVALID_REQUEST);
93
+
94
+	memcpy(ctx->iv, iv, iv_size);
95
+
96
+	return 0;
97
+}
98
+
99
+static int afalg_cipher_encrypt(void *_ctx, const void *src, size_t src_size,
100
+				void *dst, size_t dst_size)
101
+{
102
+	struct kcapi_ctx *ctx = _ctx;
103
+
104
+	if (kcapi_cipher_encrypt(ctx->handle, src, src_size, ctx->iv,

Let's take AES-128-CBC as an example: first encrypt 32-byte data with two calls to afalg_cipher_encrypt (backed by kcapi_cipher_encrypt), 16-byte each time, and then decrypt the resulting 32-byte data in one-shot. After that, the latter half of the plaintext is garbled.

Here is a reproducer: test-cbc.c.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel