Skip to content

Fix memory leak with TLS1.2 compression#19030

Closed
juergenchrist wants to merge 1 commit intoopenssl:masterfrom
juergenchrist:fix/tlscompressleak
Closed

Fix memory leak with TLS1.2 compression#19030
juergenchrist wants to merge 1 commit intoopenssl:masterfrom
juergenchrist:fix/tlscompressleak

Conversation

@juergenchrist
Copy link
Contributor

Leak sanitizer reports following leak for ssl-test-new subtest
4-tlsv1_2-both-compress:

==335733==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
#0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
#12 0x100e85b in do_handshake_internal test/helpers/handshake.c:1544
#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
#14 0x101d1a7 in test_handshake test/ssl_test.c:543
#15 0x1027875 in run_tests test/testutil/driver.c:370
#16 0x1008393 in main test/testutil/main.c:30
#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
#19 0x100864f (/code/openssl/test/ssl_test+0x100864f)
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
#0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
#12 0x1010b09 in do_handshake_internal test/helpers/handshake.c:1550
#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
#14 0x101d1a7 in test_handshake test/ssl_test.c:543
#15 0x1027875 in run_tests test/testutil/driver.c:370
#16 0x1008393 in main test/testutil/main.c:30
#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
#19 0x100864f (/code/openssl/test/ssl_test+0x100864f)
SUMMARY: AddressSanitizer: 35456 byte(s) leaked in 2 allocation(s).

Fix this by freeing the SSL3_RECORD structure inside the OSSL_RECORD_LAYER.

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

@t8m t8m added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: bug The issue/pr is/fixes a bug labels Aug 19, 2022
@juergenchrist
Copy link
Contributor Author

Any chance to get this fix reviewed? The test_ssl-new are failing for 10 days already. If not, I can only either deactivate these tests or asan.

if (rl->version == SSL3_VERSION)
OPENSSL_cleanse(rl->mac_secret, sizeof(rl->mac_secret));

SSL3_RECORD_release(rl->rrec, rl->num_recs);
Copy link
Member

Choose a reason for hiding this comment

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

This should be SSL_MAX_PIPELINES not rl->num_recs. The number of active records at any one time may vary, so the current value for rl->num_recs may be less than the maximum value it has ever been. By using SSL_MAX_PIPELINES instead we release all records regardless of whether they are currently in use or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Leak sanitizer reports following leak for ssl-test-new subtest
4-tlsv1_2-both-compress:

==335733==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
    #0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
    openssl#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
    openssl#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
    openssl#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
    openssl#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
    openssl#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
    openssl#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
    openssl#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
    openssl#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
    openssl#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
    openssl#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
    openssl#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
    openssl#12 0x100e85b in do_handshake_internal test/helpers/handshake.c:1544
    openssl#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
    openssl#14 0x101d1a7 in test_handshake test/ssl_test.c:543
    openssl#15 0x1027875 in run_tests test/testutil/driver.c:370
    openssl#16 0x1008393 in main test/testutil/main.c:30
    openssl#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
    openssl#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
    openssl#19 0x100864f  (/code/openssl/test/ssl_test+0x100864f)
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
    #0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
    openssl#1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
    openssl#2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
    openssl#3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
    openssl#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
    openssl#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
    openssl#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
    openssl#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
    openssl#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
    openssl#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
    openssl#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
    openssl#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
    openssl#12 0x1010b09 in do_handshake_internal test/helpers/handshake.c:1550
    openssl#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
    openssl#14 0x101d1a7 in test_handshake test/ssl_test.c:543
    openssl#15 0x1027875 in run_tests test/testutil/driver.c:370
    openssl#16 0x1008393 in main test/testutil/main.c:30
    openssl#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
    openssl#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
    openssl#19 0x100864f  (/code/openssl/test/ssl_test+0x100864f)
SUMMARY: AddressSanitizer: 35456 byte(s) leaked in 2 allocation(s).

Fix this by freeing the SSL3_RECORD structure inside the OSSL_RECORD_LAYER.

Signed-off-by: Juergen Christ <jchrist@linux.ibm.com>
@mattcaswell mattcaswell 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 30, 2022
@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 31, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Aug 31, 2022

Merged to master branch. Thank you for your contribution.

@t8m t8m closed this Aug 31, 2022
openssl-machine pushed a commit that referenced this pull request Aug 31, 2022
Leak sanitizer reports following leak for ssl-test-new subtest
4-tlsv1_2-both-compress:

==335733==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
    #0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
    #1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
    #2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
    #3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
    #4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
    #5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
    #6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
    #7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
    #8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
    #9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
    #10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
    #11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
    #12 0x100e85b in do_handshake_internal test/helpers/handshake.c:1544
    #13 0x1011715 in do_handshake test/helpers/handshake.c:1738
    #14 0x101d1a7 in test_handshake test/ssl_test.c:543
    #15 0x1027875 in run_tests test/testutil/driver.c:370
    #16 0x1008393 in main test/testutil/main.c:30
    #17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
    #18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
    #19 0x100864f  (/code/openssl/test/ssl_test+0x100864f)
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
    #0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
    #1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
    #2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
    #3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
    #4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
    #5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
    #6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
    #7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
    #8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
    #9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
    #10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
    #11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
    #12 0x1010b09 in do_handshake_internal test/helpers/handshake.c:1550
    #13 0x1011715 in do_handshake test/helpers/handshake.c:1738
    #14 0x101d1a7 in test_handshake test/ssl_test.c:543
    #15 0x1027875 in run_tests test/testutil/driver.c:370
    #16 0x1008393 in main test/testutil/main.c:30
    #17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
    #18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
    #19 0x100864f  (/code/openssl/test/ssl_test+0x100864f)
