Repository navigation
fix dangling local raw_chain in GnuTLS ssl_update_certs_info #5315
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| -----BEGIN CERTIFICATE----- | ||
| MIIEPjCCAiagAwIBAgIBAjANBgkqhkiG9w0BAQsFADAUMRIwEAYDVQQDDAlwanNp | ||
| cC5sYWIwHhcNMjYxMDA4MTAwNTEzWhcNMzAxMjA2MTAwNTEzWjAbMRkwFwYDVQQD | ||
| DBBjbGllbnQucGpzaXAubGFiMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKC | ||
| AQEAp/pwuY8gnNAkWTTTpEazOnmSM5ywqfCFW4h0sWlbyzpWVkPnyrYcn+SanQZk | ||
| wZYolpazWx9+q5GA7ilBkCogcG44Qu5HWytW25iKMLgcPf/JhgGpvwW1G1UQseNK | ||
| l3PtVA68jyvpTSFiIi1FtgC5Q9gkxq7IzfdvXd/1yiTdfKd1w3MOjF57RJchIZL+ | ||
| x3mK2itYQ3RyZ80LN5DT4cmK3hS5j0rktJHJuRKZt9LKvef6O1upM9iEnSSaJA0i | ||
| 30um8OO3mc5HCvX3VNYxw0dPUBHb9gQm4574N7/t0OtrIEMQkHpO+5BHHnB5dx2A | ||
| kX73kOxoudiRGsAfH2btpaDToQIDAQABo4GTMIGQMAwGA1UdEwEB/wQCMAAwDgYD | ||
| VR0PAQH/BAQDAgWgMBMGA1UdJQQMMAoGCCsGAQUFBwMCMBsGA1UdEQQUMBKCEGNs | ||
| aWVudC5wanNpcC5sYWIwHQYDVR0OBBYEFGmTl3GUTyJdbS//27kRPoc9LmqgMB8G | ||
| A1UdIwQYMBaAFF6rnJ5vs8hTda7vbDQYWmekykW3MA0GCSqGSIb3DQEBCwUAA4IC | ||
| AQAgLvWuAIPpiinnZI2ROBcNd/u1S2W1kQ2qqhbwCpo/w5gaBjQyC+lhqJcPIrop | ||
| /8VLc3F2aa0PwLKEyx5PHSFXfjbZxI2RqPKE5+9nh5ROO7EkLZKwJSmQb9jQ6EmF | ||
| dFYMTbX42T+mvDOPLh/QembVfnyUDgzrteCTwO9pK6y4KHUh8DZdHYOOobrgJ2Ly | ||
| 3y6G0kU4VDKtOOvAA2Lpjn/Dg0PtjMbRpYUdPGVs0Y93p3IgrRTSKD3NAECxtEZN | ||
| rnvTOjmY32J1C9jLA1hJa9nvNbON7sQglofJbhgygC4RU1ZsacArkG91dT7wTbOT | ||
| P8yGkuVfJxTtgvuUuI8hz8Y/6fF1MV3ayxNV/X6YSJdgukljQtzYtqsy26kFBLVq | ||
| m4EWdjtpTeW4YPYWYxhRhr4m8njBG68sfJTaOssoF0k67rehPBOGzLtF95qcFNls | ||
| xxQNrqbVWRdr+8a7QY3GT7tX0lfIk3Epo0VsjrF5OEM8/1dV2pSFGWbCVvMaKUPx | ||
| +zqzQKNCaUwqrQZCuMSGDOAmOb2VWhZZOwmKt+J0cMi5qI2uW5XpaVUBKM9bzPjs | ||
| Liqn1Q7t5XYZkC7j2JkHbn4k2b+s99Qz1AtGxnGnvarXyRbIE1bawC8br4jtucMs | ||
| WR0z5tKFtHBKCkiPPadxNiD7hrvlc8c0y8geRCZ807lAmQ== | ||
| -----END CERTIFICATE----- |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| -----BEGIN ENCRYPTED PRIVATE KEY----- | ||
| MIIFNTBfBgkqhkiG9w0BBQ0wUjAxBgkqhkiG9w0BBQwwJAQQ9rtm/f1rrMTBckut | ||
| zVwvhAICCAAwDAYIKoZIhvcNAgkFADAdBglghkgBZQMEASoEEM+p44rDxIWJMaBG | ||
| UqePmlgEggTQQblz8oxV7H1FBxNQVPA8sn6Yae7/gV52JlvMmtLPFmMx9p5fXtkW | ||
| B4mQzHM5b2Va262XKJZJHd/WnfleKfNH3kfzALyTov+JyOiMmxu3PlNaDbnZoNNH | ||
| b6CCIzjxuzDqRfOsGtTZp1zYJu/EhDe2o2ChqBoDAy9Kp9FYr5zX+IQmqV0CKl7H | ||
| x5a0IfK6IUzY+z3pKHbrjRiA0eGHtA72qypYZbZ+JEdTk+IDcjXK62AcSMXf3BKm | ||
| 8uXRhVYVgewmm7mnb51qhyfCSOg/h/f9kyBqHZ3L30WfBjZfhFFDXgOK4GpmWxhz | ||
| n2d03AWTrt1XMLMfeY1M/Fm7Os1oAR5uoq7xuO/7L14rkO9e+3SOtTD4mBuqd7gh | ||
| VHhitLc3odpCFY2Wm+hBHESkyOOzGFd9LLO5hPx4kSNXduT2xfEjSCF6tnbIMqGO | ||
| 3Use2NtxZYjQ1axvDzaj1ANR/o/HiT9SsXSLbtZ3o9uJBLG+uhB88d/6+AnHzTdC | ||
| OYVykXxQ+xhagKlC5QCgyP+2MB66rLHV2aq0U/oHLv8zcCQ2Ywmy6tedCgQ+qy/p | ||
| IJFFTeBHtRN+8zfrVY5iXrZC0zrrV8paxjAE5ldUyQtl2CLvNBpibUshK16GsUJX | ||
| MNJCloQNdafECZwLZLh1vfT0cRX+74lJiaXzenc+vudXYoHJIqJfDxp4fpm+eVuj | ||
| dUBLKHuv9IWIdEcyO1pvBJwtPZG6hwMp42Rn1cEngpnr37Q2mSE5N+FWptwmA7t3 | ||
| 6tbtoglCv5Il2NFINv4l1YtIJPiiFw9Za2mF9ArX7xm0PvE6WA7iKirqQnoUlO6L | ||
| MQCuUZvNFn2CIefuj76MrSLvTIORYbOBrE2Rpys7ZlWbrDtPNsmW9WRcGYa3GBGp | ||
| /7TqYS6Ob1h9egiiht+OEKtUjLSTG6Ta1OH2e4fNOa920qpBUVQvVBZZ54Kug/WV | ||
| uriQtZ4QK4TBBC4GzceoT++Umc/SG3lzkYQO5q8Ertk0BbJ7LHPN/DDjB8xDJ70V | ||
| 6VRUYoN4gSqhXAvkOqsV8SwcwDI3vKm0EusemjZBkTykl/eJD95sfbTNDwIJWN3z | ||
| 3ib2djhXZ/++kvNOz3U/kAyb5uVjIPLEZtms3l5YCWtm+alaD1NxiEga7z0zD/+M | ||
| lGux/rlw6eO/oboZXlWNhpleZIlXSFERZxtuc55xY/rzvpWBpTc3Nw8OJeJXyXTx | ||
| yVWX0xKepKr0DVwDbljKUpFt+x8/noU9CHvxq6Z8L7WZsitw9cvCvEnvxu8PEkI/ | ||
| isD5TtdSJWwiTrAAWYo/pJHmsXTWb/1K18VPaJ/53p5aHABgTUdxygmXmnLr1EHA | ||
| 0l0SVjKqbB7d7kcyN/dQ+0bUcsi9mvBTHodzhQBRwL+I5jo+cfx2scSYFq8O0sYL | ||
| eQqdD4WRQ6Fcl0BOxlPil7wVjkM0At9hMWsCyhsEx2P6AmtzM6NBg3lpabOm0DU4 | ||
| m3uLmNRWA1cRCeSWPEWDvlglWopCv04DMY8GQkt/G4TXhTk/C8ogouNbYRFbAJAB | ||
| rDwxSOTRNGTInH3fU1mTV1LfyY3TN6WGQWXS40dSiXm4kcVLHsad9wY+1b0k62pg | ||
| tzKAKMUXzWhJ0i9+w3G3xvf7GBtLkWJ2clHfdspTYAWsKilcoBsfx5c= | ||
| -----END ENCRYPTED PRIVATE KEY----- |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright (C) 2008-2011 Teluu Inc. (http://www.teluu.com) | ||
| * Copyright (C) 2008-2026 Teluu Inc. (http://www.teluu.com) | ||
| * Copyright (C) 2003-2008 Benny Prijono <benny@prijono.org> | ||
| * | ||
| * This program is free software; you can redistribute it and/or modify | ||
|
|
@@ -66,7 +66,9 @@ struct test_state | |
| pj_pool_t *pool; /* pool */ | ||
| pj_ioqueue_t *ioqueue; /* ioqueue */ | ||
| pj_ssl_sock_t *accepted_ssock; /* accepted server-side socket */ | ||
| struct test_state *accepted_st; /* state of the accepted socket */ | ||
| pj_bool_t is_server; /* server role flag */ | ||
| const char *cert_der; /* DER of this end's own certificate */ | ||
| pj_bool_t is_verbose; /* verbose flag, e.g: cert info */ | ||
| pj_bool_t echo; /* echo received data */ | ||
| pj_status_t err; /* error flag */ | ||
|
|
@@ -117,6 +119,61 @@ static void dump_ssl_info(const pj_ssl_sock_info *si) | |
| } | ||
| } | ||
|
|
||
| /* The local raw chain must hold the local certificate, in storage of its | ||
| * own rather than the peer chain's. der_name is the DER of the certificate | ||
| * this end presents, or NULL to compare the storage only. Backends that | ||
| * don't report the local raw chain, e.g. OpenSSL, leave cnt zero and are | ||
| * skipped. | ||
| */ | ||
| static pj_status_t check_raw_chain(const pj_ssl_sock_info *si, | ||
| pj_pool_t *pool, | ||
| const char *der_name) | ||
| { | ||
| const pj_ssl_cert_info *lci = si->local_cert_info; | ||
| const pj_ssl_cert_info *rci = si->remote_cert_info; | ||
| pj_str_t der_file; | ||
| pj_oshandle_t fd = NULL; | ||
| pj_ssize_t size; | ||
| pj_str_t der; | ||
| pj_status_t status; | ||
|
|
||
| if (!lci || lci->raw_chain.cnt == 0) | ||
| return PJ_SUCCESS; | ||
|
|
||
| if (rci && rci->raw_chain.cnt && | ||
| lci->raw_chain.cert_raw == rci->raw_chain.cert_raw) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check only fails if the allocator returns the exact same address for both chains. It never checks that the local chain actually holds the local certificate. With Could it compare the content instead, e.g. check that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added, it now compares Worth flagging though: in So both checks are in. The content one catches a chain overwritten with something else, and it would catch the dangle on any setup where the two ends present different certificates. Your allocator point still stands for the overlap half, and making the test independent of the pool handing back the same address needs a second certificate for the client. Say the word and I'll add one.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, please add a second certificate for the client. As you found, with the same cert on both ends the content check can't tell a dangling local chain from a correct one, and the overlap check depends on the pool returning the same address. With different certs on each side, the content check catches the bug no matter how the allocator behaves.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added. Before: both ends presented One backend keeps the old arrangement. The Apple backends take the private key from the Keychain and only the server key is in |
||
| { | ||
| return PJ_EBUG; | ||
| } | ||
|
|
||
| if (!der_name) | ||
| return PJ_SUCCESS; | ||
|
|
||
| der_file = pj_str((char*)der_name); | ||
| size = (pj_ssize_t)pj_file_size(der_file.ptr); | ||
| if (size <= 0) | ||
| return PJ_ENOTFOUND; | ||
|
|
||
| status = pj_file_open(pool, der_file.ptr, PJ_O_RDONLY, &fd); | ||
| if (status != PJ_SUCCESS) | ||
| return status; | ||
|
|
||
| der.ptr = (char*)pj_pool_alloc(pool, size); | ||
| status = pj_file_read(fd, der.ptr, &size); | ||
| der.slen = size; | ||
| pj_file_close(fd); | ||
| if (status != PJ_SUCCESS) | ||
| return status; | ||
|
|
||
| if (lci->raw_chain.cert_raw[0].slen != der.slen || | ||
| pj_memcmp(lci->raw_chain.cert_raw[0].ptr, der.ptr, der.slen)) | ||
| { | ||
| return PJ_EBUG; | ||
| } | ||
|
|
||
| return PJ_SUCCESS; | ||
| } | ||
|
|
||
|
|
||
| static pj_bool_t ssl_on_connect_complete(pj_ssl_sock_t *ssock, | ||
| pj_status_t status) | ||
|
|
@@ -138,6 +195,12 @@ static pj_bool_t ssl_on_connect_complete(pj_ssl_sock_t *ssock, | |
| goto on_return; | ||
| } | ||
|
|
||
| status = check_raw_chain(&info, st->pool, st->cert_der); | ||
| if (status != PJ_SUCCESS) { | ||
| app_perror("...ERROR invalid local raw chain", status); | ||
| goto on_return; | ||
| } | ||
|
|
||
| pj_sockaddr_print((pj_sockaddr_t*)&info.local_addr, buf1, sizeof(buf1), 1); | ||
| pj_sockaddr_print((pj_sockaddr_t*)&info.remote_addr, buf2, sizeof(buf2), 1); | ||
| PJ_LOG(3, ("", "...Connected %s -> %s!", buf1, buf2)); | ||
|
|
@@ -218,13 +281,25 @@ static pj_bool_t ssl_on_accept_complete(pj_ssl_sock_t *ssock, | |
|
|
||
| /* Track accepted socket for cleanup */ | ||
| parent_st->accepted_ssock = newsock; | ||
| parent_st->accepted_st = st; | ||
|
|
||
| status = pj_ssl_sock_get_info(newsock, &info); | ||
| if (status != PJ_SUCCESS) { | ||
| app_perror("...ERROR pj_ssl_sock_get_info()", status); | ||
| goto on_return; | ||
| } | ||
|
|
||
| status = check_raw_chain(&info, st->pool, st->cert_der); | ||
| if (status != PJ_SUCCESS) { | ||
| app_perror("...ERROR invalid local raw chain", status); | ||
| /* Report through the parent and keep echoing: closing from this | ||
| * callback leaves on_handshake_complete() using a released | ||
| * ssock->parent, and perf_test() waits for every client to finish. | ||
| */ | ||
| parent_st->err = status; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning
Suggestion: record the error in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, both parts. Before: the callback set For the cleanup I had to pick between closing |
||
| status = PJ_SUCCESS; | ||
| } | ||
|
|
||
| pj_sockaddr_print(src_addr, buf, sizeof(buf), 1); | ||
| PJ_LOG(3, ("", "...Accepted connection from %s", buf)); | ||
|
|
||
|
|
@@ -691,6 +766,7 @@ static int echo_test(pj_ssl_sock_proto srv_proto, pj_ssl_sock_proto cli_proto, | |
| pj_sockaddr addr, listen_addr; | ||
| pj_ssl_cipher ciphers[1]; | ||
| pj_ssl_cert_t *cert = NULL; | ||
| pj_ssl_cert_t *cli_cert = NULL; | ||
| pj_status_t status; | ||
|
|
||
| pool = pj_pool_create(mem, "ssl_echo", 256, 256, NULL); | ||
|
|
@@ -731,6 +807,7 @@ static int echo_test(pj_ssl_sock_proto srv_proto, pj_ssl_sock_proto cli_proto, | |
| state_serv.echo = PJ_TRUE; | ||
| state_serv.is_server = PJ_TRUE; | ||
| state_serv.is_verbose = PJ_TRUE; | ||
| state_serv.cert_der = CERT_DER_FILE; | ||
|
|
||
| status = pj_ssl_sock_create(pool, ¶m, &ssock_serv); | ||
| if (status != PJ_SUCCESS) { | ||
|
|
@@ -834,6 +911,7 @@ static int echo_test(pj_ssl_sock_proto srv_proto, pj_ssl_sock_proto cli_proto, | |
| state_cli.pool = pool; | ||
| state_cli.check_echo = PJ_TRUE; | ||
| state_cli.is_verbose = PJ_TRUE; | ||
| state_cli.cert_der = CERT_DER_FILE; | ||
|
|
||
| { | ||
| /* srand() should be done centrally (blp) | ||
|
|
@@ -875,6 +953,8 @@ static int echo_test(pj_ssl_sock_proto srv_proto, pj_ssl_sock_proto cli_proto, | |
| } | ||
| #else | ||
| { | ||
| cli_cert = cert; | ||
|
|
||
| if (!client_provide_cert) { | ||
| pj_str_t ca_file = pj_str(CERT_CA_FILE); | ||
| pj_str_t null_str = pj_str(""); | ||
|
|
@@ -883,11 +963,12 @@ static int echo_test(pj_ssl_sock_proto srv_proto, pj_ssl_sock_proto cli_proto, | |
| #if (defined(TEST_LOAD_FROM_FILES) && TEST_LOAD_FROM_FILES==1) | ||
| /* Reset any certificate & private keys previously set */ | ||
| status = pj_ssl_cert_load_from_files(pool, &ca_file, &null_str, | ||
| &null_str, &null_str, &cert); | ||
| &null_str, &null_str, | ||
| &cli_cert); | ||
|
|
||
| /* Also reset any direct backend objects */ | ||
| pj_bzero(&cert_direct, sizeof(cert_direct)); | ||
| pj_ssl_cert_load_direct(pool, &cert_direct, &cert); | ||
| pj_ssl_cert_load_direct(pool, &cert_direct, &cli_cert); | ||
| #else | ||
| pj_ssl_cert_buffer null_buf, ca_buf; | ||
|
|
||
|
|
@@ -899,15 +980,53 @@ static int echo_test(pj_ssl_sock_proto srv_proto, pj_ssl_sock_proto cli_proto, | |
| } | ||
|
|
||
| status = pj_ssl_cert_load_from_buffer(pool, &ca_buf, &null_buf, | ||
| &null_buf, &null_str, &cert); | ||
| &null_buf, &null_str, | ||
| &cli_cert); | ||
| #endif | ||
| if (status != PJ_SUCCESS) { | ||
| goto on_return; | ||
| } | ||
|
|
||
| } | ||
| #if TEST_CLI_OWN_CERT | ||
| else { | ||
| /* A credential of its own, so each end's local certificate | ||
| * chain differs from the peer's in content as well as storage. | ||
| */ | ||
| pj_str_t ca_file = pj_str(CERT_CA_FILE); | ||
| pj_str_t cert_file = pj_str(CERT_CLI_FILE); | ||
| pj_str_t privkey_file = pj_str(CERT_CLI_PRIVKEY_FILE); | ||
| pj_str_t privkey_pass = pj_str(CERT_PRIVKEY_PASS); | ||
| pj_ssl_cert_t *own_cert = NULL; | ||
|
|
||
| #if (defined(TEST_LOAD_FROM_FILES) && TEST_LOAD_FROM_FILES==1) | ||
| status = pj_ssl_cert_load_from_files(pool, &ca_file, &cert_file, | ||
| &privkey_file, &privkey_pass, | ||
| &own_cert); | ||
| #else | ||
| pj_ssl_cert_buffer ca_buf, cert_buf, privkey_buf; | ||
|
|
||
| status = pj_ssl_sock_set_certificate(ssock_cli, pool, cert); | ||
| status = load_cert_to_buf(pool, &ca_file, &ca_buf); | ||
| if (status == PJ_SUCCESS) | ||
| status = load_cert_to_buf(pool, &cert_file, &cert_buf); | ||
| if (status == PJ_SUCCESS) | ||
| status = load_cert_to_buf(pool, &privkey_file, &privkey_buf); | ||
| if (status == PJ_SUCCESS) { | ||
| status = pj_ssl_cert_load_from_buffer(pool, &ca_buf, &cert_buf, | ||
| &privkey_buf, | ||
| &privkey_pass, &own_cert); | ||
| } | ||
| #endif | ||
| if (status != PJ_SUCCESS) { | ||
| goto on_return; | ||
| } | ||
|
|
||
| cli_cert = own_cert; | ||
| state_cli.cert_der = CERT_CLI_DER_FILE; | ||
| } | ||
| #endif | ||
|
|
||
| status = pj_ssl_sock_set_certificate(ssock_cli, pool, cli_cert); | ||
| if (status != PJ_SUCCESS) { | ||
| goto on_return; | ||
| } | ||
|
|
@@ -978,6 +1097,11 @@ static int echo_test(pj_ssl_sock_proto srv_proto, pj_ssl_sock_proto cli_proto, | |
|
|
||
| if (ssock_serv) | ||
| pj_ssl_sock_close(ssock_serv); | ||
| if (state_serv.accepted_st && !state_serv.accepted_st->err && | ||
| !state_serv.accepted_st->done) | ||
| { | ||
| pj_ssl_sock_close(state_serv.accepted_ssock); | ||
| } | ||
| if (ssock_cli && !state_cli.err && !state_cli.done) | ||
| pj_ssl_sock_close(ssock_cli); | ||
| if (ioqueue) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the raw chains now cleared at the top of the function, there's a new mismatch on this path. If
gnutls_x509_crt_init()succeeds but both imports fail (e.g. on renegotiation), we jump here withcert != NULL, solocal_cert_infois not zeroed. Subject, issuer, serial and SAN are kept from the previous handshake, butraw_chainis now empty. Before this PR, the stale raw chain at least matched the stale identity fields.peer_outhas the same issue forremote_cert_info. Suggest clearing the whole struct on any failure, not only whencertis NULL.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that mismatch was real. Changed it to track whether each half completed and zero the whole
pj_ssl_cert_infootherwise, at bothus_outandpeer_out.Before: the struct was zeroed only when
certwas NULL, so the init-succeeded-but-both-imports-failed path kept stale identity fields next to an empty raw chain. After: each half either fully refreshes its struct or fully clears it. That also made the tworaw_chainbzeros at the top redundant, so they're gone and the singlepj_pool_reset()is all that's left there.Tradeoff is the same one noted in the PR description, just wider: a failed import now reports empty identity fields too, instead of the previous handshake's values.