diff --git a/src/cpp/security/accesscontrol/Permissions.cpp b/src/cpp/security/accesscontrol/Permissions.cpp index 55915643117..2d54a312112 100644 --- a/src/cpp/security/accesscontrol/Permissions.cpp +++ b/src/cpp/security/accesscontrol/Permissions.cpp @@ -344,6 +344,92 @@ static bool rfc2253_string_compare( } // Auxiliary functions +static BIO* load_and_verify_document( + X509_STORE* store, + BIO* in, + SecurityException& exception) +{ + BIO* out = nullptr; + + assert(nullptr != store); + assert(nullptr != in); + + // Create PKCS7 object from input data + BIO* indata = nullptr; + PKCS7* p7 = SMIME_read_PKCS7(in, &indata); + if (nullptr == p7) + { + exception = _SecurityException_("Input data has not PKCS7 S/MIME format"); + return nullptr; + } + + // ---- Get certificate stack from store ---- + // The following lines are almost equivalent to the OpenSSL 3 API X509_STORE_get1_all_certs. + // It creates a stack of X509 objects and populates the stack with the X509 objects contained in the store. + STACK_OF(X509) * stack = sk_X509_new_null(); + if (nullptr == stack) + { + exception = _SecurityException_("Cannot allocate certificate stack"); + } + else + { + STACK_OF(X509_OBJECT) * objects = X509_STORE_get0_objects(store); + int i = 0; + for (i = 0; i < sk_X509_OBJECT_num(objects); i++) + { + X509_OBJECT* tmp = sk_X509_OBJECT_value(objects, i); + X509* cert = X509_OBJECT_get0_X509(tmp); + if (nullptr != cert) + { + sk_X509_push(stack, cert); + } + } + + if (1 != sk_X509_num(stack)) + { + exception = _SecurityException_("Certificate store should have exactly one certificate"); + + sk_X509_free(stack); + stack = nullptr; + } + } + // ---- Finished getting certificate stack from store ---- + + if (nullptr != stack) + { + // Allocate output data + out = BIO_new(BIO_s_mem()); + if (nullptr == out) + { + exception = _SecurityException_("Cannot allocate output BIO"); + } + else + { + // Verify the input data using exclusively the certificates in the stack. + // PKCS7_NOINTERN is used to ignore certificates coming alongside the signed data. + // PKCS7_NOVERIFY is used since the permissions CA certificate will not be chain verified. + if (!PKCS7_verify(p7, stack, nullptr, indata, out, PKCS7_TEXT | PKCS7_NOVERIFY | PKCS7_NOINTERN)) + { + exception = _SecurityException_("PKCS7 data verification failed"); + BIO_free(out); + out = nullptr; + } + } + + // Free the certificate stack + sk_X509_free(stack); + } + + PKCS7_free(p7); + + if (indata != nullptr) + { + BIO_free(indata); + } + + return out; +} + static X509_STORE* load_permissions_ca( const std::string& permissions_ca, bool& there_are_crls, @@ -464,34 +550,9 @@ static BIO* load_signed_file( if (file.size() >= 7 && file.compare(0, 7, "file://") == 0) { BIO* in = BIO_new_file(file.substr(7).c_str(), "r"); - if (in != nullptr) { - BIO* indata = nullptr; - PKCS7* p7 = SMIME_read_PKCS7(in, &indata); - - if (p7 != nullptr) - { - out = BIO_new(BIO_s_mem()); - if (!PKCS7_verify(p7, nullptr, store, indata, out, PKCS7_TEXT)) - { - exception = _SecurityException_(std::string("Failed verification of the file ") + file); - BIO_free(out); - out = nullptr; - } - - PKCS7_free(p7); - } - else - { - exception = _SecurityException_(std::string("Cannot read as PKCS7 the file ") + file); - } - - if (indata != nullptr) - { - BIO_free(indata); - } - + out = load_and_verify_document(store, in, exception); BIO_free(in); } else @@ -596,57 +657,39 @@ static bool verify_permissions_file( if (permissions_file.size() <= static_cast(std::numeric_limits::max())) { BIO* permissions_buf = BIO_new_mem_buf(permissions_file.data(), static_cast(permissions_file.size())); - if (permissions_buf != nullptr) { - BIO* indata = nullptr; - PKCS7* p7 = SMIME_read_PKCS7(permissions_buf, &indata); - - if (p7 != nullptr) + BIO* out = load_and_verify_document(local_handle->store_, permissions_buf, exception); + if (nullptr != out) { - BIO* out = BIO_new(BIO_s_mem()); - if (PKCS7_verify(p7, nullptr, local_handle->store_, indata, out, PKCS7_TEXT)) - { - BUF_MEM* ptr = nullptr; - BIO_get_mem_ptr(out, &ptr); + BUF_MEM* ptr = nullptr; + BIO_get_mem_ptr(out, &ptr); - if (ptr != nullptr) + if (ptr != nullptr) + { + PermissionsParser parser; + if ((returned_value = parser.parse_stream(ptr->data, ptr->length)) == true) { - PermissionsParser parser; - if ((returned_value = parser.parse_stream(ptr->data, ptr->length)) == true) - { - parser.swap(permissions); - returned_value = true; - } - else - { - exception = _SecurityException_(std::string( - "Malformed permissions file ") + permissions_file); - } + parser.swap(permissions); + returned_value = true; } else { - exception = _SecurityException_("OpenSSL library cannot retrieve mem ptr from file."); + exception = _SecurityException_(std::string( + "Malformed permissions file ") + permissions_file); } } else { - exception = _SecurityException_("Failed verification of the permissions file"); + exception = _SecurityException_("OpenSSL library cannot retrieve mem ptr from file."); } - BIO_free(out); - PKCS7_free(p7); } else { exception = _SecurityException_("Cannot read as PKCS7 the permissions file."); } - if (indata != nullptr) - { - BIO_free(indata); - } - BIO_free(permissions_buf); } } diff --git a/test/certs/permissions_access_control_tests_malicious.smime b/test/certs/permissions_access_control_tests_malicious.smime new file mode 100644 index 00000000000..9643d54f4bb --- /dev/null +++ b/test/certs/permissions_access_control_tests_malicious.smime @@ -0,0 +1,212 @@ +MIME-Version: 1.0 +Content-Type: multipart/signed; protocol="application/x-pkcs7-signature"; micalg="sha-256"; boundary="----ED510A215DE6A2BE22C4071F88DD9E0A" + +This is an S/MIME signed message + +------ED510A215DE6A2BE22C4071F88DD9E0A +Content-Type: text/plain + + + + + + emailAddress=mainpub@eprosima.com, CN=Main Publisher, OU=eProsima, O=eProsima, ST=MA, C=ES + + 2013-06-01T13:00:00 + 2038-06-01T13:00:00 + + + + + 0 + 230 + + + + + HelloWorldTopic_no_partitions + + + + + + + 0 + 230 + + + + + HelloWorldTopic_no_partitions_wildcards* + + + + + + + 0 + 230 + + + + + HelloWorldTopic_single_partition + + + Partition + + + + + + + 0 + 230 + + + + + HelloWorldTopic_multiple_partition + + + Partition1 + Partition2 + Partition3 + + + + + + + 0 + 230 + + + + + HelloWorldTopic_partition_wildcards + + + Partition* + + + + DENY + + + emailAddress=mainsub@eprosima.com, CN=Main Subscriber, OU=eProsima, O=eProsima, ST=MA, C=ES + + 2013-06-01T13:00:00 + 2038-06-01T13:00:00 + + + + + 0 + 230 + + + + + HelloWorldTopic_no_partitions + + + + + + + 0 + 230 + + + + + HelloWorldTopic_no_partitions_wildcards* + + + + + + + 0 + 230 + + + + + HelloWorldTopic_single_partition + + + Partition + + + + + + + 0 + 230 + + + + + HelloWorldTopic_multiple_partition + + + Partition* + + + + + + + 0 + 230 + + + + + HelloWorldTopic_partition_wildcards + + + Partition* + + + + ALLOW + + + + +------ED510A215DE6A2BE22C4071F88DD9E0A +Content-Type: application/x-pkcs7-signature; name="smime.p7s" +Content-Transfer-Encoding: base64 +Content-Disposition: attachment; filename="smime.p7s" + +MIIEXAYJKoZIhvcNAQcCoIIETTCCBEkCAQExDzANBglghkgBZQMEAgEFADALBgkq +hkiG9w0BBwGgggIjMIICHzCCAcSgAwIBAgIJAK57rYwHWur0MAoGCCqGSM49BAMC +MIGaMQswCQYDVQQGEwJFUzELMAkGA1UECAwCTUExFDASBgNVBAcMC1RyZXMgQ2Fu +dG9zMREwDwYDVQQKDAhlUHJvc2ltYTERMA8GA1UECwwIZVByb3NpbWExHjAcBgNV +BAMMFWVQcm9zaW1hIE1haW4gVGVzdCBDQTEiMCAGCSqGSIb3DQEJARYTbWFpbmNh +QGVwcm9zaW1hLmNvbTAeFw0xNzA5MDYwOTA1MDFaFw0yNzA5MDQwOTA1MDFaMH8x +CzAJBgNVBAYTAkVTMQswCQYDVQQIDAJNQTERMA8GA1UECgwIZVByb3NpbWExETAP +BgNVBAsMCGVQcm9zaW1hMRgwFgYDVQQDDA9NYWluIFN1YnNjcmliZXIxIzAhBgkq +hkiG9w0BCQEWFG1haW5zdWJAZXByb3NpbWEuY29tMFkwEwYHKoZIzj0CAQYIKoZI +zj0DAQcDQgAEvLTbfpn4JKn5zk2rTkp8F3OfKhUNOw3195MoC98A9T2WhBHG5Pw4 +e70iaIrXB4DYPLmwwlWy6wyNp6R1cIsWPaMNMAswCQYDVR0TBAIwADAKBggqhkjO +PQQDAgNJADBGAiEA4KoekGfZ7zfPqjDVipwADH12uGFJ60p+Q2286ixQcQUCIQDt +c5kC8oK8SWlDlxkUptCYakm64653IKttpUO48Vu7RjGCAf0wggH5AgEBMIGoMIGa +MQswCQYDVQQGEwJFUzELMAkGA1UECAwCTUExFDASBgNVBAcMC1RyZXMgQ2FudG9z +MREwDwYDVQQKDAhlUHJvc2ltYTERMA8GA1UECwwIZVByb3NpbWExHjAcBgNVBAMM +FWVQcm9zaW1hIE1haW4gVGVzdCBDQTEiMCAGCSqGSIb3DQEJARYTbWFpbmNhQGVw +cm9zaW1hLmNvbQIJAK57rYwHWur0MA0GCWCGSAFlAwQCAQUAoIHkMBgGCSqGSIb3 +DQEJAzELBgkqhkiG9w0BBwEwHAYJKoZIhvcNAQkFMQ8XDTIzMDIxMDEyMjMwOVow +LwYJKoZIhvcNAQkEMSIEIOleYp0gOJKaxYnopPDeWCNPrTkYqFHQ6UUyrCk1W0s1 +MHkGCSqGSIb3DQEJDzFsMGowCwYJYIZIAWUDBAEqMAsGCWCGSAFlAwQBFjALBglg +hkgBZQMEAQIwCgYIKoZIhvcNAwcwDgYIKoZIhvcNAwICAgCAMA0GCCqGSIb3DQMC +AgFAMAcGBSsOAwIHMA0GCCqGSIb3DQMCAgEoMAoGCCqGSM49BAMCBEcwRQIhAKb2 +IuEgMrucHVPG1pShR9ijHxudXCw8RDnH7n/n0YEuAiAFKmoOKCT2onBHSZbSxPFX +gdh3RDI0oqZpmbDSIBSAmg== + +------ED510A215DE6A2BE22C4071F88DD9E0A-- + diff --git a/test/certs/permissions_access_control_tests_malicious.xml b/test/certs/permissions_access_control_tests_malicious.xml new file mode 100644 index 00000000000..19220472847 --- /dev/null +++ b/test/certs/permissions_access_control_tests_malicious.xml @@ -0,0 +1,171 @@ + + + + + emailAddress=mainpub@eprosima.com, CN=Main Publisher, OU=eProsima, O=eProsima, ST=MA, C=ES + + 2013-06-01T13:00:00 + 2038-06-01T13:00:00 + + + + + 0 + 230 + + + + + HelloWorldTopic_no_partitions + + + + + + + 0 + 230 + + + + + HelloWorldTopic_no_partitions_wildcards* + + + + + + + 0 + 230 + + + + + HelloWorldTopic_single_partition + + + Partition + + + + + + + 0 + 230 + + + + + HelloWorldTopic_multiple_partition + + + Partition1 + Partition2 + Partition3 + + + + + + + 0 + 230 + + + + + HelloWorldTopic_partition_wildcards + + + Partition* + + + + DENY + + + emailAddress=mainsub@eprosima.com, CN=Main Subscriber, OU=eProsima, O=eProsima, ST=MA, C=ES + + 2013-06-01T13:00:00 + 2038-06-01T13:00:00 + + + + + 0 + 230 + + + + + HelloWorldTopic_no_partitions + + + + + + + 0 + 230 + + + + + HelloWorldTopic_no_partitions_wildcards* + + + + + + + 0 + 230 + + + + + HelloWorldTopic_single_partition + + + Partition + + + + + + + 0 + 230 + + + + + HelloWorldTopic_multiple_partition + + + Partition* + + + + + + + 0 + 230 + + + + + HelloWorldTopic_partition_wildcards + + + Partition* + + + + ALLOW + + + diff --git a/test/unittest/security/accesscontrol/AccessControlTests.cpp b/test/unittest/security/accesscontrol/AccessControlTests.cpp index 3fffee745d7..c3aabb6db3c 100644 --- a/test/unittest/security/accesscontrol/AccessControlTests.cpp +++ b/test/unittest/security/accesscontrol/AccessControlTests.cpp @@ -62,7 +62,8 @@ class AccessControlTest : public ::testing::Test void get_access_handle( const RTPSParticipantAttributes& participant_attr, - PermissionsHandle** access_handle); + PermissionsHandle** access_handle, + bool should_success = true); void fill_candidate_participant_key(); GUID_t candidate_participant_key; @@ -159,7 +160,8 @@ void AccessControlTest::fill_common_participant_security_attributes( void AccessControlTest::get_access_handle( const RTPSParticipantAttributes& participant_attr, - PermissionsHandle** access_handle) + PermissionsHandle** access_handle, + bool should_success) { IdentityHandle* identity_handle = nullptr; @@ -186,7 +188,8 @@ void AccessControlTest::get_access_handle( participant_attr, exception); - ASSERT_TRUE(*access_handle != nullptr) << exception.what(); + bool success = *access_handle != nullptr; + ASSERT_EQ(success, should_success) << exception.what(); ASSERT_TRUE(authentication_plugin.return_identity_handle(identity_handle, exception)) << exception.what(); } @@ -448,6 +451,21 @@ TEST_F(AccessControlTest, validate_partition_access_ok_on_permission_wildcards) check_remote_datawriter(publisher_participant_attr, true); } +/* Regression test for Redmine 17099 (Github 3239). + * + * Using a modified permissions file signed with the subscriber identity certificate should fail. + */ +TEST_F(AccessControlTest, validate_fail_on_self_signed_permissions) +{ + permissions_file = "permissions_access_control_tests_malicious.smime"; + + RTPSParticipantAttributes subscriber_participant_attr; + fill_subscriber_participant_security_attributes(subscriber_participant_attr); + + PermissionsHandle* access_handle; + get_access_handle(subscriber_participant_attr, &access_handle, false); +} + int main( int argc, char** argv)