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

Fixes for TFO Tests in ASAN build #18979

Closed
wants to merge 2 commits into from
Closed

Fixes for TFO Tests in ASAN build #18979

wants to merge 2 commits into from

Conversation

juergenchrist
Copy link
Contributor

ASAN reports a heap overflow and a leak when running tfo tests.

The heap overflow might be spurious (not sure what the C standard says to that case), but we can simply fix that by using the correct member of a union instead of copying all bytes of said union including bytes that might not belong to the structure written into the union.

The leak is due to a missing freeaddrinfo call.

@juergenchrist
Fix asan finding in bio_addr
4c2b4b7 
Running test_tfo_cli under asan yields
==166214==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60700000d57c at pc 0x03ffa004ed86 bp 0x03ffe2977e80 sp 0x03ffe2977668
READ of size 112 at 0x60700000d57c thread T0
    #0 0x3ffa004ed85 in memcpy (/lib64/libasan.so.8+0x4ed85)
    openssl#1 0x3ff9f3615b7 in BIO_ADDR_dup crypto/bio/bio_addr.c:77
[...]
and fails the test.

Fix this by copying the right structure of the union.

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>
mattcaswell
mattcaswell requested changes Aug 11, 2022
View reviewed changes
test/bio_tfo_test.c Outdated Show resolved Hide resolved
@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 branch: master Merge to master branch labels Aug 11, 2022
@mattcaswell
Copy link
Member

What branches does this apply to?

@juergenchrist
Copy link
Contributor Author

What branches does this apply to?

Found it on master. Did not check other branches. But I do not think this needs to be backported.

@juergenchrist
Fix asan finding in bio_tfo_test
0f258b5 
Running bio_tfo_test under asan yields
==172342==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 380 byte(s) in 5 object(s) allocated from:
    #0 0x3ff89bba251 in malloc (/lib64/libasan.so.8+0xba251)
    openssl#1 0x3ff88cf9fd5 in gaih_inet.constprop.0 (/lib64/libc.so.6+0xf9fd5)
    openssl#2 0x3ff88cfaf6f in getaddrinfo (/lib64/libc.so.6+0xfaf6f)
    openssl#3 0x3ff89ba52a9 in __interceptor_getaddrinfo.part.0 (/lib64/libasan.so.8+0xa52a9)
    openssl#4 0x1004909 in test_fd_tfo test/bio_tfo_test.c:241
[...]

and fails the test.

Fix this by freeing the return addrinfo on exit.

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>
@mattcaswell mattcaswell removed the approval: otc review pending This pull request needs review by an OTC member label Aug 11, 2022
tmshort
tmshort approved these changes Aug 11, 2022
View reviewed changes
Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks!

@tmshort tmshort 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 11, 2022
@tmshort
Copy link
Contributor

tmshort commented Aug 11, 2022

What branches does this apply to?

Found it on master. Did not check other branches. But I do not think this needs to be backported.

TFO was not ported to 3.0

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@mattcaswell mattcaswell 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 12, 2022
@mattcaswell
Copy link
Member

Pushed. Thanks.

openssl-machine pushed a commit that referenced this pull request Aug 12, 2022
@juergenchrist @mattcaswell
Fix asan finding in bio_addr
278b0d8 
Running test_tfo_cli under asan yields
==166214==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60700000d57c at pc 0x03ffa004ed86 bp 0x03ffe2977e80 sp 0x03ffe2977668
READ of size 112 at 0x60700000d57c thread T0
    #0 0x3ffa004ed85 in memcpy (/lib64/libasan.so.8+0x4ed85)
    #1 0x3ff9f3615b7 in BIO_ADDR_dup crypto/bio/bio_addr.c:77
[...]
and fails the test.

Fix this by copying the right structure of the union.

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>

Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #18979)
openssl-machine pushed a commit that referenced this pull request Aug 12, 2022
@juergenchrist @mattcaswell
Fix asan finding in bio_tfo_test
d272ef5 
Running bio_tfo_test under asan yields
==172342==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 380 byte(s) in 5 object(s) allocated from:
    #0 0x3ff89bba251 in malloc (/lib64/libasan.so.8+0xba251)
    #1 0x3ff88cf9fd5 in gaih_inet.constprop.0 (/lib64/libc.so.6+0xf9fd5)
    #2 0x3ff88cfaf6f in getaddrinfo (/lib64/libc.so.6+0xfaf6f)
    #3 0x3ff89ba52a9 in __interceptor_getaddrinfo.part.0 (/lib64/libasan.so.8+0xa52a9)
    #4 0x1004909 in test_fd_tfo test/bio_tfo_test.c:241
[...]

and fails the test.

Fix this by freeing the return addrinfo on exit.

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>

Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #18979)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@juergenchrist @sftcd
Fix asan finding in bio_addr
c0d7479 
Running test_tfo_cli under asan yields
==166214==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60700000d57c at pc 0x03ffa004ed86 bp 0x03ffe2977e80 sp 0x03ffe2977668
READ of size 112 at 0x60700000d57c thread T0
    #0 0x3ffa004ed85 in memcpy (/lib64/libasan.so.8+0x4ed85)
    #1 0x3ff9f3615b7 in BIO_ADDR_dup crypto/bio/bio_addr.c:77
[...]
and fails the test.

Fix this by copying the right structure of the union.

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>

Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#18979)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@juergenchrist @sftcd
Fix asan finding in bio_tfo_test
8b9e666 
Running bio_tfo_test under asan yields
==172342==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 380 byte(s) in 5 object(s) allocated from:
    #0 0x3ff89bba251 in malloc (/lib64/libasan.so.8+0xba251)
    #1 0x3ff88cf9fd5 in gaih_inet.constprop.0 (/lib64/libc.so.6+0xf9fd5)
    #2 0x3ff88cfaf6f in getaddrinfo (/lib64/libc.so.6+0xfaf6f)
    #3 0x3ff89ba52a9 in __interceptor_getaddrinfo.part.0 (/lib64/libasan.so.8+0xa52a9)
    #4 0x1004909 in test_fd_tfo test/bio_tfo_test.c:241
[...]

and fails the test.

Fix this by freeing the return addrinfo on exit.

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>

Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#18979)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattcaswell mattcaswell mattcaswell approved these changes

@tmshort tmshort tmshort approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@juergenchrist @mattcaswell @tmshort @openssl-machine