Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

URL redirect and ValidateIssuer CodeQL bugs #9398

Merged
merged 7 commits into from Mar 3, 2023
Merged

Conversation

advay26
Copy link
Contributor

@advay26 advay26 commented Feb 22, 2023

Addresses https://github.com/NuGet/Engineering/issues/4790
Part of https://github.com/NuGet/Engineering/issues/4593

This PR addresses 4 NuGetGallery CodeQL issues.

  1. AuthenticationController.cs (2)
    • Problem: Untrusted URL redirection due to user-provided value. (Bug #1632796, Bug #1632797)
    • Solution: The redirect URLs used here are relative site URLs generated by us, and do not use any user-provided values. To avoid unwanted redirection, we now check that the returnUrl is a relative Url, and redirect to the 'Home' page if it isn't.
  2. ValidateRecaptchaResponseAttribute.cs (1)
    • Problem: Potential server side request forgery due to a user-provided value flowing to a constructor or method from class
      System.Net.Http.HttpClient. (Bug #1632800)
    • Solution: The Recaptcha validation URL flagged here does not use any user-provided values either. These values are generated by client-side reCAPTCHA code. The mitigation here is that the validation Url is limited to a specific host, which should prevent unwanted redirection. Added a suppression comment.
  3. AzureActiveDirectoryV2Authenticator.cs (1)
    • Problem: "The security sensitive property ValidateIssuer is being disabled by the followign value: false." We were disabling
      the ValidateIssuer flag in our AAD auth. When Issuer Validation is turned on, the issuer in the token is compared
      against a list of accepted tenants. (Bug #1632802)
    • Solution: We have a multi-tenant app, and do not restrict token issuers to a limited set of tenants, so we can keep this
      flag off. Added a suppression comment.

@advay26 advay26 requested a review from a team as a code owner February 22, 2023 22:29
drewgillies
drewgillies previously approved these changes Feb 22, 2023
…the suppression comment to reflect this as the mitigation for the vulnerability.
@advay26 advay26 requested a review from agr February 28, 2023 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants