From c2434ec46d30dd9a2a59434915c672564b3e07c0 Mon Sep 17 00:00:00 2001 From: John Dennis Date: Aug 14 2018 20:05:22 +0000 Subject: Resolves: rhbz# 1614977 - fix unit test segfault, the problem was not limited exclusively to s390x, but s390x provoked it. --- diff --git a/mod_auth_openidc.spec b/mod_auth_openidc.spec index 07baa6d..4384584 100644 --- a/mod_auth_openidc.spec +++ b/mod_auth_openidc.spec @@ -15,7 +15,7 @@ Name: mod_auth_openidc Version: 2.3.7 -Release: 1%{?dist} +Release: 2%{?dist} Summary: OpenID Connect auth module for Apache HTTP Server Group: System Environment/Daemons @@ -23,6 +23,8 @@ License: ASL 2.0 URL: https://github.com/zmartzone/mod_auth_openidc Source0: https://github.com/zmartzone/mod_auth_openidc/releases/download/v%{version}/mod_auth_openidc-%{version}.tar.gz +Patch1: test-segfault.patch + BuildRequires: gcc BuildRequires: httpd-devel BuildRequires: openssl-devel @@ -42,6 +44,7 @@ an OpenID Connect Relying Party and/or OAuth 2.0 Resource Server. %prep %setup -q +%patch1 -p1 %build # workaround rpm-buildroot-usage @@ -94,6 +97,10 @@ install -m 700 -d $RPM_BUILD_ROOT%{httpd_pkg_cache_dir}/cache %dir %attr(0700, apache, apache) %{httpd_pkg_cache_dir}/cache %changelog +* Tue Aug 14 2018 - 2.3.7-2 +- Resolves: rhbz# 1614977 - fix unit test segfault, + the problem was not limited exclusively to s390x, but s390x provoked it. + * Wed Aug 1 2018 - 2.3.7-1 - upgrade to upstream 2.3.7 diff --git a/test-segfault.patch b/test-segfault.patch new file mode 100644 index 0000000..61dd1c8 --- /dev/null +++ b/test-segfault.patch @@ -0,0 +1,129 @@ +commit f7104535a5c686173c8cb875ae2ab56ab51b9e56 +Author: John Dennis +Date: Tue Aug 14 15:36:51 2018 -0400 + + test_proto_authorization_request() segfault due to uninitialized value + + Many thanks to Florian Weimer for his assistence + in helping diagnose the problem. + + In test_proto_authorization_request() it creates a provider object but + fails to fully initialize it. In particular it fails to initialize + auth_request_method to OIDC_AUTH_REQUEST_METHOD_GET. + + The function oidc_proto_authorization_request() in the file + src/proto.c has a weak test for provider->auth_request_method on line + 646. + + if (provider->auth_request_method == OIDC_AUTH_REQUEST_METHOD_POST) { + /* construct a HTML POST auto-submit page with the authorization request parameters */ + } else { + /* construct the full authorization request URL */ + } + + The assumption is provider->auth_request_method must be one of + OIDC_AUTH_REQUEST_METHOD_GET or OIDC_AUTH_REQUEST_METHOD_POST but if + the provider struct is not properly initialized it could be any random + value. Therefore the fact provider->auth_request_method does not equal + OIDC_AUTH_REQUEST_METHOD_POST does not imply it's + OIDC_AUTH_REQUEST_METHOD_GET. The test would also be a problem if + OIDC_AUTH_REQUEST_METHOD ever added a new enumerated value. + + The defined values for OIDC_AUTH_REQUEST_METHOD are: + + So what the test on line src/proto.c:646 is really saying is this: + if provider->auth_request_method != 1 then use the GET method. + + The unit test works most of the time because it's unlikely the + unitialized auth_request_method member will be exactly 1. Except on + some architectures it is (e.g. s390x). Of course what's on the stack + is influenced by a variety of factors (all unknown). + + The segfault occurs because oidc_proto_authorization_request() takes + the OIDC_AUTH_REQUEST_METHOD_POST branch and calls + oidc_proto_html_post() which then operates on more uninitialized + data. Specfically request->connection->bucket_alloc is + NULL. Fortunately the request object was intialized to zero via + apr_pcalloc() so at least bucket_alloc will consistetnly be NULL. + + Here is the stack trace: + + Program received signal SIGSEGV, Segmentation fault. + 0x00007ffff6b9f67a in apr_bucket_alloc () from /lib64/libaprutil-1.so.0 + (gdb) bt + from /lib64/libaprutil-1.so.0 + data=0x6adfe0 "\n\n \n \n Submitting"..., + data_len=825, content_type=0x47311a "text/html", success_rvalue=-2) + at src/util.c:1307 + title=0x46bf28 "Submitting...", html_head=0x0, + on_load=0x46bf0d "document.forms[0].submit()", + html_body=0x6add40 " <p>Submitting Authentication Request...</p>\n <form method=\"post\" action=\"https://idp.example.com/authorize\">\n <p>\n <input type=\"hidden\" name=\"response_type\" value=\"code\">\n <input"..., status_code=-2) at src/util.c:1349 + url=0x465758 "https://idp.example.com/authorize", params=0x6acf30) + at src/proto.c:544 + provider=0x7fffffffd260, login_hint=0x0, + redirect_uri=0x465790 "https://www.example.com/protected/", + state=0x4657b3 "12345", proto_state=0x68e5f0, id_token_hint=0x0, + nge=0x0, auth_request_params=0x0, path_scope=0x0) at src/proto.c:650 + + This patch does the following: + + 1) Initializes the provider struct created on the stack in + test_proto_authorization_request to zero so it's at least + initialized to known consistent values. + + 2) Initializes provider.auth_request_method to + OIDC_AUTH_REQUEST_METHOD_GET. + + 3) Initializes request->connection->bucket_alloc via + apr_bucket_alloc_create() so if one does enter that code it won't + segfault. + + It's easy to verify the above observations. + + If you explicitly intialize provider.auth_request_method to + OIDC_AUTH_REQUEST_METHOD_POST in test_proto_authorization_request() + instead of allowing it to be a random stack value you will segfault + every time. However if you initialize request->connection->bucket_alloc + the segfault goes away and the unit test correctly reports the error, + e.g. + + Failed: # test_proto_authorization_request: error in oidc_proto_authorization_request (1): result "0" != expected "1" + + WARNING: This patch does not address the test in proto.c:646. That + test should be augmented to assure only valid enumerated values are + operated on and if the enumerated value is not valid it should return + an error. + + Signed-off-by: John Dennis <jdennis@redhat.com> + +diff --git a/test/test.c b/test/test.c +index 16f09b5..87d3700 100755 +--- a/test/test.c ++++ b/test/test.c +@@ -1019,6 +1019,9 @@ static char *test_proto_validate_code(request_rec *r) { + static char * test_proto_authorization_request(request_rec *r) { + + oidc_provider_t provider; ++ ++ memset(&provider, 0, sizeof(provider)); ++ + provider.issuer = "https://idp.example.com"; + provider.authorization_endpoint_url = "https://idp.example.com/authorize"; + provider.scope = "openid"; +@@ -1028,6 +1031,8 @@ static char * test_proto_authorization_request(request_rec *r) { + provider.auth_request_params = NULL; + provider.request_object = NULL; + provider.token_binding_policy = OIDC_TOKEN_BINDING_POLICY_OPTIONAL; ++ provider.auth_request_method = OIDC_AUTH_REQUEST_METHOD_GET; ++ + const char *redirect_uri = "https://www.example.com/protected/"; + const char *state = "12345"; + +@@ -1260,6 +1265,7 @@ static request_rec * test_setup(apr_pool_t *pool) { + sizeof(struct process_rec)); + request->server->process->pool = request->pool; + request->connection = apr_pcalloc(request->pool, sizeof(struct conn_rec)); ++ request->connection->bucket_alloc = apr_bucket_alloc_create(request->pool); + request->connection->local_addr = apr_pcalloc(request->pool, + sizeof(apr_sockaddr_t)); +