Fix invalid malloc failures in `PEM_write_bio_PKCS8PrivateKey*()` #17507

RaisinTen · 2022-01-14T11:33:28Z

When PEM_write_bio_PKCS8PrivateKey() and PEM_write_bio_PKCS8PrivateKey_nid()
were passed empty passphrase strings, OPENSSL_memdup() was incorrectly getting
used for 0 bytes size allocations, which resulted in malloc failures.

Fixes: #17506

Signed-off-by: Darshan Sen [email protected]

Checklist

documentation is added or updated
tests are added or updated

crypto/o_str.c

crypto/passphrase.c

t8m · 2022-01-14T12:08:27Z

In general this is wrong place to fix your problem. This needs to be properly handled at the callers to not try 0 bytes size allocation or memory duplication.

RaisinTen · 2022-01-15T12:24:30Z

In general this is wrong place to fix your problem. This needs to be properly handled at the callers to not try 0 bytes size allocation or memory duplication.

@t8m thanks, updated accordingly.

t8m

LGTM. To be able to accept the PR we will need a CLA signed from you. Please follow the process described at https://www.openssl.org/policies/cla.html

Too early

t8m · 2022-01-18T10:50:14Z

The CI failures are relevant.

RaisinTen · 2022-01-18T14:10:50Z

