Hello everyone, attached you will find a new patch which takes Daniel's feedback into account: - Replaced random overwrite with secure zeroing using SecureZeroMemory [2] - Option renamed from --disable-memory-overwrite to --disable-clear-memory - Define renamed from LIBSSH2_MEMORY_OVERWRITE to LIBSSH2_CLEAR_MEMORY
On 18.05.2014 19:12, Marc Hoersken wrote: > On 18.05.2014 19:02, Daniel Stenberg wrote: >> This option only disables the random fill of the free data, it still >> overwrites memory - only with zeros instead. So it doesn't disable >> memory overwrite at all. > You are right, [snip] > >> A question though: is there really anyone who suggests that it is >> safer to fill the data with random data rather than just zeros? I just >> can't see the point with doing such a slow operation and waste random >> seed on this. > I don't have specific expertise in this area, but I think a reason could > be that a compiler might be tempted to optimize memset(buf, 0, len) out. > > Looking at the memory erasure procedure of the Tails operating system > [1], it seems like overwriting with zeros is enough. The feature now overwrites the data in internal memory buffers with zeros using the secure functionality provided by the OS. If this feature is ever expanded to other backends and of course different operating systems, such a function would need to be provided and used. Thanks, Daniel! Please review the new patch. Any feedback is welcome. I guess the patch should also include some warning about it only being available for Windows with WinCNG for now before being merged at the current stage of the implementation. Best regards, Marc [1] https://tails.boum.org/contribute/design/memory_erasure/ [2] http://msdn.microsoft.com/en-us/library/windows/desktop/aa366877.aspx
>From 137b97b84137d1aee7849a31a15e1e4786dfb0f6 Mon Sep 17 00:00:00 2001 From: Marc Hoersken <[email protected]> Date: Thu, 19 Jun 2014 18:54:10 +0200 Subject: [PATCH] wincng: Added explicit clear memory feature to WinCNG backend This re-introduces the original feature proposed during the development of the WinCNG crypto backend. It still needs to be added to libssh2 itself and probably other backends. Memory is cleared using the function SecureZeroMemory which is available on Windows systems, just like the WinCNG backend. --- configure.ac | 7 +++ src/wincng.c | 146 +++++++++++++++++++++++++++++++++-------------------------- 2 files changed, 88 insertions(+), 65 deletions(-) diff --git a/configure.ac b/configure.ac index ba4dd7a..cbc2312 100644 --- a/configure.ac +++ b/configure.ac @@ -197,6 +197,13 @@ if test "$GEX_NEW" != "no"; then AC_DEFINE(LIBSSH2_DH_GEX_NEW, 1, [Enable newer diffie-hellman-group-exchange-sha1 syntax]) fi +AC_ARG_ENABLE(clear-memory, + AC_HELP_STRING([--disable-clear-memory],[Disable clearing of memory before being freed]), + [CLEAR_MEMORY=$enableval]) +if test "$CLEAR_MEMORY" != "no"; then + AC_DEFINE(LIBSSH2_CLEAR_MEMORY, 1, [Enable clearing of memory before being freed]) +fi + dnl ************************************************************ dnl option to switch on compiler debug options dnl diff --git a/src/wincng.c b/src/wincng.c index b575350..5dfca52 100644 --- a/src/wincng.c +++ b/src/wincng.c @@ -280,6 +280,20 @@ _libssh2_wincng_random(void *buf, int len) return BCRYPT_SUCCESS(ret) ? 0 : -1; } +static void +_libssh2_wincng_mfree(void *buf, int len) +{ + if (!buf) + return; + +#ifdef LIBSSH2_CLEAR_MEMORY + if (len > 0) + SecureZeroMemory(buf, len); +#endif + + free(buf); +} + /*******************************************************************/ /* @@ -322,7 +336,7 @@ _libssh2_wincng_hash_init(_libssh2_wincng_hash_ctx *ctx, pbHashObject, dwHashObject, key, keylen, 0); if (!BCRYPT_SUCCESS(ret)) { - free(pbHashObject); + _libssh2_wincng_mfree(pbHashObject, dwHashObject); return -1; } @@ -355,11 +369,11 @@ _libssh2_wincng_hash_final(_libssh2_wincng_hash_ctx *ctx, ret = BCryptFinishHash(ctx->hHash, hash, ctx->cbHash, 0); BCryptDestroyHash(ctx->hHash); + ctx->hHash = NULL; - if (ctx->pbHashObject) - free(ctx->pbHashObject); - - memset(ctx, 0, sizeof(_libssh2_wincng_hash_ctx)); + _libssh2_wincng_mfree(ctx->pbHashObject, ctx->dwHashObject); + ctx->pbHashObject = NULL; + ctx->dwHashObject = 0; return ret; } @@ -403,11 +417,11 @@ void _libssh2_wincng_hmac_cleanup(_libssh2_wincng_hash_ctx *ctx) { BCryptDestroyHash(ctx->hHash); + ctx->hHash = NULL; - if (ctx->pbHashObject) - free(ctx->pbHashObject); - - memset(ctx, 0, sizeof(_libssh2_wincng_hash_ctx)); + _libssh2_wincng_mfree(ctx->pbHashObject, ctx->dwHashObject); + ctx->pbHashObject = NULL; + ctx->dwHashObject = 0; } @@ -449,17 +463,17 @@ _libssh2_wincng_key_sha1_verify(_libssh2_wincng_key_ctx *ctx, _libssh2_wincng.hAlgHashSHA1, hash, hashlen); - free(data); + _libssh2_wincng_mfree(data, datalen); if (ret) { - free(hash); + _libssh2_wincng_mfree(hash, hashlen); return -1; } datalen = sig_len; data = malloc(datalen); if (!data) { - free(hash); + _libssh2_wincng_mfree(hash, hashlen); return -1; } @@ -474,8 +488,8 @@ _libssh2_wincng_key_sha1_verify(_libssh2_wincng_key_ctx *ctx, ret = BCryptVerifySignature(ctx->hKey, pPaddingInfo, hash, hashlen, data, datalen, flags); - free(hash); - free(data); + _libssh2_wincng_mfree(hash, hashlen); + _libssh2_wincng_mfree(data, datalen); return BCRYPT_SUCCESS(ret) ? 0 : -1; } @@ -568,7 +582,7 @@ _libssh2_wincng_asn_decode(unsigned char *pbEncoded, pbEncoded, cbEncoded, 0, NULL, pbDecoded, &cbDecoded); if (!ret) { - free(pbDecoded); + _libssh2_wincng_mfree(pbDecoded, cbDecoded); return -1; } @@ -638,7 +652,7 @@ _libssh2_wincng_asn_decode_bn(unsigned char *pbEncoded, *ppbDecoded = pbDecoded; *pcbDecoded = cbDecoded; } - free(pbInteger); + _libssh2_wincng_mfree(pbInteger, cbInteger); } return ret; @@ -683,10 +697,10 @@ _libssh2_wincng_asn_decode_bns(unsigned char *pbEncoded, *pcbCount = length; } else { for (length = 0; length < index; length++) { - if (rpbDecoded[length]) { - free(rpbDecoded[length]); - rpbDecoded[length] = NULL; - } + _libssh2_wincng_mfree(rpbDecoded[length], + rcbDecoded[length]); + rpbDecoded[length] = NULL; + rcbDecoded[length] = 0; } free(rpbDecoded); free(rcbDecoded); @@ -699,7 +713,7 @@ _libssh2_wincng_asn_decode_bns(unsigned char *pbEncoded, ret = -1; } - free(pbDecoded); + _libssh2_wincng_mfree(pbDecoded, cbDecoded); } return ret; @@ -845,7 +859,7 @@ _libssh2_wincng_rsa_new(libssh2_rsa_ctx **rsa, ret = BCryptImportKeyPair(_libssh2_wincng.hAlgRSA, NULL, lpszBlobType, &hKey, key, keylen, 0); if (!BCRYPT_SUCCESS(ret)) { - free(key); + _libssh2_wincng_mfree(key, keylen); return -1; } @@ -853,7 +867,7 @@ _libssh2_wincng_rsa_new(libssh2_rsa_ctx **rsa, *rsa = malloc(sizeof(libssh2_rsa_ctx)); if (!(*rsa)) { BCryptDestroyKey(hKey); - free(key); + _libssh2_wincng_mfree(key, keylen); return -1; } @@ -889,7 +903,7 @@ _libssh2_wincng_rsa_new_private(libssh2_rsa_ctx **rsa, PKCS_RSA_PRIVATE_KEY, &pbStructInfo, &cbStructInfo); - free(pbEncoded); + _libssh2_wincng_mfree(pbEncoded, cbEncoded); if (ret) { return -1; @@ -900,7 +914,7 @@ _libssh2_wincng_rsa_new_private(libssh2_rsa_ctx **rsa, LEGACY_RSAPRIVATE_BLOB, &hKey, pbStructInfo, cbStructInfo, 0); if (!BCRYPT_SUCCESS(ret)) { - free(pbStructInfo); + _libssh2_wincng_mfree(pbStructInfo, cbStructInfo); return -1; } @@ -908,7 +922,7 @@ _libssh2_wincng_rsa_new_private(libssh2_rsa_ctx **rsa, *rsa = malloc(sizeof(libssh2_rsa_ctx)); if (!(*rsa)) { BCryptDestroyKey(hKey); - free(pbStructInfo); + _libssh2_wincng_mfree(pbStructInfo, cbStructInfo); return -1; } @@ -982,7 +996,7 @@ _libssh2_wincng_rsa_sha1_sign(LIBSSH2_SESSION *session, ret = STATUS_NO_MEMORY; } - free(data); + _libssh2_wincng_mfree(data, datalen); return BCRYPT_SUCCESS(ret) ? 0 : -1; } @@ -994,12 +1008,10 @@ _libssh2_wincng_rsa_free(libssh2_rsa_ctx *rsa) return; BCryptDestroyKey(rsa->hKey); + rsa->hKey = NULL; - if (rsa->pbKeyObject) - free(rsa->pbKeyObject); - - memset(rsa, 0, sizeof(libssh2_rsa_ctx)); - free(rsa); + _libssh2_wincng_mfree(rsa->pbKeyObject, rsa->cbKeyObject); + _libssh2_wincng_mfree(rsa, sizeof(libssh2_rsa_ctx)); } @@ -1093,7 +1105,7 @@ _libssh2_wincng_dsa_new(libssh2_dsa_ctx **dsa, ret = BCryptImportKeyPair(_libssh2_wincng.hAlgDSA, NULL, lpszBlobType, &hKey, key, keylen, 0); if (!BCRYPT_SUCCESS(ret)) { - free(key); + _libssh2_wincng_mfree(key, keylen); return -1; } @@ -1101,7 +1113,7 @@ _libssh2_wincng_dsa_new(libssh2_dsa_ctx **dsa, *dsa = malloc(sizeof(libssh2_dsa_ctx)); if (!(*dsa)) { BCryptDestroyKey(hKey); - free(key); + _libssh2_wincng_mfree(key, keylen); return -1; } @@ -1135,7 +1147,7 @@ _libssh2_wincng_dsa_new_private(libssh2_dsa_ctx **dsa, ret = _libssh2_wincng_asn_decode_bns(pbEncoded, cbEncoded, &rpbDecoded, &rcbDecoded, &length); - free(pbEncoded); + _libssh2_wincng_mfree(pbEncoded, cbEncoded); if (ret) { return -1; @@ -1154,10 +1166,9 @@ _libssh2_wincng_dsa_new_private(libssh2_dsa_ctx **dsa, } for (index = 0; index < length; index++) { - if (rpbDecoded[index]) { - free(rpbDecoded[index]); - rpbDecoded[index] = NULL; - } + _libssh2_wincng_mfree(rpbDecoded[index], rcbDecoded[index]); + rpbDecoded[index] = NULL; + rcbDecoded[index] = 0; } free(rpbDecoded); @@ -1215,14 +1226,14 @@ _libssh2_wincng_dsa_sha1_sign(libssh2_dsa_ctx *dsa, memcpy(sig_fixed, sig, siglen); } - free(sig); + _libssh2_wincng_mfree(sig, siglen); } else ret = STATUS_NO_MEMORY; } else ret = STATUS_NO_MEMORY; } - free(data); + _libssh2_wincng_mfree(data, datalen); return BCRYPT_SUCCESS(ret) ? 0 : -1; } @@ -1234,12 +1245,10 @@ _libssh2_wincng_dsa_free(libssh2_dsa_ctx *dsa) return; BCryptDestroyKey(dsa->hKey); + dsa->hKey = NULL; - if (dsa->pbKeyObject) - free(dsa->pbKeyObject); - - memset(dsa, 0, sizeof(libssh2_dsa_ctx)); - free(dsa); + _libssh2_wincng_mfree(dsa->pbKeyObject, dsa->cbKeyObject); + _libssh2_wincng_mfree(dsa, sizeof(libssh2_dsa_ctx)); } #endif @@ -1289,7 +1298,7 @@ _libssh2_wincng_pub_priv_keyfile(LIBSSH2_SESSION *session, ret = _libssh2_wincng_asn_decode_bns(pbEncoded, cbEncoded, &rpbDecoded, &rcbDecoded, &length); - free(pbEncoded); + _libssh2_wincng_mfree(pbEncoded, cbEncoded); if (ret) { return -1; @@ -1362,10 +1371,9 @@ _libssh2_wincng_pub_priv_keyfile(LIBSSH2_SESSION *session, for (index = 0; index < length; index++) { - if (rpbDecoded[index]) { - free(rpbDecoded[index]); - rpbDecoded[index] = NULL; - } + _libssh2_wincng_mfree(rpbDecoded[index], rcbDecoded[index]); + rpbDecoded[index] = NULL; + rcbDecoded[index] = 0; } free(rpbDecoded); @@ -1461,10 +1469,10 @@ _libssh2_wincng_cipher_init(_libssh2_cipher_ctx *ctx, ret = BCryptImportKey(*type.phAlg, NULL, BCRYPT_KEY_DATA_BLOB, &hKey, pbKeyObject, dwKeyObject, key, keylen, 0); - free(key); + _libssh2_wincng_mfree(key, keylen); if (!BCRYPT_SUCCESS(ret)) { - free(pbKeyObject); + _libssh2_wincng_mfree(pbKeyObject, dwKeyObject); return -1; } @@ -1472,7 +1480,7 @@ _libssh2_wincng_cipher_init(_libssh2_cipher_ctx *ctx, pbIV = malloc(dwBlockLength); if (!pbIV) { BCryptDestroyKey(hKey); - free(pbKeyObject); + _libssh2_wincng_mfree(pbKeyObject, dwKeyObject); return -1; } dwIV = dwBlockLength; @@ -1531,7 +1539,7 @@ _libssh2_wincng_cipher_crypt(_libssh2_cipher_ctx *ctx, memcpy(block, pbOutput, cbOutput); } - free(pbOutput); + _libssh2_wincng_mfree(pbOutput, cbOutput); } else ret = STATUS_NO_MEMORY; } @@ -1543,13 +1551,11 @@ void _libssh2_wincng_cipher_dtor(_libssh2_cipher_ctx *ctx) { BCryptDestroyKey(ctx->hKey); + ctx->hKey = NULL; - if (ctx->pbKeyObject) { - free(ctx->pbKeyObject); - ctx->pbKeyObject = NULL; - } - - memset(ctx, 0, sizeof(_libssh2_cipher_ctx)); + _libssh2_wincng_mfree(ctx->pbKeyObject, ctx->dwKeyObject); + ctx->pbKeyObject = NULL; + ctx->dwKeyObject = 0; } @@ -1581,6 +1587,12 @@ _libssh2_wincng_bignum_resize(_libssh2_bn *bn, unsigned long length) if (length == bn->length) return 0; +#ifdef LIBSSH2_CLEAR_MEMORY + if (bn->bignum && bn->length > 0 && length < bn->length) { + SecureZeroMemory(bn->bignum + length, bn->length - length); + } +#endif + bignum = realloc(bn->bignum, length); if (!bignum) return -1; @@ -1688,7 +1700,7 @@ _libssh2_wincng_bignum_mod_exp(_libssh2_bn *r, r->bignum, r->length, &offset, BCRYPT_PAD_NONE); - free(bignum); + _libssh2_wincng_mfree(bignum, length); if (BCRYPT_SUCCESS(ret)) { _libssh2_wincng_bignum_resize(r, offset); @@ -1702,7 +1714,7 @@ _libssh2_wincng_bignum_mod_exp(_libssh2_bn *r, BCryptDestroyKey(hKey); } - free(key); + _libssh2_wincng_mfree(key, keylen); return BCRYPT_SUCCESS(ret) ? 0 : -1; } @@ -1780,6 +1792,10 @@ _libssh2_wincng_bignum_from_bin(_libssh2_bn *bn, unsigned long len, if (offset > 0) { memmove(bn->bignum, bn->bignum + offset, length); +#ifdef LIBSSH2_CLEAR_MEMORY + SecureZeroMemory(bn->bignum + length, offset); +#endif + bignum = realloc(bn->bignum, length); if (bignum) { bn->bignum = bignum; @@ -1801,11 +1817,11 @@ _libssh2_wincng_bignum_free(_libssh2_bn *bn) { if (bn) { if (bn->bignum) { - free(bn->bignum); + _libssh2_wincng_mfree(bn->bignum, bn->length); bn->bignum = NULL; } bn->length = 0; - free(bn); + _libssh2_wincng_mfree(bn, sizeof(_libssh2_bn)); } } -- 1.9.2.msysgit.0
_______________________________________________ libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
