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

Preview Give feedback

1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
2. Ensure that the pull request title represents the desired changelog entry
3. Please describe what you did
4. Link to relevant issues in GitHub or Jira
5. Link to relevant pull requests, esp. upstream and downstream changes
6. Ensure you have provided tests - that demonstrates feature works or fixes the issue

[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.