From bd181f27d4d0c16c500c9f49394213d1fbad1f09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 5 Feb 2018 09:48:38 +0100 Subject: [PATCH 1/4] test: add a simple smoke test for string_hashsum() This is enough to show memory leakages pointed out by Stef Bon . --- src/test/meson.build | 5 +++++ src/test/test-gcrypt-util.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 src/test/test-gcrypt-util.c diff --git a/src/test/meson.build b/src/test/meson.build index 1db8aa107d7..4f28ef87222 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -643,6 +643,11 @@ tests += [ [], []], + [['src/test/test-gcrypt-util.c'], + [], + [], + 'HAVE_GCRYPT'], + [['src/test/test-nss.c'], [], [libdl], diff --git a/src/test/test-gcrypt-util.c b/src/test/test-gcrypt-util.c new file mode 100644 index 00000000000..d7d13a13037 --- /dev/null +++ b/src/test/test-gcrypt-util.c @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: LGPL-2.1+ + * Copyright 2018 Zbigniew Jędrzejewski-Szmek + */ + +#include "alloc-util.h" +#include "gcrypt-util.h" +#include "macro.h" +#include "string-util.h" + +static void test_string_hashsum(void) { + _cleanup_free_ char *out1 = NULL, *out2 = NULL, *out3 = NULL, *out4 = NULL; + + assert_se(string_hashsum("asdf", 4, GCRY_MD_SHA224, &out1) == 0); + /* echo -n 'asdf' | sha224sum - */ + assert_se(streq(out1, "7872a74bcbf298a1e77d507cd95d4f8d96131cbbd4cdfc571e776c8a")); + + assert_se(string_hashsum("asdf", 4, GCRY_MD_SHA256, &out2) == 0); + /* echo -n 'asdf' | sha256sum - */ + assert_se(streq(out2, "f0e4c2f76c58916ec258f246851bea091d14d4247a2fc3e18694461b1816e13b")); + + assert_se(string_hashsum("", 0, GCRY_MD_SHA224, &out3) == 0); + /* echo -n '' | sha224sum - */ + assert_se(streq(out3, "d14a028c2a3a2bc9476102bb288234c415a2b01f828ea62ac5b3e42f")); + + assert_se(string_hashsum("", 0, GCRY_MD_SHA256, &out4) == 0); + /* echo -n '' | sha256sum - */ + assert_se(streq(out4, "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")); +} + +int main(int argc, char **argv) { + test_string_hashsum(); + + return 0; +} From bd944e6e1854df9aa6002fed015226d6298029b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 5 Feb 2018 09:54:57 +0100 Subject: [PATCH 2/4] gcrypt-util: fix memleak --- src/basic/gcrypt-util.c | 2 +- src/basic/gcrypt-util.h | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/basic/gcrypt-util.c b/src/basic/gcrypt-util.c index 1bfb776725c..34d0d399b7c 100644 --- a/src/basic/gcrypt-util.c +++ b/src/basic/gcrypt-util.c @@ -42,7 +42,7 @@ void initialize_libgcrypt(bool secmem) { } int string_hashsum(const char *s, size_t len, int md_algorithm, char **out) { - gcry_md_hd_t md = NULL; + _cleanup_(gcry_md_closep) gcry_md_hd_t md = NULL; size_t hash_size; void *hash; char *enc; diff --git a/src/basic/gcrypt-util.h b/src/basic/gcrypt-util.h index 69faf08e56f..714cae673bb 100644 --- a/src/basic/gcrypt-util.h +++ b/src/basic/gcrypt-util.h @@ -20,6 +20,8 @@ along with systemd; If not, see . ***/ +#pragma once + #include #include #include @@ -27,8 +29,12 @@ #if HAVE_GCRYPT #include +#include "macro.h" + void initialize_libgcrypt(bool secmem); int string_hashsum(const char *s, size_t len, int md_algorithm, char **out); + +DEFINE_TRIVIAL_CLEANUP_FUNC(gcry_md_hd_t, gcry_md_close); #endif static inline int string_hashsum_sha224(const char *s, size_t len, char **out) { From 8530efc1c3d600e9ed23175c41ff8a0e79dab327 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 5 Feb 2018 10:06:36 +0100 Subject: [PATCH 3/4] resolved: fix memleak of gcrypt context on error Bug found by Stef Bon . Thanks! --- src/resolve/resolved-dns-dnssec.c | 41 ++++++++++--------------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/src/resolve/resolved-dns-dnssec.c b/src/resolve/resolved-dns-dnssec.c index e3eca7e62cc..05941473b0e 100644 --- a/src/resolve/resolved-dns-dnssec.c +++ b/src/resolve/resolved-dns-dnssec.c @@ -716,7 +716,7 @@ int dnssec_verify_rrset( uint8_t wire_format_name[DNS_WIRE_FOMAT_HOSTNAME_MAX]; DnsResourceRecord **list, *rr; const char *source, *name; - gcry_md_hd_t md = NULL; + _cleanup_(gcry_md_closep) gcry_md_hd_t md = NULL; int r, md_algorithm; size_t k, n = 0; size_t sig_size = 0; @@ -841,13 +841,13 @@ int dnssec_verify_rrset( r = dns_name_to_wire_format(rrsig->rrsig.signer, wire_format_name, sizeof(wire_format_name), true); if (r < 0) - goto finish; + return r; fwrite(wire_format_name, 1, r, f); /* Convert the source of synthesis into wire format */ r = dns_name_to_wire_format(source, wire_format_name, sizeof(wire_format_name), true); if (r < 0) - goto finish; + return r; for (k = 0; k < n; k++) { size_t l; @@ -885,26 +885,20 @@ int dnssec_verify_rrset( #endif case DNSSEC_ALGORITHM_ED448: *result = DNSSEC_UNSUPPORTED_ALGORITHM; - r = 0; - goto finish; + return 0; default: /* OK, the RRs are now in canonical order. Let's calculate the digest */ md_algorithm = algorithm_to_gcrypt_md(rrsig->rrsig.algorithm); if (md_algorithm == -EOPNOTSUPP) { *result = DNSSEC_UNSUPPORTED_ALGORITHM; - r = 0; - goto finish; - } - if (md_algorithm < 0) { - r = md_algorithm; - goto finish; + return 0; } + if (md_algorithm < 0) + return md_algorithm; gcry_md_open(&md, md_algorithm, 0); - if (!md) { - r = -EIO; - goto finish; - } + if (!md) + return -EIO; hash_size = gcry_md_get_algo_dlen(md_algorithm); assert(hash_size > 0); @@ -912,10 +906,8 @@ int dnssec_verify_rrset( gcry_md_write(md, sig_data, sig_size); hash = gcry_md_read(md, 0); - if (!hash) { - r = -EIO; - goto finish; - } + if (!hash) + return -EIO; } switch (rrsig->rrsig.algorithm) { @@ -950,9 +942,8 @@ int dnssec_verify_rrset( break; #endif } - if (r < 0) - goto finish; + return r; /* Now, fix the ttl, expiry, and remember the synthesizing source and the signer */ if (r > 0) @@ -965,13 +956,7 @@ int dnssec_verify_rrset( else *result = DNSSEC_VALIDATED; - r = 0; - -finish: - if (md) - gcry_md_close(md); - - return r; + return 0; } int dnssec_rrsig_match_dnskey(DnsResourceRecord *rrsig, DnsResourceRecord *dnskey, bool revoked_ok) { From 15c533103a75af7266c557e0bf32a403d8635430 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 5 Feb 2018 10:07:39 +0100 Subject: [PATCH 4/4] resolved: use _cleanup_ in one more place No functional change. --- src/resolve/resolved-dns-dnssec.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/resolve/resolved-dns-dnssec.c b/src/resolve/resolved-dns-dnssec.c index 05941473b0e..1bd2c93a337 100644 --- a/src/resolve/resolved-dns-dnssec.c +++ b/src/resolve/resolved-dns-dnssec.c @@ -1167,7 +1167,7 @@ static int digest_to_gcrypt_md(uint8_t algorithm) { int dnssec_verify_dnskey_by_ds(DnsResourceRecord *dnskey, DnsResourceRecord *ds, bool mask_revoke) { char owner_name[DNSSEC_CANONICAL_HOSTNAME_MAX]; - gcry_md_hd_t md = NULL; + _cleanup_(gcry_md_closep) gcry_md_hd_t md = NULL; size_t hash_size; int md_algorithm, r; void *result; @@ -1223,16 +1223,10 @@ int dnssec_verify_dnskey_by_ds(DnsResourceRecord *dnskey, DnsResourceRecord *ds, gcry_md_write(md, dnskey->dnskey.key, dnskey->dnskey.key_size); result = gcry_md_read(md, 0); - if (!result) { - r = -EIO; - goto finish; - } + if (!result) + return -EIO; - r = memcmp(result, ds->ds.digest, ds->ds.digest_size) != 0; - -finish: - gcry_md_close(md); - return r; + return memcmp(result, ds->ds.digest, ds->ds.digest_size) != 0; } int dnssec_verify_dnskey_by_ds_search(DnsResourceRecord *dnskey, DnsAnswer *validated_ds) {