Skip to content

Commit

Permalink
mscng signatures: don't free hash/hashobject memory before closing th…
Browse files Browse the repository at this point in the history
…e hash handle

The new order of cleanup matches MSDN documentation and fixes this
drmemory error:

	Error lsh123#1: UNADDRESSABLE ACCESS of freed memory: reading 0x0092c5e0-0x0092c5e4 4 byte(s)
	# 0 bcrypt.dll!BCryptDestroyHash              +0x74     (0x72701ad3 <bcrypt.dll+0x1ad3>)
	# 1 bcrypt.dll!BCryptDestroyHash              +0xe      (0x72701a6e <bcrypt.dll+0x1a6e>)
	# 2 libxmlsec-mscng.dll!xmlSecMSCngSignatureFinalize [xmlsec\src\mscng\signatures.c:274]
	# 3 libxmlsec.dll!xmlSecTransformDestroy       [xmlsec\src\transforms.c:1271]
	# 4 libxmlsec.dll!xmlSecTransformCtxReset      [xmlsec\src\transforms.c:394]
	# 5 libxmlsec.dll!xmlSecTransformCtxFinalize   [xmlsec\src\transforms.c:361]
	# 6 libxmlsec.dll!xmlSecDSigCtxFinalize        [xmlsec\src\xmldsig.c:178]
	# 7 xmlSecAppVerifyFile                        [xmlsec\apps\xmlsec.c:1324]
	# 8 main                                       [xmlsec\apps\xmlsec.c:1091]
	Note: @0:00:02.891 in thread 2640
	Note: prev lower malloc:  0x0092c5a0-0x0092c5bc
	Note: 0x0092c5e0-0x0092c5e4 overlaps memory 0x0092c5e0-0x0092c692 that was freed here:
	Note: # 0 replace_free                               [d:\drmemory_package\common\alloc_replace.c:2706]
	Note: # 1 libxmlsec-mscng.dll!xmlSecMSCngSignatureFinalize [xmlsec\src\mscng\signatures.c:270]
	Note: # 2 libxmlsec.dll!xmlSecTransformDestroy       [xmlsec\src\transforms.c:1271]
	Note: # 3 libxmlsec.dll!xmlSecTransformCtxReset      [xmlsec\src\transforms.c:394]
	Note: # 4 libxmlsec.dll!xmlSecTransformCtxFinalize   [xmlsec\src\transforms.c:361]
	Note: # 5 libxmlsec.dll!xmlSecDSigCtxFinalize        [xmlsec\src\xmldsig.c:178]
	Note: # 6 xmlSecAppVerifyFile                        [xmlsec\apps\xmlsec.c:1324]
	Note: # 7 main                                       [xmlsec\apps\xmlsec.c:1091]
	Note: instruction: cmp    (%eax) $0x00000018
  • Loading branch information
vmiklos committed Jun 17, 2018
1 parent 30f7dd4 commit ce8c6f8
Showing 1 changed file with 13 additions and 5 deletions.
18 changes: 13 additions & 5 deletions src/mscng/signatures.c
Original file line number Diff line number Diff line change
Expand Up @@ -258,20 +258,28 @@ static void xmlSecMSCngSignatureFinalize(xmlSecTransformPtr transform) {
xmlSecKeyDataDestroy(ctx->data);
}

if(ctx->pbHash != NULL) {
xmlFree(ctx->pbHash);
}
// MSDN documents at
// https://msdn.microsoft.com/en-us/library/windows/desktop/aa376217(v=vs.85).aspx
// that the order of cleanup should be:
// - algo handle
// - hash handle
// - hash object pointer
// - hash pointer

if(ctx->hHashAlg != 0) {
BCryptCloseAlgorithmProvider(ctx->hHashAlg, 0);
}

if(ctx->hHash != 0) {
BCryptDestroyHash(ctx->hHash);
}

if(ctx->pbHashObject != NULL) {
xmlFree(ctx->pbHashObject);
}

if(ctx->hHash != 0) {
BCryptDestroyHash(ctx->hHash);
if(ctx->pbHash != NULL) {
xmlFree(ctx->pbHash);
}

memset(ctx, 0, sizeof(xmlSecMSCngSignatureCtx));
Expand Down

0 comments on commit ce8c6f8

Please sign in to comment.