Project

General

Profile

Bug #54689

Dead code after the recent CVE patch

Added by Jakub Hrozek 9 days ago.

Status:
Nouveau
Priority:
Normal
Assignee:
-
Category:
-
Target version:
-
Start date:
09 Jun 2021
Due date:
% Done:

0%

Estimated time:
Patch proposed:
No
Planning:
No

Description

I ran the recent CVE patch through Coverity and Coverity found out that there is unreachable code in lasso_saml20_login_process_response_status_and_assertion.

This part:

        switch (verify_hint) {
                case LASSO_PROFILE_SIGNATURE_VERIFY_HINT_FORCE:
                case LASSO_PROFILE_SIGNATURE_VERIFY_HINT_MAYBE:
                        break;
                case LASSO_PROFILE_SIGNATURE_VERIFY_HINT_IGNORE:
                        /* ignore signature errors */
                        if (rc == LASSO_PROFILE_ERROR_CANNOT_VERIFY_SIGNATURE) {
                                rc = 0;
                        }
                        break;
                default:
                        g_assert(0);
        }

rc is only ever assigned to here:

                      if (lasso_strisnotequal(in_response_to,login->private_data->request_id)) {
                                rc = LASSO_LOGIN_ERROR_ASSERTION_DOES_NOT_MATCH_REQUEST_ID;
                                goto cleanup;
                        }

and the rc value can therefore never be LASSO_PROFILE_ERROR_CANNOT_VERIFY_SIGNATURE.

I'm unsure what the code was supposed to do there? Reading the code seems correct to me as if the signature validation fails, the code is always ignored, but I'm unsure if the condition on 1440:

»·······»·······if (profile->signature_status != 0) {
»·······»·······»·······/* When response signature is not present */
»·······»·······»·······if (verify_hint == LASSO_PROFILE_SIGNATURE_VERIFY_HINT_MAYBE) {
»·······»·······»·······»·······assertion_signature_status =
»·······»·······»·······»·······»·······lasso_saml20_login_check_assertion_signature(login, assertion);
»·······»·······»·······»·······if (assertion_signature_status) {
»·······»·······»·······»·······»·······goto_cleanup_with_rc(assertion_signature_status);
»·······»·······»·······»·······}
»·······»·······»·······}

is also supposed to handle HINT_FORCE?

Sorry I'm not contributing a patch, but this code seems complex and I still don't know my way around lasso well enough.

Also available in: Atom PDF