SUMMARY: AddressSanitizer: 35456 byte(s) leaked in 2 allocation(s).

Fix this by freeing the SSL3_RECORD structure inside the OSSL_RECORD_LAYER.

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

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19030)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
Leak sanitizer reports following leak for ssl-test-new subtest
4-tlsv1_2-both-compress:

==335733==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
    #0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
    #1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
    #2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
    #3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
    #4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
    #5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
    #6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
    #7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
    #8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
    #9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
    #10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
    #11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
    #12 0x100e85b in do_handshake_internal test/helpers/handshake.c:1544
    #13 0x1011715 in do_handshake test/helpers/handshake.c:1738
    #14 0x101d1a7 in test_handshake test/ssl_test.c:543
    #15 0x1027875 in run_tests test/testutil/driver.c:370
    #16 0x1008393 in main test/testutil/main.c:30
    #17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
    #18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
    #19 0x100864f  (/code/openssl/test/ssl_test+0x100864f)
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
    #0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
    #1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
    #2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
    #3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
    #4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
    #5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
    #6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
    #7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
    #8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
    #9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
    #10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
    #11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
    #12 0x1010b09 in do_handshake_internal test/helpers/handshake.c:1550
    #13 0x1011715 in do_handshake test/helpers/handshake.c:1738
    #14 0x101d1a7 in test_handshake test/ssl_test.c:543
    #15 0x1027875 in run_tests test/testutil/driver.c:370
    #16 0x1008393 in main test/testutil/main.c:30
    #17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
    #18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
    #19 0x100864f  (/code/openssl/test/ssl_test+0x100864f)
SUMMARY: AddressSanitizer: 35456 byte(s) leaked in 2 allocation(s).

Fix this by freeing the SSL3_RECORD structure inside the OSSL_RECORD_LAYER.

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

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19030)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Leak sanitizer reports following leak for ssl-test-new subtest
4-tlsv1_2-both-compress:

==335733==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
    #0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
    #1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
    #2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
    #3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
    openssl#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
    openssl#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
    openssl#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
    openssl#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
    openssl#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
    openssl#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
    openssl#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
    openssl#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
    openssl#12 0x100e85b in do_handshake_internal test/helpers/handshake.c:1544
    openssl#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
    openssl#14 0x101d1a7 in test_handshake test/ssl_test.c:543
    openssl#15 0x1027875 in run_tests test/testutil/driver.c:370
    openssl#16 0x1008393 in main test/testutil/main.c:30
    openssl#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
    openssl#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
    openssl#19 0x100864f  (/code/openssl/test/ssl_test+0x100864f)
Direct leak of 17728 byte(s) in 1 object(s) allocated from:
    #0 0x3ff9fbba251 in malloc (/usr/lib64/libasan.so.8+0xba251)
    #1 0x3ff9f71744f in tls_do_uncompress ssl/record/methods/tls_common.c:868
    #2 0x3ff9f7175bd in tls_default_post_process_record ssl/record/methods/tls_common.c:896
    #3 0x3ff9f715ee7 in tls_get_more_records ssl/record/methods/tls_common.c:773
    openssl#4 0x3ff9f712209 in tls_read_record ssl/record/methods/tls_common.c:958
    openssl#5 0x3ff9f6ef73f in ssl3_read_bytes ssl/record/rec_layer_s3.c:1235
    openssl#6 0x3ff9f776165 in tls_get_message_header ssl/statem/statem_lib.c:1198
    openssl#7 0x3ff9f74709b in read_state_machine ssl/statem/statem.c:624
    openssl#8 0x3ff9f74709b in state_machine ssl/statem/statem.c:478
    openssl#9 0x3ff9f662e61 in SSL_do_handshake ssl/ssl_lib.c:4430
    openssl#10 0x100c55d in do_handshake_step test/helpers/handshake.c:775
    openssl#11 0x100c55d in do_connect_step test/helpers/handshake.c:1134
    openssl#12 0x1010b09 in do_handshake_internal test/helpers/handshake.c:1550
    openssl#13 0x1011715 in do_handshake test/helpers/handshake.c:1738
    openssl#14 0x101d1a7 in test_handshake test/ssl_test.c:543
    openssl#15 0x1027875 in run_tests test/testutil/driver.c:370
    openssl#16 0x1008393 in main test/testutil/main.c:30
    openssl#17 0x3ff9cc2b871 in __libc_start_call_main (/usr/lib64/libc.so.6+0x2b871)
    openssl#18 0x3ff9cc2b94f in __libc_start_main_alias_2 (/usr/lib64/libc.so.6+0x2b94f)
    openssl#19 0x100864f  (/code/openssl/test/ssl_test+0x100864f)
SUMMARY: AddressSanitizer: 35456 byte(s) leaked in 2 allocation(s).

Fix this by freeing the SSL3_RECORD structure inside the OSSL_RECORD_LAYER.

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

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19030)
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: master Applies to master branch triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants