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.

Sign up for GitHub

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

Jump to bottom

Only free the read buffers if we're not using them (3.1/3.0) #24395

Closed
wants to merge 7 commits into from
Closed

Only free the read buffers if we're not using them (3.1/3.0) #24395

wants to merge 7 commits into from
+229 −21

Conversation

mattcaswell
Copy link
Member

If we're part way through processing a record, or the application has
not released all the records then we should not free our buffer because
they are still needed.

CVE-2024-4741

wbl and others added 5 commits May 14, 2024 15:20
@wbl @mattcaswell
Only free the read buffers if we're not using them
8011033 
If we're part way through processing a record, or the application has
not released all the records then we should not free our buffer because
they are still needed.

CVE-2024-4741
@mattcaswell
Set rlayer.packet to NULL after we've finished using it
1c4f067 
In order to ensure we do not have a UAF we reset the rlayer.packet pointer
to NULL after we free it.

CVE-2024-4741
@mattcaswell
Extend the SSL_free_buffers testing
b0ee93c 
Test that attempting to free the buffers at points where they should not
be freed works as expected.

Follow on from CVE-2024-4741
@mattcaswell
Move the ability to load the dasync engine into ssltestlib.c
9656aaa 
The sslapitest has a helper function to load the dasync engine which is
useful for testing pipelining. We would like to have the same facility
from sslbuffertest, so we move the function to the common location
ssltestlib.c

Follow on from CVE-2024-4741
@mattcaswell
Further extend the SSL_free_buffers testing
1153eda 
We extend the testing to test what happens when pipelining is in use.

Follow on from CVE-2024-4741
@mattcaswell mattcaswell added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member severity: urgent Fixes an urgent issue (exempt from 24h grace period) branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: present The PR has suitable tests present labels May 14, 2024
@mattcaswell mattcaswell changed the title Only free the read buffers if we're not using them Only free the read buffers if we're not using them (3.1/3.0) May 14, 2024
@mattcaswell
Copy link
Member Author

Backport to 3.1/3.0 of #24394

@mattcaswell mattcaswell mentioned this pull request May 14, 2024
Closed
@t8m t8m added triaged: bug The issue/pr is/fixes a bug severity: important Important bugs affecting a released version and removed severity: urgent Fixes an urgent issue (exempt from 24h grace period) labels May 14, 2024
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label May 14, 2024
@t8m t8m requested a review from nhorman May 16, 2024 16:17
@mattcaswell mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: review pending This pull request needs review by a committer labels May 27, 2024
@mattcaswell
Copy link
Member Author

Pushed to 3.1/3.0. Thanks.

openssl-machine pushed a commit that referenced this pull request May 28, 2024
@wbl @mattcaswell
Only free the read buffers if we're not using them
b3f0eb0 
If we're part way through processing a record, or the application has
not released all the records then we should not free our buffer because
they are still needed.

CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #24395)

(cherry picked from commit 704f725)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Only free the read buffers if we're not using them
38690ca 
If we're part way through processing a record, or the application has
not released all the records then we should not free our buffer because
they are still needed.

CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Set rl->packet to NULL after we've finished using it
bfb8128 
In order to ensure we do not have a UAF we reset the rl->packet pointer
to NULL after we free it.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Extend the SSL_free_buffers testing
566f306 
Test that attempting to free the buffers at points where they should not
be freed works as expected.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Move the ability to load the dasync engine into ssltestlib.c
0575247 
The sslapitest has a helper function to load the dasync engine which is
useful for testing pipelining. We would like to have the same facility
from sslbuffertest, so we move the function to the common location
ssltestlib.c

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Further extend the SSL_free_buffers testing
c1bd38a 
We extend the testing to test what happens when pipelining is in use.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Only free the read buffers if we're not using them
e509313 
If we're part way through processing a record, or the application has
not released all the records then we should not free our buffer because
they are still needed.

CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)

(cherry picked from commit 38690ca)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Set rl->packet to NULL after we've finished using it
d9dd9af 
In order to ensure we do not have a UAF we reset the rl->packet pointer
to NULL after we free it.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)

(cherry picked from commit bfb8128)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Extend the SSL_free_buffers testing
84e0230 
Test that attempting to free the buffers at points where they should not
be freed works as expected.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)

(cherry picked from commit 566f306)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Move the ability to load the dasync engine into ssltestlib.c
359b18d 
The sslapitest has a helper function to load the dasync engine which is
useful for testing pipelining. We would like to have the same facility
from sslbuffertest, so we move the function to the common location
ssltestlib.c

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)

(cherry picked from commit 0575247)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Further extend the SSL_free_buffers testing
9c5ccb6 
We extend the testing to test what happens when pipelining is in use.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)

(cherry picked from commit c1bd38a)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@wbl @mattcaswell
Only free the read buffers if we're not using them
704f725 
If we're part way through processing a record, or the application has
not released all the records then we should not free our buffer because
they are still needed.

CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #24395)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Set rlayer.packet to NULL after we've finished using it
d146349 
In order to ensure we do not have a UAF we reset the rlayer.packet pointer
to NULL after we free it.

CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Extend the SSL_free_buffers testing
4238abc 
Test that attempting to free the buffers at points where they should not
be freed works as expected.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Move the ability to load the dasync engine into ssltestlib.c
0544c21 
The sslapitest has a helper function to load the dasync engine which is
useful for testing pipelining. We would like to have the same facility
from sslbuffertest, so we move the function to the common location
ssltestlib.c

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)
openssl-machine pushed a commit that referenced this pull request May 28, 2024
@mattcaswell
Further extend the SSL_free_buffers testing
6972d5a 
We extend the testing to test what happens when pipelining is in use.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from #24395)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jun 2, 2024
@wbl @bernd-edlinger
Only free the read buffers if we're not using them
921ccd9 
If we're part way through processing a record, or the application has
not released all the records then we should not free our buffer because
they are still needed.

CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#24395)

(cherry picked from commit 704f725)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jun 2, 2024
@mattcaswell @bernd-edlinger
Extend the SSL_free_buffers testing
149b258 
Test that attempting to free the buffers at points where they should not
be freed works as expected.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)

(cherry picked from commit 4238abc)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jun 2, 2024
@mattcaswell @bernd-edlinger
Extend the SSL_free_buffers testing
3bdc94d 
Test that attempting to free the buffers at points where they should not
be freed works as expected.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)

(cherry picked from commit 4238abc)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jun 2, 2024
@mattcaswell @bernd-edlinger
Extend the SSL_free_buffers testing
923a91e 
Test that attempting to free the buffers at points where they should not
be freed works as expected.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)

(cherry picked from commit 4238abc)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jun 2, 2024
@mattcaswell @bernd-edlinger
Extend the SSL_free_buffers testing
b93d46d 
Test that attempting to free the buffers at points where they should not
be freed works as expected.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)

(cherry picked from commit 4238abc)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jun 2, 2024
@mattcaswell @bernd-edlinger
Further extend the SSL_free_buffers testing
626a9d7 
We extend the testing to test what happens when pipelining is in use.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)

(cherry picked from commit 6972d5a)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jun 2, 2024
@mattcaswell @bernd-edlinger
Further extend the SSL_free_buffers testing
0397249 
We extend the testing to test what happens when pipelining is in use.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)

(cherry picked from commit 6972d5a)
jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024
@mattcaswell @jvdsn
Only free the read buffers if we're not using them
c9e8e92 
If we're part way through processing a record, or the application has
not released all the records then we should not free our buffer because
they are still needed.

CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)
jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024
@mattcaswell @jvdsn
Set rl->packet to NULL after we've finished using it
c43b65f 
In order to ensure we do not have a UAF we reset the rl->packet pointer
to NULL after we free it.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)
jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024
@mattcaswell @jvdsn
Extend the SSL_free_buffers testing
b1ef0c6 
Test that attempting to free the buffers at points where they should not
be freed works as expected.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)
jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024
@mattcaswell @jvdsn
Move the ability to load the dasync engine into ssltestlib.c
fb03e7a 
The sslapitest has a helper function to load the dasync engine which is
useful for testing pipelining. We would like to have the same facility
from sslbuffertest, so we move the function to the common location
ssltestlib.c

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)
jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024
@mattcaswell @jvdsn
Further extend the SSL_free_buffers testing
48a4d4c 
We extend the testing to test what happens when pipelining is in use.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jun 4, 2024
@wbl @bernd-edlinger
Only free the read buffers if we're not using them
3b1a204 
If we're part way through processing a record, or the application has
not released all the records then we should not free our buffer because
they are still needed.

CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#24395)

(cherry picked from commit 704f725)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jun 4, 2024
@mattcaswell @bernd-edlinger
Extend the SSL_free_buffers testing
3131ce4 
Test that attempting to free the buffers at points where they should not
be freed works as expected.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)

(cherry picked from commit 4238abc)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Jun 4, 2024
@mattcaswell @bernd-edlinger
Further extend the SSL_free_buffers testing
1db4939 
We extend the testing to test what happens when pipelining is in use.

Follow on from CVE-2024-4741

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from openssl#24395)

(cherry picked from commit 6972d5a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nhorman nhorman nhorman approved these changes

@t8m t8m t8m approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 severity: important Important bugs affecting a released version tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mattcaswell @nhorman @t8m @wbl