Fix for SECURITY-2979 / CVE-2023-50771

[tumbl3w33d]

Hi, this fixes the open redirect vulnerability of #259 by validating. I've patched that for our internal use and I didn't work on the other open CVE because from my understanding it's only relevant if you enable this fallback user functionality which is not relevant for us. I thought it might still make sense to propose this here so others can quickly patch it for themselves or just use our fork release.

Please forgive me for not writing a test. I just wanted to share the fix to make the world a better place with the little time I have. Feel free to steal my code and do it properly.

Testing done

I manually tested this following these steps:

• install vulnerable version to craft a working URL
• this works to exploit it: https://jenkins.example.com/securityRealm/commenceLogin?from=https://google.com
• install my patched version of the plugin
• call the exploit URL again - result: no redirect anymore
• the rest of the functionality seems to work fine as before, but I didn't do much more than getting logged in via oidc

Submitter checklist

Edit tasklist title

Beta Give feedback Tasklist Submitter checklist, more options

• Rename
• Copy markdown
• Delete tasklist

Delete tasklist

Delete tasklist block?

Are you sure? All relationships in this tasklist will be removed.

Cancel Delete

1. Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!

   Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
2. Ensure that the pull request title represents the desired changelog entry

   Ensure that the pull request title represents the desired changelog entry
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
3. Please describe what you did

   Please describe what you did
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
4. Link to relevant issues in GitHub or Jira

   Link to relevant issues in GitHub or Jira
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
5. Link to relevant pull requests, esp. upstream and downstream changes

   Link to relevant pull requests, esp. upstream and downstream changes
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove
6. Ensure you have provided tests - that demonstrates feature works or fixes the issue

   Ensure you have provided tests - that demonstrates feature works or fixes the issue
   Options

   • Convert to issue
   • Toggle completion
   • Rename
   • Remove

Add item to Submitter checklist

[codecov]

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (742fe57) 71.21% compared to head (11f77a7) 70.46%.
Report is 3 commits behind head on master.

❗ Current head 11f77a7 differs from pull request most recent head f4d98ec. Consider uploading reports for the commit f4d98ec to get more accurate results

Files	Patch %	Lines
...va/org/jenkinsci/plugins/oic/OicSecurityRealm.java	8.33%	9 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #261      +/-   ##
============================================
- Coverage     71.21%   70.46%   -0.75%     
  Complexity      190      190              
============================================
  Files             9        9              
  Lines           740      745       +5     
  Branches        122      124       +2     
============================================
- Hits            527      525       -2     
- Misses          153      160       +7     
  Partials         60       60              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[michael-doubez]

Root URL is not always correctly setup (typicallty localhost on k8s deployments) and people rely on referer to deduce the correct URL as seen by the user.

[tumbl3w33d]

See my comment in the issue, please. The referer is passed in when calling the function.

[michael-doubez]

Please use referer if not empty.

[tumbl3w33d]

Hello @michael-doubez , thank you for your feedback. The referer is already passed in when the validation method is being called. The precedence is the same as before: from wins against Referer wins against root URL (which is also the fallback for all kinds of malformed URLs).

[michael-doubez]

Right.
LGTM

[tumbl3w33d]

I've added a test yet, but I cannot figure out how to determine the dynamic port of the wireMockRule. Hints appreciated. It has a constructor that accepts the rootUrl as string and I wanted to use that one but it was ignored in github ci.