search

hypothesis / product-backlog

Where new feature ideas and current bugs for the Hypothesis product live
118 stars 7 forks source link

Security vulnerability - V-004 Potential OAuth Authentication Bypass - `Low` #1522

Closed indigobravo closed 8 months ago

indigobravo commented 1 year ago

Overview

Impacts

Description

The following is taken from the Draft SubGraph 2023 Security Review

Discussion

Note: Strict CVSS3 rating results in a score in the medium range, however Subgraph has downgraded this to low given the low probability of exploitability.

There is a potential theoretical vulnerability in the implementation of OAuth client authentication. The potentially problematic code is located here:

provided_secret = request.client_secret
        if request.client_secret is None:
            # hmac.compare_digest raises when one value is `None`
            provided_secret = ""

        if not hmac.compare_digest(client.secret, provided_secret):
            return False

        request.client = Client(client)
        return True

The above code is from a function that authenticates an OAuth client. The snippet covers the case where client_secret evaluates (for some reason) to None; in that case the empty string value is assigned. The reason for this is because hmac.compare_digest() will throw an exception if one of the two values being compared is None. From a security perspective, this codes creates a theoretical possibility that, if the client_secret is None, then authenticated messages could be forged as the secret falls back to a known, hardcoded value. The right approach may be to fail the validation if the value of the client_secret is None. This wasn’t tested or reproduced, so it is not know how or when, if ever, this issue could manifest. It may only be possible in circumstances where there is major human error.

Impact Analysis

This is a low risk issue. In order for this to be exploitable, the client_secret must evaluate to the Python constant None. The circumstances in which this could occur are not fully understood, and may never practically exist, though, if there are any such circumstances, it is possible that an adversary could forge e.g. launch requests.

Remediation Recommendations

The risk of this function should be more deeply understood, and, if warranted, more robust error handling for this case should be implemented.

robertknight commented 11 months ago

OAuth clients in this function should always have a non-empty secret. It would be easy to just return False if the request doesn't include a secret. I think we should just do that.

indigobravo commented 11 months ago

This is the final outstanding finding from the 2023 Subgraph security audit. Once this issue has been fixed we can ask for a retest and an updated report to be produced.

Additional / already fixed findings