Skip to content

Commit 5044966

Browse files
zandbeltzandbelt
committed
response: avoid proto memory leaks upon errors in response processing
refactor oidc_response_process Signed-off-by: Hans Zandbelt <[email protected]>
1 parent 6e4a14e commit 5044966

File tree

2 files changed

+86
-77
lines changed

2 files changed

+86
-77
lines changed

‎ChangeLog‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
which results in up to 7% performance increase, depending on the number of claims stored
55
- session/cookie: save 20-40 bytes on the session and client-cookie size
66
NB: the internal session format has changed and is backwards incompatible: existing sessions and cookies will be invalid
7+
- response: avoid proto state memory leaks upon errors in response processing
78
- drop support for Apache 2.2
89
- bump to 2.4.19dev
910

‎src/handle/response.c‎

Lines changed: 85 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@ static int oidc_response_authorization_error(request_rec *r, oidc_cfg_t *c, oidc
7272
const char *prompt = oidc_proto_state_get_prompt(proto_state);
7373
if (prompt != NULL)
7474
prompt = apr_pstrdup(r->pool, prompt);
75-
oidc_proto_state_destroy(proto_state);
7675
if ((prompt != NULL) && (_oidc_strcmp(prompt, OIDC_PROTO_PROMPT_NONE) == 0)) {
7776
return oidc_response_redirect_parent_window_to_logout(r, c);
7877
}
@@ -377,7 +376,6 @@ static apr_byte_t oidc_response_proto_state_restore(request_rec *r, oidc_cfg_t *
377376
r,
378377
"calculated state from cookie does not match state parameter passed back in URL: \"%s\" != \"%s\"",
379378
state, calc);
380-
oidc_proto_state_destroy(*proto_state);
381379
return FALSE;
382380
}
383381

@@ -396,7 +394,6 @@ static apr_byte_t oidc_response_proto_state_restore(request_rec *r, oidc_cfg_t *
396394
oidc_proto_state_get_original_url(*proto_state)),
397395
OK);
398396
}
399-
oidc_proto_state_destroy(*proto_state);
400397
return FALSE;
401398
}
402399

@@ -433,13 +430,7 @@ static apr_byte_t oidc_response_match_state(request_rec *r, oidc_cfg_t *c, const
433430

434431
*provider = oidc_get_provider_for_issuer(r, c, oidc_proto_state_get_issuer(*proto_state), FALSE);
435432

436-
if (*provider == NULL) {
437-
oidc_proto_state_destroy(*proto_state);
438-
*proto_state = NULL;
439-
return FALSE;
440-
}
441-
442-
return TRUE;
433+
return (*provider != NULL);
443434
}
444435

445436
/*
@@ -545,16 +536,25 @@ static char *_oidc_response_post_restore_template_contents = NULL;
545536
*/
546537
static int oidc_response_process(request_rec *r, oidc_cfg_t *c, oidc_session_t *session, apr_table_t *params,
547538
const char *response_mode) {
548-
549-
oidc_debug(r, "enter, response_mode=%s", response_mode);
550-
539+
int rc = -1;
551540
oidc_provider_t *provider = NULL;
552541
oidc_proto_state_t *proto_state = NULL;
553542
oidc_jwt_t *id_token = NULL;
543+
json_t *userinfo_claims = NULL;
544+
int expires_in = 0;
545+
char *userinfo_jwt = NULL;
546+
const char *s_userinfo_claims = NULL;
547+
const char *original_url = NULL;
548+
const char *original_method = NULL;
549+
const char *prompt = NULL;
550+
551+
oidc_debug(r, "enter, response_mode=%s", response_mode);
554552

555553
/* see if this response came from a browser-back event */
556-
if (oidc_response_browser_back(r, apr_table_get(params, OIDC_PROTO_STATE), session) == TRUE)
557-
return HTTP_MOVED_TEMPORARILY;
554+
if (oidc_response_browser_back(r, apr_table_get(params, OIDC_PROTO_STATE), session) == TRUE) {
555+
rc = HTTP_MOVED_TEMPORARILY;
556+
goto end;
557+
}
558558

559559
/* match the returned state parameter against the state stored in the browser */
560560
if (oidc_response_match_state(r, c, apr_table_get(params, OIDC_PROTO_STATE), &provider, &proto_state) ==
@@ -566,115 +566,109 @@ static int oidc_response_process(request_rec *r, oidc_cfg_t *c, oidc_session_t *
566566
oidc_cfg_default_sso_url_get(c));
567567
oidc_http_hdr_out_location_set(r, oidc_util_url_abs(r, c, oidc_cfg_default_sso_url_get(c)));
568568
OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_STATE_MISMATCH);
569-
return HTTP_MOVED_TEMPORARILY;
569+
rc = HTTP_MOVED_TEMPORARILY;
570+
goto end;
570571
}
571572
oidc_error(r,
572573
"invalid authorization response state and no default SSO URL is set, sending an error...");
573574

574575
// if error text was already produced (e.g. state timeout) then just return with a 400
575576
if (apr_table_get(r->subprocess_env, OIDC_ERROR_ENVVAR) != NULL) {
576577
OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_STATE_EXPIRED);
577-
return HTTP_BAD_REQUEST;
578+
rc = HTTP_BAD_REQUEST;
579+
goto end;
578580
}
579581

580582
OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_STATE_MISMATCH);
581583

582-
return oidc_util_html_send_error(r, "Invalid Authorization Response",
583-
"Could not match the authorization response to an earlier request via "
584-
"the state parameter and corresponding state cookie",
585-
HTTP_BAD_REQUEST);
584+
rc = oidc_util_html_send_error(r, "Invalid Authorization Response",
585+
"Could not match the authorization response to an earlier request via "
586+
"the state parameter and corresponding state cookie",
587+
HTTP_BAD_REQUEST);
588+
goto end;
586589
}
587590

588591
/* see if the response is an error response */
589592
if (apr_table_get(params, OIDC_PROTO_ERROR) != NULL) {
590593
OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_PROVIDER);
591-
return oidc_response_authorization_error(r, c, proto_state, apr_table_get(params, OIDC_PROTO_ERROR),
592-
apr_table_get(params, OIDC_PROTO_ERROR_DESCRIPTION));
594+
rc = oidc_response_authorization_error(r, c, proto_state, apr_table_get(params, OIDC_PROTO_ERROR),
595+
apr_table_get(params, OIDC_PROTO_ERROR_DESCRIPTION));
596+
goto end;
593597
}
594598

595599
/* handle the code, implicit or hybrid flow */
596600
if (oidc_response_flows(r, c, proto_state, provider, params, response_mode, &id_token) == FALSE) {
597601
OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_PROTOCOL);
598-
return oidc_response_authorization_error(r, c, proto_state, "Error in handling response type.", NULL);
602+
rc = oidc_response_authorization_error(r, c, proto_state, "Error in handling response type.", NULL);
603+
goto end;
599604
}
600605

601606
if (id_token == NULL) {
602607
oidc_error(r, "no id_token was provided");
603-
return oidc_response_authorization_error(r, c, proto_state, "No id_token was provided.", NULL);
608+
rc = oidc_response_authorization_error(r, c, proto_state, "No id_token was provided.", NULL);
609+
goto end;
604610
}
605611

606-
int expires_in = _oidc_str_to_int(apr_table_get(params, OIDC_PROTO_EXPIRES_IN), -1);
607-
char *userinfo_jwt = NULL;
612+
expires_in = _oidc_str_to_int(apr_table_get(params, OIDC_PROTO_EXPIRES_IN), -1);
608613

