Willy,
Am 09.06.20 um 05:14 schrieb Willy Tarreau:
>> memcmp compares two binary strings in constant time.
>
> (...)
>
> I'd say that the name is quite misleading if you want to enforce the
> constant time requirement, because memcmp() is well known and initially
> I didn't understand the forced dependency on openssl which can be quite
> surprizing for the user, especially since no other converter proceeds
> like this.
Indeed I wasn't sure whether just 'memcmp' would be a good name for it, but the
alternatives all ended up being fairly long-winded.
> I think that instead it could be done slightly differently, by using
> CRYPTO_memcmp() when openssl is present or memcmp() when not, and
> documenting that the constant time is enforced when haproxy is compiled
I don't think that's a good thing, because it can easily result in an
accidental loss of "security". Also the behavior would change, because with
OpenSSL you would only get back a true or false and without you would also get
an ordering.
> with openssl. Otherwise, maybe create two converters, memcmp() and maybe
> secure_memcmp() or whatever, the latter being only available with openssl,
> and promising constant time. But quite frankly I don't think there's much
> interest in constant time in environments where openssl is not built in.
A regular memcmp can already be simulated by hex encoding both values and then
using the strcmp converter. Hex encoding preserves the ordering of the byte
values, thus I believe this should be equivalent if someone needs to know the
ordering.
> What do you think ?
>
I think I'll just rename the converter to `secure_memcmp` and in fact did so
for the attached patch.
Best regards
Tim Düsterhus
Apply with `git am --scissors` to automatically cut the commit message.
-- >8 --
secure_memcmp compares two binary strings in constant time.
---
doc/configuration.txt | 21 ++++
reg-tests/converter/secure_memcmp.vtc | 137 ++++++++++++++++++++++++++
src/sample.c | 57 ++++++++++-
3 files changed, 214 insertions(+), 1 deletion(-)
create mode 100644 reg-tests/converter/secure_memcmp.vtc
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 15cbe889c..ad507888e 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -15148,6 +15148,24 @@ sdbm([<avalanche>])
32-bit hash is trivial to break. See also "crc32", "djb2", "wt6", "crc32c",
and the "hash-type" directive.
+secure_memcmp(<var>)
+ Compares the contents of <var> with the input value. Both values are treated
+ as a binary string. Returns a boolean indicating whether both binary strings
+ match.
+
+ If both binary strings have the same length then the comparison will be
+ performed in constant time.
+
+ Please note that this converter is only available when haproxy has been
+ compiled with USE_OPENSSL.
+
+ Example :
+
+ http-request set-var(txn.token) hdr(token)
+ # Check whether the token sent by the client matches the secret token
+ # value, without leaking the contents using a timing attack.
+ acl token_given str(my_secret_token),secure_memcmp(txn.token)
+
set-var(<var name>)
Sets a variable with the input content and returns the content on the output
as-is. The variable keeps the value and the associated input type. The name
of
@@ -15190,6 +15208,9 @@ strcmp(<var>)
than 0 otherwise (right string greater than left string or the right string
is
shorter).
+ See also the secure_memcmp converter if you need to compare two binary
+ strings in constant time.
+
Example :
http-request set-var(txn.host) hdr(host)
diff --git a/reg-tests/converter/secure_memcmp.vtc
b/reg-tests/converter/secure_memcmp.vtc
new file mode 100644
index 000000000..f9341f942
--- /dev/null
+++ b/reg-tests/converter/secure_memcmp.vtc
@@ -0,0 +1,137 @@
+varnishtest "secure_memcmp converter Test"
+
+#REQUIRE_VERSION=2.2
+#REQUIRE_OPTION=OPENSSL
+
+feature ignore_unknown_macro
+
+server s1 {
+ rxreq
+ txresp
+} -repeat 4 -start
+
+server s2 {
+ rxreq
+ txresp
+} -repeat 7 -start
+
+haproxy h1 -conf {
+ defaults
+ mode http
+ timeout connect 1s
+ timeout client 1s
+ timeout server 1s
+
+ frontend fe
+ # This frontend matches two base64 encoded values and does not need to
+ # handle null bytes.
+
+ bind "fd@${fe}"
+
+ #### requests
+ http-request set-var(txn.hash) req.hdr(hash)
+ http-request set-var(txn.raw) req.hdr(raw)
+
+ acl is_match var(txn.raw),sha1,base64,secure_memcmp(txn.hash)
+
+ http-response set-header Match true if is_match
+ http-response set-header Match false if !is_match
+
+ default_backend be
+
+ frontend fe2
+ # This frontend matches two binary values, needing to handle null
+ # bytes.
+ bind "fd@${fe2}"
+
+ #### requests
+ http-request set-var(txn.hash) req.hdr(hash),b64dec
+ http-request set-var(txn.raw) req.hdr(raw)
+
+ acl is_match var(txn.raw),sha1,secure_memcmp(txn.hash)
+
+ http-response set-header Match true if is_match
+ http-response set-header Match false if !is_match
+
+ default_backend be2
+
+ backend be
+ server s1 ${s1_addr}:${s1_port}
+
+ backend be2
+ server s2 ${s2_addr}:${s2_port}
+} -start
+
+client c1 -connect ${h1_fe_sock} {
+ txreq -url "/" \
+ -hdr "Raw: 1" \
+ -hdr "Hash: NWoZK3kTsExUV00Ywo1G5jlUKKs="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.match == "true"
+ txreq -url "/" \
+ -hdr "Raw: 2" \
+ -hdr "Hash: 2kuSN7rMzfGcB2DKt67EqDWQELA="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.match == "true"
+ txreq -url "/" \
+ -hdr "Raw: 2" \
+ -hdr "Hash: 2kuSN7rMzfGcB2DKt67EqDWQELX="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.match == "false"
+ txreq -url "/" \
+ -hdr "Raw: 3" \
+ -hdr "Hash: 2kuSN7rMzfGcB2DKt67EqDWQELA="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.match == "false"
+} -run
+
+client c2 -connect ${h1_fe2_sock} {
+ txreq -url "/" \
+ -hdr "Raw: 1" \
+ -hdr "Hash: NWoZK3kTsExUV00Ywo1G5jlUKKs="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.match == "true"
+ txreq -url "/" \
+ -hdr "Raw: 2" \
+ -hdr "Hash: 2kuSN7rMzfGcB2DKt67EqDWQELA="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.match == "true"
+ txreq -url "/" \
+ -hdr "Raw: 2" \
+ -hdr "Hash: 2kuSN7rMzfGcB2DKt67EqDWQELX="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.match == "false"
+ txreq -url "/" \
+ -hdr "Raw: 3" \
+ -hdr "Hash: 2kuSN7rMzfGcB2DKt67EqDWQELA="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.match == "false"
+
+ # Test for values with leading nullbytes.
+ txreq -url "/" \
+ -hdr "Raw: 6132845" \
+ -hdr "Hash: AAAAVaeL9nNcSok1j6sd40EEw8s="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.match == "true"
+ txreq -url "/" \
+ -hdr "Raw: 49177200" \
+ -hdr "Hash: AAAA9GLglTNv2JoMv2n/w9Xadhc="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.match == "true"
+ txreq -url "/" \
+ -hdr "Raw: 6132845" \
+ -hdr "Hash: AAAA9GLglTNv2JoMv2n/w9Xadhc="
+ rxresp
+ expect resp.status == 200
+ expect resp.http.match == "false"
+} -run
diff --git a/src/sample.c b/src/sample.c
index b223547f0..f10f4395b 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -3065,7 +3065,7 @@ static int smp_check_concat(struct arg *args, struct
sample_conv *conv,
return 1;
}
-/* compares string with a variable containing a string. Return value
+/* Compares string with a variable containing a string. Return value
* is compatible with strcmp(3)'s return value.
*/
static int sample_conv_strcmp(const struct arg *arg_p, struct sample *smp,
void *private)
@@ -3099,6 +3099,41 @@ static int sample_conv_strcmp(const struct arg *arg_p,
struct sample *smp, void
return 1;
}
+#ifdef USE_OPENSSL
+/* Compares bytestring with a variable containing a bytestring. Return value
+ * is `true` if both bytestrings are bytewise identical and `false` otherwise.
+ *
+ * Comparison will be performed in constant time if both bytestrings are of
+ * the same length. If the lengths differ execution time will not be constant.
+ */
+static int sample_conv_secure_memcmp(const struct arg *arg_p, struct sample
*smp, void *private)
+{
+ struct sample tmp;
+ int result;
+
+ smp_set_owner(&tmp, smp->px, smp->sess, smp->strm, smp->opt);
+ if (arg_p[0].type != ARGT_VAR)
+ return 0;
+ if (!vars_get_by_desc(&arg_p[0].data.var, &tmp))
+ return 0;
+ if (!sample_casts[tmp.data.type][SMP_T_BIN](&tmp))
+ return 0;
+
+ if (smp->data.u.str.data != tmp.data.u.str.data) {
+ smp->data.u.sint = 0;
+ smp->data.type = SMP_T_BOOL;
+ return 1;
+ }
+
+ /* The following comparison is performed in constant time. */
+ result = CRYPTO_memcmp(smp->data.u.str.area, tmp.data.u.str.area,
smp->data.u.str.data);
+
+ smp->data.u.sint = result == 0;
+ smp->data.type = SMP_T_BOOL;
+ return 1;
+}
+#endif
+
#define GRPC_MSG_COMPRESS_FLAG_SZ 1 /* 1 byte */
#define GRPC_MSG_LENGTH_SZ 4 /* 4 bytes */
#define GRPC_MSG_HEADER_SZ (GRPC_MSG_COMPRESS_FLAG_SZ +
GRPC_MSG_LENGTH_SZ)
@@ -3186,6 +3221,23 @@ static int smp_check_strcmp(struct arg *args, struct
sample_conv *conv,
return 0;
}
+#ifdef USE_OPENSSL
+/* This function checks the "secure_memcmp" converter's arguments and extracts
the
+ * variable name and its scope.
+ */
+static int smp_check_secure_memcmp(struct arg *args, struct sample_conv *conv,
+ const char *file, int line, char **err)
+{
+ /* Try to decode a variable. */
+ if (vars_check_arg(&args[0], NULL))
+ return 1;
+
+ memprintf(err, "failed to register variable name '%s'",
+ args[0].data.str.area);
+ return 0;
+}
+#endif
+
/**/
static int sample_conv_htonl(const struct arg *arg_p, struct sample *smp, void
*private)
{
@@ -3727,6 +3779,9 @@ static struct sample_conv_kw_list sample_conv_kws = {ILH,
{
#endif
{ "concat", sample_conv_concat, ARG3(1,STR,STR,STR),
smp_check_concat, SMP_T_STR, SMP_T_STR },
{ "strcmp", sample_conv_strcmp, ARG1(1,STR), smp_check_strcmp,
SMP_T_STR, SMP_T_SINT },
+#ifdef USE_OPENSSL
+ { "secure_memcmp", sample_conv_secure_memcmp, ARG1(1,STR),
smp_check_secure_memcmp, SMP_T_BIN, SMP_T_BOOL },
+#endif
/* gRPC converters. */
{ "ungrpc", sample_conv_ungrpc, ARG2(1,PBUF_FNUM,STR),
sample_conv_protobuf_check, SMP_T_BIN, SMP_T_BIN },
--
2.27.0
