|
 |
5b8408 |
commit fe7dfb14c45262df3b15bda374b2ee390b43cfb4
|
|
|
5b8408 |
Author: John Dennis <jdennis@redhat.com>
|
|
|
5b8408 |
Date: Tue Aug 14 18:08:56 2018 -0400
|
|
|
5b8408 |
|
|
|
5b8408 |
test_proto_authorization_request() segfault due to uninitialized value
|
|
|
5b8408 |
|
|
|
5b8408 |
Many thanks to Florian Weimer <fweimer@redhat.com> for his assistence
|
|
|
5b8408 |
in helping diagnose the problem.
|
|
|
5b8408 |
|
|
|
5b8408 |
In test_proto_authorization_request() it creates a provider object but
|
|
|
5b8408 |
fails to fully initialize it. In particular it fails to initialize
|
|
|
5b8408 |
auth_request_method to OIDC_AUTH_REQUEST_METHOD_GET.
|
|
|
5b8408 |
|
|
|
5b8408 |
The function oidc_proto_authorization_request() in the file
|
|
|
5b8408 |
src/proto.c has a weak test for provider->auth_request_method on line
|
|
|
5b8408 |
646.
|
|
|
5b8408 |
|
|
|
5b8408 |
if (provider->auth_request_method == OIDC_AUTH_REQUEST_METHOD_POST) {
|
|
|
5b8408 |
/* construct a HTML POST auto-submit page with the authorization request parameters */
|
|
|
5b8408 |
} else {
|
|
|
5b8408 |
/* construct the full authorization request URL */
|
|
|
5b8408 |
}
|
|
|
5b8408 |
|
|
|
5b8408 |
The assumption is provider->auth_request_method must be one of
|
|
|
5b8408 |
OIDC_AUTH_REQUEST_METHOD_GET or OIDC_AUTH_REQUEST_METHOD_POST but if
|
|
|
5b8408 |
the provider struct is not properly initialized it could be any random
|
|
|
5b8408 |
value. Therefore the fact provider->auth_request_method does not equal
|
|
|
5b8408 |
OIDC_AUTH_REQUEST_METHOD_POST does not imply it's
|
|
|
5b8408 |
OIDC_AUTH_REQUEST_METHOD_GET. The test would also be a problem if
|
|
|
5b8408 |
OIDC_AUTH_REQUEST_METHOD ever added a new enumerated value.
|
|
|
5b8408 |
|
|
|
5b8408 |
The defined values for OIDC_AUTH_REQUEST_METHOD are:
|
|
|
5b8408 |
define OIDC_AUTH_REQUEST_METHOD_GET 0
|
|
|
5b8408 |
define OIDC_AUTH_REQUEST_METHOD_POST 1
|
|
|
5b8408 |
|
|
|
5b8408 |
So what the test on line src/proto.c:646 is really saying is this:
|
|
|
5b8408 |
if provider->auth_request_method != 1 then use the GET method.
|
|
|
5b8408 |
|
|
|
5b8408 |
The unit test works most of the time because it's unlikely the
|
|
|
5b8408 |
unitialized auth_request_method member will be exactly 1. Except on
|
|
|
5b8408 |
some architectures it is (e.g. s390x). Of course what's on the stack
|
|
|
5b8408 |
is influenced by a variety of factors (all unknown).
|
|
|
5b8408 |
|
|
|
5b8408 |
The segfault occurs because oidc_proto_authorization_request() takes
|
|
|
5b8408 |
the OIDC_AUTH_REQUEST_METHOD_POST branch and calls
|
|
|
5b8408 |
oidc_proto_html_post() which then operates on more uninitialized
|
|
|
5b8408 |
data. Specfically request->connection->bucket_alloc is
|
|
|
5b8408 |
NULL. Fortunately the request object was intialized to zero via
|
|
|
5b8408 |
apr_pcalloc() so at least bucket_alloc will consistetnly be NULL.
|
|
|
5b8408 |
|
|
|
5b8408 |
Here is the stack trace:
|
|
|
5b8408 |
|
|
|
5b8408 |
Program received signal SIGSEGV, Segmentation fault.
|
|
|
5b8408 |
0x00007ffff6b9f67a in apr_bucket_alloc () from /lib64/libaprutil-1.so.0
|
|
|
5b8408 |
(gdb) bt
|
|
|
5b8408 |
from /lib64/libaprutil-1.so.0
|
|
|
5b8408 |
data=0x6adfe0 "\n<html>\n <head>\n <meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-8\">\n <title>Submitting"...,
|
|
|
5b8408 |
data_len=825, content_type=0x47311a "text/html", success_rvalue=-2)
|
|
|
5b8408 |
at src/util.c:1307
|
|
|
5b8408 |
title=0x46bf28 "Submitting...", html_head=0x0,
|
|
|
5b8408 |
on_load=0x46bf0d "document.forms[0].submit()",
|
|
|
5b8408 |
html_body=0x6add40 " Submitting Authentication Request... \n <form method=\"post\" action=\"https://idp.example.com/authorize\">\n \n <input type=\"hidden\" name=\"response_type\" value=\"code\">\n
|
|
|
5b8408 |
url=0x465758 "https://idp.example.com/authorize", params=0x6acf30)
|
|
|
5b8408 |
at src/proto.c:544
|
|
|
5b8408 |
provider=0x7fffffffd260, login_hint=0x0,
|
|
|
5b8408 |
redirect_uri=0x465790 "https://www.example.com/protected/",
|
|
|
5b8408 |
state=0x4657b3 "12345", proto_state=0x68e5f0, id_token_hint=0x0,
|
|
|
5b8408 |
nge=0x0, auth_request_params=0x0, path_scope=0x0) at src/proto.c:650
|
|
|
5b8408 |
|
|
|
5b8408 |
This patch does the following:
|
|
|
5b8408 |
|
|
|
5b8408 |
1) Initializes the provider struct created on the stack in
|
|
|
5b8408 |
test_proto_authorization_request to zero so it's at least
|
|
|
5b8408 |
initialized to known consistent values.
|
|
|
5b8408 |
|
|
|
5b8408 |
2) Initializes provider.auth_request_method to
|
|
|
5b8408 |
OIDC_AUTH_REQUEST_METHOD_GET.
|
|
|
5b8408 |
|
|
|
5b8408 |
3) Initializes request->connection->bucket_alloc via
|
|
|
5b8408 |
apr_bucket_alloc_create() so if one does enter that code it won't
|
|
|
5b8408 |
segfault.
|
|
|
5b8408 |
|
|
|
5b8408 |
It's easy to verify the above observations.
|
|
|
5b8408 |
|
|
|
5b8408 |
If you explicitly intialize provider.auth_request_method to
|
|
|
5b8408 |
OIDC_AUTH_REQUEST_METHOD_POST in test_proto_authorization_request()
|
|
|
5b8408 |
instead of allowing it to be a random stack value you will segfault
|
|
|
5b8408 |
every time. However if you initialize request->connection->bucket_alloc
|
|
|
5b8408 |
the segfault goes away and the unit test correctly reports the error,
|
|
|
5b8408 |
e.g.
|
|
|
5b8408 |
|
|
|
5b8408 |
Failed: # test_proto_authorization_request: error in oidc_proto_authorization_request (1): result "0" != expected "1"
|
|
|
5b8408 |
|
|
|
5b8408 |
WARNING: This patch does not address the test in proto.c:646. That
|
|
|
5b8408 |
test should be augmented to assure only valid enumerated values are
|
|
|
5b8408 |
operated on and if the enumerated value is not valid it should return
|
|
|
5b8408 |
an error.
|
|
|
5b8408 |
|
|
|
5b8408 |
Note: The above was fixed in the following commit.
|
|
|
5b8408 |
|
|
|
5b8408 |
Signed-off-by: John Dennis <jdennis@redhat.com>
|
|
|
5b8408 |
|
|
|
5b8408 |
diff --git a/test/test.c b/test/test.c
|
|
|
5b8408 |
index 16f09b5..87d3700 100755
|
|
|
5b8408 |
--- a/test/test.c
|
|
|
5b8408 |
+++ b/test/test.c
|
|
|
5b8408 |
@@ -1019,6 +1019,9 @@ static char *test_proto_validate_code(request_rec *r) {
|
|
|
5b8408 |
static char * test_proto_authorization_request(request_rec *r) {
|
|
|
5b8408 |
|
|
|
5b8408 |
oidc_provider_t provider;
|
|
|
5b8408 |
+
|
|
|
5b8408 |
+ memset(&provider, 0, sizeof(provider));
|
|
|
5b8408 |
+
|
|
|
5b8408 |
provider.issuer = "https://idp.example.com";
|
|
|
5b8408 |
provider.authorization_endpoint_url = "https://idp.example.com/authorize";
|
|
|
5b8408 |
provider.scope = "openid";
|
|
|
5b8408 |
@@ -1028,6 +1031,8 @@ static char * test_proto_authorization_request(request_rec *r) {
|
|
|
5b8408 |
provider.auth_request_params = NULL;
|
|
|
5b8408 |
provider.request_object = NULL;
|
|
|
5b8408 |
provider.token_binding_policy = OIDC_TOKEN_BINDING_POLICY_OPTIONAL;
|
|
|
5b8408 |
+ provider.auth_request_method = OIDC_AUTH_REQUEST_METHOD_GET;
|
|
|
5b8408 |
+
|
|
|
5b8408 |
const char *redirect_uri = "https://www.example.com/protected/";
|
|
|
5b8408 |
const char *state = "12345";
|
|
|
5b8408 |
|
|
|
5b8408 |
@@ -1260,6 +1265,7 @@ static request_rec * test_setup(apr_pool_t *pool) {
|
|
|
5b8408 |
sizeof(struct process_rec));
|
|
|
5b8408 |
request->server->process->pool = request->pool;
|
|
|
5b8408 |
request->connection = apr_pcalloc(request->pool, sizeof(struct conn_rec));
|
|
|
5b8408 |
+ request->connection->bucket_alloc = apr_bucket_alloc_create(request->pool);
|
|
|
5b8408 |
request->connection->local_addr = apr_pcalloc(request->pool,
|
|
|
5b8408 |
sizeof(apr_sockaddr_t));
|
|
|
5b8408 |
|
|
|
5b8408 |
commit aca77a82c1ce2f1ec8f363066ffbc480b3bd75c8
|
|
|
5b8408 |
Author: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
|
|
|
5b8408 |
Date: Wed Aug 15 07:47:57 2018 +0200
|
|
|
5b8408 |
|
|
|
5b8408 |
add sanity check on provider->auth_request_method; closes #382
|
|
|
5b8408 |
|
|
|
5b8408 |
thanks @jdennis; bump to 2.3.8rc4
|
|
|
5b8408 |
|
|
|
5b8408 |
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
|
|
|
5b8408 |
|
|
|
5b8408 |
diff --git a/src/proto.c b/src/proto.c
|
|
|
5b8408 |
index e9dbc99..ac7696a 100644
|
|
|
5b8408 |
--- a/src/proto.c
|
|
|
5b8408 |
+++ b/src/proto.c
|
|
|
5b8408 |
@@ -649,7 +649,7 @@ int oidc_proto_authorization_request(request_rec *r,
|
|
|
5b8408 |
rv = oidc_proto_html_post(r, provider->authorization_endpoint_url,
|
|
|
5b8408 |
params);
|
|
|
5b8408 |
|
|
|
5b8408 |
- } else {
|
|
|
5b8408 |
+ } else if (provider->auth_request_method == OIDC_AUTH_REQUEST_METHOD_GET) {
|
|
|
5b8408 |
|
|
|
5b8408 |
/* construct the full authorization request URL */
|
|
|
5b8408 |
authorization_request = oidc_util_http_query_encoded_url(r,
|
|
|
5b8408 |
@@ -666,6 +666,10 @@ int oidc_proto_authorization_request(request_rec *r,
|
|
|
5b8408 |
/* and tell Apache to return an HTTP Redirect (302) message */
|
|
|
5b8408 |
rv = HTTP_MOVED_TEMPORARILY;
|
|
|
5b8408 |
}
|
|
|
5b8408 |
+ } else {
|
|
|
5b8408 |
+ oidc_error(r, "provider->auth_request_method set to wrong value: %d",
|
|
|
5b8408 |
+ provider->auth_request_method);
|
|
|
5b8408 |
+ return HTTP_INTERNAL_SERVER_ERROR;
|
|
|
5b8408 |
}
|
|
|
5b8408 |
|
|
|
5b8408 |
/* add a referred token binding request for the provider if enabled */
|