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

JENKINS-54273: RAW HTML is shown in maven deployment links since 2.138.2 #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bguerin
Copy link

@bguerin bguerin commented Nov 24, 2018

No description provided.

@daniel-beck
Copy link
Member

daniel-beck commented Nov 24, 2018

I recommend a detailed security review before merging this. A similar PR (re-)introduced a security vulnerability in another plugin, as the input was not actually safe.

@bguerin
Copy link
Author

bguerin commented Nov 24, 2018

@daniel-beck
Copy link
Member

daniel-beck commented Nov 24, 2018

Please think about the data flow here.

screen shot

The lastIndexOf/substring combination for the link label makes this minimally weird, but it's still really easy to exploit (hence output is starting with Uploading: / in the build log).

Build script that works on any Linux/Unix:

<project>
  <actions/>
  <description></description>
  <keepDependencies>false</keepDependencies>
  <properties/>
  <scm class="hudson.scm.NullSCM"/>
  <canRoam>true</canRoam>
  <disabled>false</disabled>
  <blockBuildWhenDownstreamBuilding>false</blockBuildWhenDownstreamBuilding>
  <blockBuildWhenUpstreamBuilding>false</blockBuildWhenUpstreamBuilding>
  <triggers/>
  <concurrentBuild>false</concurrentBuild>
  <builders>
    <hudson.tasks.Shell>
      <command>#!/bin/bash
cat &lt;&lt; FOO
Uploading: /&lt;img src=fail onerror=&quot;alert(&apos;the plugin processes the build log, and that needs to be considered untrusted&apos;);&quot;&gt;
FOO</command>
    </hudson.tasks.Shell>
  </builders>
  <publishers>
    <hudson.plugins.mavendeploymentlinker.MavenDeploymentLinkerRecorder plugin="[email protected]">
      <regexp></regexp>
    </hudson.plugins.mavendeploymentlinker.MavenDeploymentLinkerRecorder>
  </publishers>
  <buildWrappers/>
</project>

@bguerin
Copy link
Author

bguerin commented Nov 24, 2018

@daniel-beck added a java.net.URL parsing, if it fails, url will be escaped

@daniel-beck
Copy link
Member

To bypass the protection in 3e44fe1:

Building in workspace /…/workspace/JENKINS-54273
[JENKINS-54273] $ /bin/bash /var/folders/39/ggldtdps6034ct7d_y6x4_v80000gn/T/jenkins5535853518735171775.sh
Uploading: https://example.org#/<img src=fail onerror="alert('the plugin processes the build log, and that needs to be considered untrusted');">
Finished: SUCCESS

Unfortunately I'm out of time here, so will not be able to test further attempts to fix this.

FWIW the approach in the plugin appears to be flawed and too cumbersome to make work, it should instead just add the links from the raw data in Jelly, rather than to insert a blob of questionable HTML.

@bguerin
Copy link
Author

bguerin commented Nov 24, 2018

Agree with your point of view, I did not choose this way at first glance to avoid too many changes.
Will rework on it
Thanks for your time and help !

@bguerin
Copy link
Author

bguerin commented Nov 25, 2018

PR reworked as suggested by @daniel-beck , his last attack is now harmless

${it.latestDeployments.text}
<ul>
<j:forEach items="${it.latestDeployments.deployments}" var="deployment">
<li><a href="${deployment.url}">${deployment.name}</a></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not disallow URLs with javascript: scheme, so I expect there's still an XSS vulnerability here.

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.

2 participants