Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix null pointer dereference when memory allocation fails #21606

Conversation

yangyangtiantianlonglong
Copy link
Contributor

@yangyangtiantianlonglong yangyangtiantianlonglong commented Jul 31, 2023

A null pointer dereference occurs when memory allocation fails
Fixes #21605

@t8m t8m added approval: review pending This pull request needs review by a committer branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug tests: exempted The PR is exempt from requirements for testing labels Jul 31, 2023
t8m
t8m previously approved these changes Jul 31, 2023
@t8m t8m self-requested a review July 31, 2023 20:01
ssl/ssl_sess.c Outdated Show resolved Hide resolved
@paulidale paulidale removed the approval: otc review pending This pull request needs review by an OTC member label Aug 1, 2023
ssl/ssl_sess.c Outdated Show resolved Hide resolved
@tom-cosgrove-arm tom-cosgrove-arm changed the title A null pointer dereference occurs when memory allocation fails Fix null pointer dereference when memory allocation fails Aug 1, 2023
@mspncp
Copy link
Contributor

mspncp commented Aug 1, 2023

Side note: looking at the source code for 3.0

openssl/ssl/ssl_sess.c

Lines 820 to 826 in bbc1aa9

void SSL_SESSION_free(SSL_SESSION *ss)
{
int i;
if (ss == NULL)
return;
CRYPTO_DOWN_REF(&ss->references, &i, ss->lock);

and 3.1,

openssl/ssl/ssl_sess.c

Lines 822 to 828 in 162b5a3

void SSL_SESSION_free(SSL_SESSION *ss)
{
int i;
if (ss == NULL)
return;
CRYPTO_DOWN_REF(&ss->references, &i, ss->lock);

it seems like the fix needs to be applied to the 3.0 and 3.1 branches as well (and it cherry-picks cleanly). However, looking at @paulidale's commit 99fd5b2 it turns out not to be an issue because the lock parameter is ignored in CRYPTO_DOWN_REF().

@yangyangtiantianlonglong
Copy link
Contributor Author

Side note: looking at the source code for 3.0

openssl/ssl/ssl_sess.c

Lines 820 to 826 in bbc1aa9

void SSL_SESSION_free(SSL_SESSION *ss)
{
int i;
if (ss == NULL)
return;
CRYPTO_DOWN_REF(&ss->references, &i, ss->lock);

and 3.1,

openssl/ssl/ssl_sess.c

Lines 822 to 828 in 162b5a3

void SSL_SESSION_free(SSL_SESSION *ss)
{
int i;
if (ss == NULL)
return;
CRYPTO_DOWN_REF(&ss->references, &i, ss->lock);

it seems like the fix needs to be applied to the 3.0 and 3.1 branches as well (and it cherry-picks cleanly). However, looking at @paulidale's commit 99fd5b2 it turns out not to be an issue because the lock parameter is ignored in CRYPTO_DOWN_REF().

yes,I only encountered this issue with GCC 4.1.1 and openssl-1.1.1.

@hlandau hlandau 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 Aug 1, 2023
@t8m t8m added branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Aug 1, 2023
@openssl-machine 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 Aug 2, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Aug 2, 2023
Fixes #21605

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21606)
@hlandau hlandau removed the branch: master Merge to master branch label Aug 2, 2023
@hlandau
Copy link
Member

hlandau commented Aug 2, 2023

Merged to 1.1.1, 3.1 and 3.0. Does not appear to be applicable to master.

@hlandau hlandau closed this Aug 2, 2023
openssl-machine pushed a commit that referenced this pull request Aug 2, 2023
Fixes #21605

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21606)

(cherry picked from commit a8da305)
openssl-machine pushed a commit that referenced this pull request Aug 2, 2023
Fixes #21605

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21606)

(cherry picked from commit a8da305)
xl32 pushed a commit to xl32/openssl that referenced this pull request Sep 29, 2023
Fixes openssl#21605

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#21606)

(cherry picked from commit a8da305)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants