Attention is currently required from: Arne Schwabe, Frank Lichtenheld, Răzvan
Cojocaru.
Hello Arne Schwabe, Răzvan Cojocaru,
I'd like you to reexamine a change. Please visit
https://gerrit.openvpn.net/c/openvpn/+/2158?usp=email
to look at the new patch set (#5).
The following approvals got outdated and were removed:
Code-Review-1 by Răzvan Cojocaru
Change subject: crypto*: Avoid sign conversion warnings by using defines
directly
......................................................................
crypto*: Avoid sign conversion warnings by using defines directly
Using the defines directly avoids the issue with sign
conversion by letting the compiler deduce the types of
the literals.
I do not think that using an intermediate variable
increases the readability.
Change-Id: I3bcb4cb8e17ff80a5cb8acdaf18d1072159184e5
Signed-off-by: Frank Lichtenheld <[email protected]>
---
M src/openvpn/crypto.c
M src/openvpn/crypto_epoch.c
M src/openvpn/packet_id.h
M src/openvpn/tls_crypt.c
M src/openvpn/tls_crypt.h
M tests/unit_tests/openvpn/test_tls_crypt.c
6 files changed, 24 insertions(+), 50 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/58/2158/5
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index a4c2683..6955760 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -81,7 +81,6 @@
const struct key_ctx *ctx = &opt->key_ctx_bi.encrypt;
uint8_t *mac_out = NULL;
- const int mac_len = OPENVPN_AEAD_TAG_LENGTH;
/* IV, packet-ID and implicit IV required for this mode. */
ASSERT(ctx->cipher);
@@ -142,7 +141,7 @@
dmsg(D_PACKET_CONTENT, "ENCRYPT FROM: %s", format_hex(BPTR(buf),
BLENZ(buf), 80, &gc));
/* Buffer overflow check */
- if (!buf_safe(&work, buf->len + mac_len +
cipher_ctx_block_size(ctx->cipher)))
+ if (!buf_safe(&work, BLENZ(buf) + OPENVPN_AEAD_TAG_LENGTH +
cipher_ctx_block_size(ctx->cipher)))
{
msg(D_CRYPT_ERRORS, "ENCRYPT: buffer size error, bc=%d bo=%d bl=%d
wc=%d wo=%d wl=%d",
buf->capacity, buf->offset, buf->len, work.capacity, work.offset,
work.len);
@@ -156,7 +155,7 @@
if (!use_epoch_data_format)
{
/* Reserve space for authentication tag */
- mac_out = buf_write_alloc(&work, mac_len);
+ mac_out = buf_write_alloc(&work, OPENVPN_AEAD_TAG_LENGTH);
ASSERT(mac_out);
}
@@ -177,12 +176,12 @@
if (use_epoch_data_format)
{
/* Reserve space for authentication tag */
- mac_out = buf_write_alloc(&work, mac_len);
+ mac_out = buf_write_alloc(&work, OPENVPN_AEAD_TAG_LENGTH);
ASSERT(mac_out);
}
/* Write authentication tag */
- ASSERT(cipher_ctx_get_tag(ctx->cipher, mac_out, mac_len));
+ ASSERT(cipher_ctx_get_tag(ctx->cipher, mac_out, OPENVPN_AEAD_TAG_LENGTH));
*buf = work;
@@ -453,9 +452,6 @@
CRYPT_DROP("Decryption failed verification limit reached.");
}
- const int tag_size = OPENVPN_AEAD_TAG_LENGTH;
-
-
ASSERT(frame);
ASSERT(buf->len > 0);
ASSERT(ctx->cipher);
@@ -470,14 +466,14 @@
ASSERT(packet_id_initialized(&opt->packet_id));
/* Ensure that the packet size is long enough */
- int min_packet_len = packet_id_size(false) + tag_size + 1;
+ size_t min_packet_len = packet_id_size(false) + OPENVPN_AEAD_TAG_LENGTH +
1;
if (use_epoch_data_format)
{
min_packet_len += sizeof(uint32_t);
}
- if (buf->len < min_packet_len)
+ if (BLENZ(buf) < min_packet_len)
{
CRYPT_ERROR("missing IV info, missing tag or no payload");
}
@@ -540,24 +536,24 @@
}
}
- const int ad_size = (int)(BPTR(buf) - ad_start);
+ const uintptr_t ad_size = (uintptr_t)(BPTR(buf) - ad_start);
uint8_t *tag_ptr = NULL;
int data_len = 0;
if (use_epoch_data_format)
{
- data_len = BLEN(buf) - tag_size;
+ data_len = BLEN(buf) - OPENVPN_AEAD_TAG_LENGTH;
tag_ptr = BPTR(buf) + data_len;
}
else
{
tag_ptr = BPTR(buf);
- ASSERT(buf_advance(buf, tag_size));
+ ASSERT(buf_advance(buf, OPENVPN_AEAD_TAG_LENGTH));
data_len = BLEN(buf);
}
- dmsg(D_PACKET_CONTENT, "DECRYPT MAC: %s", format_hex(tag_ptr, tag_size, 0,
&gc));
+ dmsg(D_PACKET_CONTENT, "DECRYPT MAC: %s", format_hex(tag_ptr,
OPENVPN_AEAD_TAG_LENGTH, 0, &gc));
dmsg(D_PACKET_CONTENT, "DECRYPT FROM: %s", format_hex(BPTR(buf),
BLENZ(buf), 0, &gc));
/* Buffer overflow check (should never fail) */
@@ -567,7 +563,7 @@
}
/* feed in tag and the authenticated data */
- ASSERT(cipher_ctx_update_ad(ctx->cipher, ad_start, ad_size));
+ ASSERT(cipher_ctx_update_ad(ctx->cipher, ad_start, (int)ad_size));
dmsg(D_PACKET_CONTENT, "DECRYPT AD: %s", format_hex(ad_start, ad_size, 0,
&gc));
/* Decrypt and authenticate packet */
@@ -578,7 +574,7 @@
}
ASSERT(buf_inc_len(&work, outlen));
- if (!cipher_ctx_final_check_tag(ctx->cipher, BPTR(&work) + outlen,
&outlen, tag_ptr, tag_size))
+ if (!cipher_ctx_final_check_tag(ctx->cipher, BPTR(&work) + outlen,
&outlen, tag_ptr, OPENVPN_AEAD_TAG_LENGTH))
{
ctx->failed_verifications++;
CRYPT_DROP("packet tag authentication failed");
@@ -1118,20 +1114,19 @@
static void
generate_key_random(struct key *key)
{
- int cipher_len = MAX_CIPHER_KEY_LENGTH;
- int hmac_len = MAX_HMAC_KEY_LENGTH;
-
struct gc_arena gc = gc_new();
CLEAR(*key);
- if (!rand_bytes(key->cipher, cipher_len) || !rand_bytes(key->hmac,
hmac_len))
+ if (!rand_bytes(key->cipher, MAX_CIPHER_KEY_LENGTH)
+ || !rand_bytes(key->hmac, MAX_HMAC_KEY_LENGTH))
{
msg(M_FATAL, "ERROR: Random number generator cannot obtain entropy for
key generation");
}
dmsg(D_SHOW_KEY_SOURCE, "Cipher source entropy: %s",
- format_hex(key->cipher, cipher_len, 0, &gc));
- dmsg(D_SHOW_KEY_SOURCE, "HMAC source entropy: %s", format_hex(key->hmac,
hmac_len, 0, &gc));
+ format_hex(key->cipher, MAX_CIPHER_KEY_LENGTH, 0, &gc));
+ dmsg(D_SHOW_KEY_SOURCE, "HMAC source entropy: %s",
+ format_hex(key->hmac, MAX_HMAC_KEY_LENGTH, 0, &gc));
gc_free(&gc);
}
diff --git a/src/openvpn/crypto_epoch.c b/src/openvpn/crypto_epoch.c
index 243b5a2..40e8cec 100644
--- a/src/openvpn/crypto_epoch.c
+++ b/src/openvpn/crypto_epoch.c
@@ -38,11 +38,6 @@
#include "buffer.h"
#include "integer.h"
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wsign-conversion"
-#endif
-
void
ovpn_hkdf_expand(const uint8_t *secret, const uint8_t *info, size_t info_len,
uint8_t *out, size_t out_len)
@@ -51,26 +46,23 @@
hmac_ctx_init(hmac_ctx, secret, "SHA256");
ASSERT(info_len <= INT_MAX);
- const unsigned int digest_size = SHA256_DIGEST_LENGTH;
/* T(0) = empty string */
uint8_t t_prev[SHA256_DIGEST_LENGTH];
- unsigned int t_prev_len = 0;
- for (uint8_t block = 1; (block - 1) * digest_size < out_len; block++)
+ for (uint8_t block = 1; (block - 1u) * SHA256_DIGEST_LENGTH < out_len;
block++)
{
hmac_ctx_reset(hmac_ctx);
/* calculate T(block) */
- hmac_ctx_update(hmac_ctx, t_prev, t_prev_len);
+ hmac_ctx_update(hmac_ctx, t_prev, block == 1 ? 0 :
SHA256_DIGEST_LENGTH);
hmac_ctx_update(hmac_ctx, info, (int)info_len);
hmac_ctx_update(hmac_ctx, &block, 1);
hmac_ctx_final(hmac_ctx, t_prev);
- t_prev_len = digest_size;
/* Copy a full hmac output or remaining bytes */
- size_t out_offset = (block - 1) * digest_size;
- size_t copylen = min_size(digest_size, out_len - out_offset);
+ size_t out_offset = (block - 1u) * SHA256_DIGEST_LENGTH;
+ size_t copylen = min_size(SHA256_DIGEST_LENGTH, out_len - out_offset);
memcpy(out + out_offset, t_prev, copylen);
}
@@ -463,7 +455,3 @@
epoch_iterate_send_key(opt);
}
}
-
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
diff --git a/src/openvpn/packet_id.h b/src/openvpn/packet_id.h
index 48752d3..782dd58 100644
--- a/src/openvpn/packet_id.h
+++ b/src/openvpn/packet_id.h
@@ -312,7 +312,7 @@
const char *packet_id_net_print(const struct packet_id_net *pin, bool
print_timestamp,
struct gc_arena *gc);
-static inline int
+static inline unsigned int
packet_id_size(bool long_form)
{
return sizeof(packet_id_type) + (long_form ? sizeof(net_time_t) : 0);
diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index 2dadc4f..7eb83ce 100644
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -52,7 +52,7 @@
return create_kt("AES-256-CTR", "SHA256", "tls-crypt");
}
-int
+unsigned int
tls_crypt_buf_overhead(void)
{
return packet_id_size(true) + TLS_CRYPT_TAG_SIZE + TLS_CRYPT_BLOCK_SIZE;
diff --git a/src/openvpn/tls_crypt.h b/src/openvpn/tls_crypt.h
index d179cbb..8e5567f 100644
--- a/src/openvpn/tls_crypt.h
+++ b/src/openvpn/tls_crypt.h
@@ -130,7 +130,7 @@
* Returns the maximum overhead (in bytes) added to the destination buffer by
* tls_crypt_wrap().
*/
-int tls_crypt_buf_overhead(void);
+unsigned int tls_crypt_buf_overhead(void);
/**
* Wrap a control channel packet (both authenticates and encrypts the data).
diff --git a/tests/unit_tests/openvpn/test_tls_crypt.c
b/tests/unit_tests/openvpn/test_tls_crypt.c
index 2d31c00..00de73a 100644
--- a/tests/unit_tests/openvpn/test_tls_crypt.c
+++ b/tests/unit_tests/openvpn/test_tls_crypt.c
@@ -232,11 +232,6 @@
}
}
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wsign-conversion"
-#endif
-
/**
* Check that short messages are successfully wrapped-and-unwrapped.
*/
@@ -778,10 +773,6 @@
tls_wrap_free(&wrap_ctx);
}
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
static void
test_tls_crypt_v2_write_server_key_file(void **state)
{
--
To view, visit https://gerrit.openvpn.net/c/openvpn/+/2158?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I3bcb4cb8e17ff80a5cb8acdaf18d1072159184e5
Gerrit-Change-Number: 2158
Gerrit-PatchSet: 5
Gerrit-Owner: Frank Lichtenheld <[email protected]>
Gerrit-Reviewer: Arne Schwabe <[email protected]>
Gerrit-Reviewer: Răzvan Cojocaru <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: Răzvan Cojocaru <[email protected]>
Gerrit-Attention: Arne Schwabe <[email protected]>
Gerrit-Attention: Frank Lichtenheld <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