609614
/*
610615
* optionally resolve additional claims against the userinfo endpoint
611616
* parsed claims are not actually used here but need to be parsed anyway for error checking purposes
612617
*/
613-
json_t *userinfo_claims = NULL;
614-
const char *s_userinfo_claims = oidc_userinfo_retrieve_claims(
618+
s_userinfo_claims = oidc_userinfo_retrieve_claims(
615619
r, c, provider, apr_table_get(params, OIDC_PROTO_ACCESS_TOKEN),
616620
apr_table_get(params, OIDC_PROTO_TOKEN_TYPE), NULL, id_token->payload.sub, &userinfo_claims, &userinfo_jwt);
617621

618622
/* restore the original protected URL that the user was trying to access */
619-
const char *original_url = oidc_proto_state_get_original_url(proto_state);
623+
original_url = oidc_proto_state_get_original_url(proto_state);
620624
if (original_url != NULL)
621625
original_url = apr_pstrdup(r->pool, original_url);
622-
const char *original_method = oidc_proto_state_get_original_method(proto_state);
626+
original_method = oidc_proto_state_get_original_method(proto_state);
623627
if (original_method != NULL)
624628
original_method = apr_pstrdup(r->pool, original_method);
625-
const char *prompt = oidc_proto_state_get_prompt(proto_state);
629+
prompt = oidc_proto_state_get_prompt(proto_state);
626630

627631
/* set the user */
628-
if (oidc_response_set_request_user(r, c, provider, id_token, userinfo_claims) == TRUE) {
629-
630-
/* session management: if the user in the new response is not equal to the old one, error out */
631-
if ((prompt != NULL) && (_oidc_strcmp(prompt, OIDC_PROTO_PROMPT_NONE) == 0)) {
632-
// TOOD: actually need to compare sub? (need to store it in the session separately then
633-
// const char *sub = NULL;
634-
// oidc_session_get(r, session, "sub", &sub);
635-
// if (_oidc_strcmp(sub, jwt->payload.sub) != 0) {
636-
if (_oidc_strcmp(session->remote_user, r->user) != 0) {
637-
oidc_warn(r, "user set from new id_token is different from current one");
638-
oidc_jwt_destroy(id_token);
639-
json_decref(userinfo_claims);
640-
return oidc_response_authorization_error(r, c, proto_state, "User changed!", NULL);
641-
}
642-
}
643-
644-
/* store resolved information in the session */
645-
if (oidc_response_save_in_session(
646-
r, c, session, provider, r->user, apr_table_get(params, OIDC_PROTO_ID_TOKEN), id_token,
647-
s_userinfo_claims, userinfo_claims, apr_table_get(params, OIDC_PROTO_ACCESS_TOKEN),
648-
apr_table_get(params, OIDC_PROTO_TOKEN_TYPE), expires_in,
649-
apr_table_get(params, OIDC_PROTO_REFRESH_TOKEN), apr_table_get(params, OIDC_PROTO_SCOPE),
650-
apr_table_get(params, OIDC_PROTO_SESSION_STATE), apr_table_get(params, OIDC_PROTO_STATE),
651-
original_url, userinfo_jwt) == FALSE) {
652-
oidc_proto_state_destroy(proto_state);
653-
oidc_jwt_destroy(id_token);
654-
json_decref(userinfo_claims);
655-
return HTTP_INTERNAL_SERVER_ERROR;
656-
}
657-
658-
oidc_debug(r, "set remote_user to \"%s\" in new session \"%s\"", r->user, session->uuid);
659-
660-
} else {
632+
if (oidc_response_set_request_user(r, c, provider, id_token, userinfo_claims) == FALSE) {
661633
oidc_error(r, "remote user could not be set");
662-
oidc_jwt_destroy(id_token);
663-
json_decref(userinfo_claims);
664634
OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_REMOTE_USER);
665-
return oidc_response_authorization_error(
635+
rc = oidc_response_authorization_error(
666636
r, c, proto_state, "Remote user could not be set: contact the website administrator", NULL);
637+
goto end;
638+
}
639+
640+
oidc_debug(r, "set remote_user to \"%s\" in new session \"%s\"", r->user, session->uuid);
641+
642+
/* session management: if the user in the new response is not equal to the old one, error out */
643+
if ((prompt != NULL) && (_oidc_strcmp(prompt, OIDC_PROTO_PROMPT_NONE) == 0)) {
644+
// TOOD: actually need to compare sub? (need to store it in the session separately then
645+
// const char *sub = NULL;
646+
// oidc_session_get(r, session, "sub", &sub);
647+
// if (_oidc_strcmp(sub, jwt->payload.sub) != 0) {
648+
if (_oidc_strcmp(session->remote_user, r->user) != 0) {
649+
oidc_warn(r, "user set from new id_token is different from current one");
650+
rc = oidc_response_authorization_error(r, c, proto_state, "User changed!", NULL);
651+
goto end;
652+
}
667653
}
668654

669-
/* cleanup */
670-
oidc_proto_state_destroy(proto_state);
671-
oidc_jwt_destroy(id_token);
672-
json_decref(userinfo_claims);
655+
/* store resolved information in the session */
656+
if (oidc_response_save_in_session(
657+
r, c, session, provider, r->user, apr_table_get(params, OIDC_PROTO_ID_TOKEN), id_token,
658+
s_userinfo_claims, userinfo_claims, apr_table_get(params, OIDC_PROTO_ACCESS_TOKEN),
659+
apr_table_get(params, OIDC_PROTO_TOKEN_TYPE), expires_in,
660+
apr_table_get(params, OIDC_PROTO_REFRESH_TOKEN), apr_table_get(params, OIDC_PROTO_SCOPE),
661+
apr_table_get(params, OIDC_PROTO_SESSION_STATE), apr_table_get(params, OIDC_PROTO_STATE), original_url,
662+
userinfo_jwt) == FALSE) {
663+
rc = HTTP_INTERNAL_SERVER_ERROR;
664+
goto end;
665+
}
673666

674667
/* check that we've actually authenticated a user; functions as error handling for oidc_get_remote_user */
675668
if (r->user == NULL) {
676669
OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_REMOTE_USER);
677-
return HTTP_UNAUTHORIZED;
670+
rc = HTTP_UNAUTHORIZED;
671+
goto end;
678672
}
679673

680674
/* log the successful response */
@@ -683,18 +677,32 @@ static int oidc_response_process(request_rec *r, oidc_cfg_t *c, oidc_session_t *
683677

684678
/* check whether form post data was preserved; if so restore it */
685679
if (_oidc_strcmp(original_method, OIDC_METHOD_FORM_POST) == 0) {
686-
if (oidc_cfg_post_restore_template_get(c) != NULL)
687-
return oidc_util_html_send_in_template(
680+
if (oidc_cfg_post_restore_template_get(c) != NULL) {
681+
rc = oidc_util_html_send_in_template(
688682
r, oidc_cfg_post_restore_template_get(c), &_oidc_response_post_restore_template_contents,
689683
original_url, OIDC_POST_PRESERVE_ESCAPE_JAVASCRIPT, "", OIDC_POST_PRESERVE_ESCAPE_NONE);
690-
return oidc_response_post_preserved_restore(r, original_url);
684+
} else {
685+
rc = oidc_response_post_preserved_restore(r, original_url);
686+
}
687+
goto end;
691688
}
692689

693690
/* now we've authenticated the user so go back to the URL that he originally tried to access */
694691
oidc_http_hdr_out_location_set(r, original_url);
695692

696693
/* do the actual redirect to the original URL */
697-
return HTTP_MOVED_TEMPORARILY;
694+
rc = HTTP_MOVED_TEMPORARILY;
695+
696+
end:
697+
698+
if (proto_state)
699+
oidc_proto_state_destroy(proto_state);
700+
if (id_token)
701+
oidc_jwt_destroy(id_token);
702+
if (userinfo_claims)
703+
json_decref(userinfo_claims);
704+
705+
return rc;
698706
}
699707

700708
/*

0 commit comments

Comments
 (0)