commit fe7dfb14c45262df3b15bda374b2ee390b43cfb4 Author: John Dennis Date: Tue Aug 14 18:08:56 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: define OIDC_AUTH_REQUEST_METHOD_GET 0 define OIDC_AUTH_REQUEST_METHOD_POST 1 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. Note: The above was fixed in the following commit. 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)); commit aca77a82c1ce2f1ec8f363066ffbc480b3bd75c8 Author: Hans Zandbelt <hans.zandbelt@zmartzone.eu> Date: Wed Aug 15 07:47:57 2018 +0200 add sanity check on provider->auth_request_method; closes #382 thanks @jdennis; bump to 2.3.8rc4 Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu> diff --git a/src/proto.c b/src/proto.c index e9dbc99..ac7696a 100644 --- a/src/proto.c +++ b/src/proto.c @@ -649,7 +649,7 @@ int oidc_proto_authorization_request(request_rec *r, rv = oidc_proto_html_post(r, provider->authorization_endpoint_url, params); - } else { + } else if (provider->auth_request_method == OIDC_AUTH_REQUEST_METHOD_GET) { /* construct the full authorization request URL */ authorization_request = oidc_util_http_query_encoded_url(r, @@ -666,6 +666,10 @@ int oidc_proto_authorization_request(request_rec *r, /* and tell Apache to return an HTTP Redirect (302) message */ rv = HTTP_MOVED_TEMPORARILY; } + } else { + oidc_error(r, "provider->auth_request_method set to wrong value: %d", + provider->auth_request_method); + return HTTP_INTERNAL_SERVER_ERROR; } /* add a referred token binding request for the provider if enabled */