@t8m it's just the callback variation that fails which was happening even before this change was added. I'm suspecting that's happening due to an incorrect invocation of the API? (suggestions would be great!) I've commented out that test for now with a TODO. If CI passes now (not sure why these tests aren't run for me locally, will investigate later) and y'all are good with the change, I'll try to sign the CLA.

t8m · 2022-01-20T16:45:33Z

Please re-enable the last test case. The error can be fixed by changing https://github.com/openssl/openssl/blob/master/crypto/ui/ui_util.c#L117 to if (len < 0)

t8m · 2022-01-20T16:46:50Z

Also please sign and send the CLA.

bernd-edlinger · 2022-01-20T19:28:39Z

Please re-enable the last test case. The error can be fixed by changing https://github.com/openssl/openssl/blob/master/crypto/ui/ui_util.c#L117 to if (len < 0)

but please be careful, return values 0 and -1 have a special meaning,
and the user callback has no way to return 0 any more with this change.
this is the caller:

openssl/crypto/ui/ui_lib.c

Lines 516 to 530 in 5288303

    
           switch (ui->meth->ui_read_string(ui, 
        
                                            sk_UI_STRING_value(ui->strings, 
        
                                                               i))) { 
        
           case -1:           /* Interrupt/Cancel/something... */ 
        
               ui->flags &= ~UI_FLAG_REDOABLE; 
        
               ok = -2; 
        
               goto err; 
        
           case 0:            /* Errors */ 
        
               state = "reading strings"; 
        
               ok = -1; 
        
               goto err; 
        
           default:           /* Success */ 
        
               ok = 0; 
        
               break; 
        
           }

and this is the callee:

openssl/crypto/ui/ui_util.c

Lines 111 to 121 in 5288303

    
           int len = data->cb(result, 
        
                              maxsize > PEM_BUFSIZE ? PEM_BUFSIZE : maxsize, 
        
                              data->rwflag, UI_get0_user_data(ui)); 
        
           if (len >= 0) 
        
               result[len] = '\0'; 
        
           if (len <= 0) 
        
               return len; 
        
           if (UI_set_result_ex(ui, uis, result, len) >= 0) 
        
               return 1; 
        
           return 0;

t8m · 2022-01-21T10:54:43Z

@bernd-edlinger the callback is the pem callback which returns 0 if the password is zero length, not that the callback has failed. We should not return 0 from ui_read() if the pem callback did not fail but simply returned 0 length password.
The ui_read() will return 0 if UI_set_result_ex() fails, and that is correct.
So I am sure that the change I propose is correct.

openssl-machine · 2022-01-26T13:00:23Z

This pull request is ready to merge

When `PEM_write_bio_PKCS8PrivateKey()` was passed an empty passphrase string, `OPENSSL_memdup()` was incorrectly getting used for 0 bytes size allocation, which resulted in malloc failures. Fixes: #17506 Signed-off-by: Darshan Sen <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #17507)

Signed-off-by: Darshan Sen <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #17507)

When `PEM_write_bio_PKCS8PrivateKey()` was passed an empty passphrase string, `OPENSSL_memdup()` was incorrectly getting used for 0 bytes size allocation, which resulted in malloc failures. Fixes: #17506 Signed-off-by: Darshan Sen <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #17507) (cherry picked from commit 59ccb72)

Signed-off-by: Darshan Sen <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #17507) (cherry picked from commit 1d28ada)

t8m · 2022-01-26T16:25:08Z

Merged to master and 3.0 branches (resolving the CHANGES.md conflict manually). Thank you for your contribution.

RaisinTen · 2022-01-27T06:02:20Z

@t8m thanks! Should this PR be closed, now that my commits have landed on the relevant branches or is there anything else I can help out with?

When `PEM_write_bio_PKCS8PrivateKey()` was passed an empty passphrase string, `OPENSSL_memdup()` was incorrectly getting used for 0 bytes size allocation, which resulted in malloc failures. Fixes: openssl#17506 Signed-off-by: Darshan Sen <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#17507)

Signed-off-by: Darshan Sen <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#17507)

openssl/openssl@59ccb72: ``` commit 59ccb72cd5cec3b4e312853621e12a68dacdbc7e Author: Darshan Sen <[email protected]> Date: Fri Jan 14 16:22:41 2022 +0530 Fix invalid malloc failures in PEM_write_bio_PKCS8PrivateKey() When `PEM_write_bio_PKCS8PrivateKey()` was passed an empty passphrase string, `OPENSSL_memdup()` was incorrectly getting used for 0 bytes size allocation, which resulted in malloc failures. Fixes: openssl/openssl#17506 Signed-off-by: Darshan Sen <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl/openssl#17507) ``` openssl/openssl@1d28ada: ``` commit 1d28ada1c39997c10fe5392f4235bbd2bc44b40f Author: Darshan Sen <[email protected]> Date: Sat Jan 22 17:56:05 2022 +0530 Allow empty passphrase in PEM_write_bio_PKCS8PrivateKey_nid() Signed-off-by: Darshan Sen <[email protected]> Reviewed-by: Bernd Edlinger <[email protected]> Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl/openssl#17507) ``` Refs: openssl/openssl#17507 Fixes: nodejs#41428 Signed-off-by: Darshan Sen <[email protected]>

Refs: openssl/openssl#17507 Refs: nodejs#41428 Signed-off-by: Darshan Sen <[email protected]>

Refs: openssl/openssl#17507 Refs: #41428 Signed-off-by: Darshan Sen <[email protected]> PR-URL: #42319 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Refs: openssl/openssl#17507 Refs: nodejs#41428 Signed-off-by: Darshan Sen <[email protected]> PR-URL: nodejs#42319 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Refs: openssl/openssl#17507 Refs: #41428 Signed-off-by: Darshan Sen <[email protected]> PR-URL: #42319 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Refs: openssl/openssl#17507 Refs: nodejs#41428 Signed-off-by: Darshan Sen <[email protected]> PR-URL: nodejs#42319 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jan 14, 2022

RaisinTen mentioned this pull request Jan 14, 2022

PEM_write_bio_PKCS8PrivateKey() doesn't accept empty passphrase strings anymore #17506

Closed

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jan 14, 2022

mattcaswell reviewed Jan 14, 2022

View reviewed changes

crypto/o_str.c Outdated Show resolved Hide resolved

bernd-edlinger reviewed Jan 14, 2022

View reviewed changes

crypto/passphrase.c Outdated Show resolved Hide resolved

RaisinTen force-pushed the fix-invalid-malloc-failures branch from b7dcbd1 to 3df2ce3 Compare January 15, 2022 12:06

RaisinTen changed the title ~~Fix invalid malloc failures~~ Fix invalid malloc failures in PEM_write_bio_PKCS8PrivateKey() Jan 15, 2022

RaisinTen force-pushed the fix-invalid-malloc-failures branch 2 times, most recently from 167f7d1 to ee25717 Compare January 15, 2022 12:20

RaisinTen requested review from bernd-edlinger and mattcaswell January 15, 2022 12:24

RaisinTen force-pushed the fix-invalid-malloc-failures branch from ee25717 to 0ed5713 Compare January 15, 2022 14:35

t8m added branch: 3.0 Applies to openssl-3.0 branch branch: master Applies to master branch triaged: bug The issue/pr is/fixes a bug labels Jan 18, 2022

t8m previously approved these changes Jan 18, 2022

View reviewed changes

t8m added the approval: review pending This pull request needs review by a committer label Jan 18, 2022

t8m removed the approval: review pending This pull request needs review by a committer label Jan 18, 2022

RaisinTen force-pushed the fix-invalid-malloc-failures branch from 0ed5713 to 2a0f6be Compare January 18, 2022 14:04

RaisinTen force-pushed the fix-invalid-malloc-failures branch from 2a0f6be to 00a1dad Compare January 18, 2022 14:13

github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label Jan 18, 2022

bernd-edlinger added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 25, 2022

paulidale approved these changes Jan 26, 2022

View reviewed changes

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 26, 2022

t8m removed the branch: 3.0 Applies to openssl-3.0 branch label Jan 26, 2022

t8m added the branch: 3.0 Applies to openssl-3.0 branch label Jan 26, 2022

t8m closed this Jan 27, 2022

RaisinTen deleted the fix-invalid-malloc-failures branch January 27, 2022 17:31

RaisinTen mentioned this pull request Jan 28, 2022

crypto.generateKeyPair generates error ERR_OSSL_CRYPTO_MALLOC_FAILURE in node 17.2.0 nodejs/node#41428

Closed

RaisinTen mentioned this pull request Jan 29, 2022

Fix invalid malloc failures in PEM_write_bio_PKCS8PrivateKey*() quictls/openssl#75

Closed

2 tasks

RaisinTen mentioned this pull request Mar 13, 2022

test,crypto: add and update empty passphrase regression tests nodejs/node#42319

Merged

RaisinTen added a commit to RaisinTen/node that referenced this pull request Mar 17, 2022

test,crypto: add and update empty passphrase regression tests

4fba5aa

Refs: openssl/openssl#17507 Refs: nodejs#41428 Signed-off-by: Darshan Sen <[email protected]>

Uh oh!

Fix invalid malloc failures in PEM_write_bio_PKCS8PrivateKey*() #17507

Fix invalid malloc failures in PEM_write_bio_PKCS8PrivateKey*() #17507

Uh oh!

Conversation

RaisinTen commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

t8m commented Jan 14, 2022

Uh oh!

RaisinTen commented Jan 15, 2022

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

t8m commented Jan 18, 2022

Uh oh!

RaisinTen commented Jan 18, 2022

Uh oh!

t8m commented Jan 20, 2022

Uh oh!

t8m commented Jan 20, 2022

Uh oh!

bernd-edlinger commented Jan 20, 2022

Uh oh!

t8m commented Jan 21, 2022

Uh oh!

openssl-machine commented Jan 26, 2022

Uh oh!

t8m commented Jan 26, 2022

Uh oh!

RaisinTen commented Jan 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fix invalid malloc failures in `PEM_write_bio_PKCS8PrivateKey*()` #17507

Fix invalid malloc failures in `PEM_write_bio_PKCS8PrivateKey*()` #17507

RaisinTen commented Jan 14, 2022 •

edited

Loading