diff --git a/test-segfault.patch b/test-segfault.patch deleted file mode 100644 index 34bf7f0..0000000 --- a/test-segfault.patch +++ /dev/null @@ -1,167 +0,0 @@ -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 */