Bug 501191 - [PEM] psession : memory leak and and incorrect return values in psession:make_key and pem_mdSession_Login
[PEM] psession : memory leak and and incorrect return values in psession:make...
Status: CLOSED CURRENTRELEASE
Product: NSS - Network Security Services
Classification: Retired
Component: Libraries (Show other bugs)
unspecified
All Linux
medium Severity medium
: ---
: ---
Assigned To: Elio Maldonado Batiz
:
: 362231 (view as bug list)
Depends On:
Blocks: 501138
  Show dependency treegraph
 
Reported: 2009-05-17 15:12 EDT by Elio Maldonado Batiz
Modified: 2009-08-06 11:54 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-08-06 11:54:15 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Deallocate md5 context, zeroize working buffer prior to using it (693 bytes, patch)
2009-05-17 15:15 EDT, Elio Maldonado Batiz
no flags Details | Diff
Fix leak, clear working buffer prior to using it, fixed size. (685 bytes, patch)
2009-05-17 15:19 EDT, Elio Maldonado Batiz
no flags Details | Diff
Fix leak in make_key and pem_mdSession_Login and some return codes (1.91 KB, patch)
2009-05-18 17:30 EDT, Elio Maldonado Batiz
no flags Details | Diff
Fixes mem leaks in make_key and pem_mdSession, fixed error codes returned (2.03 KB, patch)
2009-05-18 19:58 EDT, Elio Maldonado Batiz
no flags Details | Diff
Fix leaks in make_key and md_SessionLogin plus error codes (2.04 KB, patch)
2009-05-19 12:23 EDT, Elio Maldonado Batiz
no flags Details | Diff

  None (edit)
Description Elio Maldonado Batiz 2009-05-17 15:12:03 EDT
Description of problem: The make_key function creates an M5Context object and doesn't destroy it. Additionally it first doesn't clear the temporary buffer it uses for computing the hash.


Version-Release number of selected component (if applicable): 3.12.2


How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:
Comment 1 Elio Maldonado Batiz 2009-05-17 15:15:38 EDT
Created attachment 344351 [details]
Deallocate md5 context, zeroize working buffer prior to using it
Comment 2 Elio Maldonado Batiz 2009-05-17 15:19:30 EDT
Created attachment 344352 [details]
Fix leak, clear working buffer prior to using it, fixed size.
Comment 3 Kamil Dudka 2009-05-17 16:14:32 EDT
Follow-up here:
https://bugzilla.redhat.com/show_bug.cgi?id=500815#

The memset() call is IMO not ultimately necessary because H is not read but only written in the first wheel.
Comment 4 Kamil Dudka 2009-05-18 02:34:41 EDT
Elio, do we need to keep the bug private? I can't see any security risk in psession's memory leaks. Maybe we want also append Rob to the CC list to discuss proposed bug fixes...

I am also in-lining the referred code change from comment #3 here as this is the right place to talk about it:

diff -ruNp psession.c psession.c
--- psession.c  2009-05-17 21:43:26.148306000 +0200
+++ psession.c  2009-05-17 21:45:30.999391394 +0200
@@ -391,6 +391,7 @@ pem_mdSession_Login
     if (rv != SECSuccess)
         goto loser;

+    nss_ZFreeIf(output);
     return CKR_OK;

   loser:
Comment 5 Elio Maldonado Batiz 2009-05-18 14:27:02 EDT
It's now public, I had mistakenly set the security flag. Thanks for catching this. Also, it looks to me that the pem_mdSession_Login function may be miss-reporting memory allocation failures as password failures.
Comment 6 Kamil Dudka 2009-05-18 14:38:53 EDT
Yes, +1 for the split of error codes.
Comment 7 Elio Maldonado Batiz 2009-05-18 17:30:21 EDT
Created attachment 344528 [details]
Fix leak in make_key and pem_mdSession_Login and some return codes

Besides the memory leaks fix, pem_mdSession_Login now returns CKR_HOST_MEMORY on memory allocation failures rather than CKR_PIN_INCORRECT.
Comment 8 Kamil Dudka 2009-05-18 18:08:48 EDT
The memory leak situation seems better now. I'd only mention two nits. The indentation is not consistent. You are using tabs (probably equivalent to 4 spaces) in contrast to the original code which uses only spaces for indentation.

> @@ -391,7 +398,7 @@ pem_mdSession_Login
>      if (rv != SECSuccess)
>          goto loser;

I suggest here:

    if (rv != SECSuccess) {
        rv = CKR_PIN_INCORRECT;
        goto loser;
    }

> +	if (rv == CKR_OK || rv == CKR_HOST_MEMORY)
> +		return rv;
> +

This condition is then not necessary. You can just return the rv value.
Comment 9 Elio Maldonado Batiz 2009-05-18 18:42:50 EDT
(In reply to comment #8)
No problem, I'll switch to spaces to be consistent. By the way, the upstream nss style is actually Mozilla's which will tab-based at 8 but so I may have to re-indent yet again to respect their style :-)  

I see, mapping the return code right at the point we examine the result from decryption simplifies the code below as you rightly point out. 

Another think, I forgot to test the value from DES_CreateContext which could conceivably fail and I should set rv to CK_HOST_MEMORY as I did in the other cases.
Comment 10 Elio Maldonado Batiz 2009-05-18 19:58:15 EDT
Created attachment 344533 [details]
Fixes mem leaks in make_key and pem_mdSession, fixed error codes returned

handle context creation failure, indent with spaces, slightly simpler logic
Comment 11 Kamil Dudka 2009-05-19 04:34:09 EDT
Thanks for considering it!

>      cx = DES_CreateContext((const unsigned char *) mykey, iv,
>                             io->u.key.cipher, PR_FALSE);
> +    if (!ck) {
> +        rv = CKR_HOST_MEMORY;
> +        goto loser;
> +    }

s/ck/cx/

I can still see two lines indented with tabs, but it should be ok. This bug is not about indentation :-)
Comment 12 Elio Maldonado Batiz 2009-05-19 12:23:14 EDT
Created attachment 344649 [details]
Fix leaks in make_key and md_SessionLogin plus error codes

s/ck/cx per last review
Comment 13 Kamil Dudka 2009-05-19 12:35:37 EDT
r+, seems ok to me now...
Comment 14 Rob Crittenden 2009-07-02 11:53:38 EDT
*** Bug 362231 has been marked as a duplicate of this bug. ***
Comment 15 Kamil Dudka 2009-08-06 11:45:18 EDT
The bug is fixed in F-10, F-11 and rawhide. It should be closed probably. Elio?
Comment 16 Elio Maldonado Batiz 2009-08-06 11:54:15 EDT
Closed.

Note You need to log in before you can comment on or make changes to this bug.