Fix cross-site scripting vulnerability in Archiva.

Hi,

Implementation proposal:

I seem to have noticed the lack of field validation in most of the input forms. I will start by strengthening the field validation for those that are vulnerable to XSS exploits. Also I will be altering some JSP output tags, since some of them uses struts2 output tags that does not escape the injected scripts. The jsp native output function c:out would escape injected scripts.

Validation messages/notifications would be in property(.properties) files.

Any thoughts on this proposal??
Comments & suggestions, would be much appreciated.

On second thought on my previous comment regarding the validation messages/notifications would be in a property file, I'll just follow the current validation set-up(validation messages/notifications would be in the action class), seeing that there is already some that are implemented.

Did a lot of testing with my initial small implementation(single action class), it seems that struts2's validation framework is having a problem dealing with trailing white-spaces. The trailing white-spaces are not being included in checking for the regular expression pattern.

Hi Marc, your proposal to tighten the field validation looks good. If you're having problems with the built-in regex field validator, I think you can implement your own validator class. You need to configure it in the validators.xml though before you can use it in your action validation

That seems like overkill - what fields require a non-standard type of validation? Even a regex seems like more than is needed for most fields.

What's the whitespace problem?

@Brett:

The white-space problem:

• Struts 2's validation framework truncates trailing white-spaces & does not include them(the trailing white-spaces) in the pattern matching process(but the data with the trailing white-spaces is carried over to the action class). So my solution would be allow, white-spaces.

@Brett Porter: here is a sneak peek of a single action class' validator.xml:

 
<validators>
  <field name="groupId">
    <field-validator type="requiredstring">
      <message>You must enter a groupId.</message>
    </field-validator>
    <field-validator type="regex">
        <param name="expression">^([a-zA-Z0-9.-]|s)+$</param>
        <message>groupId must only contain alphanumeric characters, white-spaces, dot(.) characters, and dash(-) characters.</message>
    </field-validator>
  </field>
  <field name="artifactId">
    <field-validator type="requiredstring">
      <message>You must enter an artifactId.</message>
    </field-validator>
    <field-validator type="regex">
        <param name="expression">^([a-zA-Z0-9.-]|s)+$</param>
        <message>artifactId must only contain alphanumeric characters, white-spaces, dot(.) characters, and dash(-) characters.</message>
    </field-validator>
  </field>
  <field name="version">
    <field-validator type="requiredstring">
      <message>You must enter a version.</message>
    </field-validator>
  </field>
  <field name="repositoryId">
    <field-validator type="regex">
        <param name="expression">^([a-zA-Z0-9.!-]|s)+$</param>
        <message>repositoryId must only contain alphanumeric characters, white-spaces, exclamation point(!) characters, dot(.) characters, and dash(-) characters.</message>
    </field-validator>
  </field>
</validators>

I can't think of a valid reason to allow trailing whitespace - we should be trimming the values. Whitespace isn't allowed in any of the above.

You'll need to make sure the regexes selected are appropriate for each field - and take note on what happens to existing fields that don't match.

@Brett:

OK, will find a way to deal with this validation problem(white-spaces) of struts2.

That's not what I'm saying...

There's no reason on any of our fields to allow a trailing whitespace. The validation doesn't need to deal with that - we should deal with it in trimming. The fields you've listed shouldn't allow whitespace at all. Which I point out to say that as you make a patch, make sure you capture the actual requirements of the fields.

Patch(with Unit test) for the prevention of cross-site scripting vulnerability in Archiva.

This patch is a composition of previous attachment(MRM-1468) + selenium scripts.

Thanks for the patch Marc!

Can you also add selenium tests for the following XSS scenarios that was reported in Archiva so that we can make sure that they're addressed by the fixes?

1. http://127.0.0.1:8080/archiva/deleteArtifact!doDelete.action?groupId=1<script>alert('xss')</script>&artifactId=1<script>alert('xss')</script>&version=1&repositoryId=internal
2. http://127.0.0.1:8080/archiva/admin/addLegacyArtifactPath!commit.action?legacyArtifactPath.path=test%3Cscript%3Ealert%28%27xss%27%29%3C%2Fscript%3E&groupId=test%3Cscript%3Ealert%28%27xss%27%29%3C%2Fscript%3E&artifactId=test%3Cscript%3Ealert%28%27xss%27%29%3C%2Fscript%3E&version=test%3Cscript%3Ealert%28%27xss%27%29%3C%2Fscript%3E&classifier=test%3Cscript%3Ealert%28%27xss%27%29%3C%2Fscript%3E&type=test%3Cscript%3Ealert%28%27xss%27%29%3C%2Fscript%3E
3. http://127.0.0.1:8080/archiva/admin/deleteNetworkProxy!confirm.action?proxyid=test%3Cscript%3Ealert%28%27xss%27%29%3C/script%3E

—
Stored (Persistent) XSS:

The exploit is executed in multiple pages. I have highlighted the input page and 1 page on which the code gets executed.

1. Stored XSS in Add Repository. Use the following exploit code for PoC:

Exploit Code: test"><script>alert('xss')</script>
Input URL: http://127.0.0.1:8080/archiva/admin/addRepository.action [Input fields: Identifier:repository.id, Name:repository.name, Directory:repository.location, Index Directory:repository.indexDir] Rendered On: http://127.0.0.1:8080/archiva/admin/confirmDeleteRepository.action?repoid=test

2. Stored XSS in Edit Appearance

Exploit Code: test<script>alert('xss')</script> Input URL: http://127.0.0.1:8080/archiva/admin/editAppearance.action [Input fields: Name:organisationName, URL:organisation:URL, LogoURL:organisation:URL] Rendered On: http://127.0.0.1:8080/archiva/admin/configureAppearance.action

3. Stored XSS in Add Legacy Artifact Path Exploit Code: test<script>alert('xss')</script> Input Page:

http://127.0.0.1:8080/archiva/admin/addLegacyArtifactPath.action [Input Fields: Path:name=legacyArtifactPath.path, GroupId:groupId, ArtifactId:artifactId, Version:version, Classifier:classifier, Type:type] Rendered On: http://127.0.0.1:8080/archiva/admin/legacyArtifactPath.action

4. Stored XSS in Add Network Proxy

Exploit Code: test<script>alert('xss')</script> Input Page: http://127.0.0.1:8080/archiva/admin/addNetworkProxy.action [Input Fields: Identifier:proxy.id, Protocol:proxy.protocol, Hostname:proxy.host, Port:proxy.port, Username:proxy.username] Rendered On: http://127.0.0.1:8080/archiva/admin/networkProxies.action

[Removed duplicate comment posted]

@Maria Odea Ching: ok, will make the selenium scripts for the xxs cases.

MRM-1468-2.patch contains additional fix on some unnoticed XSS vulnerability findings.

• Added selenium scripts for the possible XSS test cases.
• Refactored some of the redundant codes that appear in the new additions & the in the old remnants.
• Moved a couple of methods to a higher heirarchy for common inheritance use.
• Added some new classes.

Hi Marc, I was able to apply your patch locally and also ran the selenium tests successfully. Sorry I missed the following in the previous review of the patch, but can you make the following changes to the fix:

• include '\' as accepted value for proxy username
• use Struts2 URL validator to validate URLs

Thanks!

@Maria Odea Ching: ok

@Maria Odea Ching: Attached is the latest patch(MRM-1468-3.patch) with the requested addition/changes

Thanks again for the patches Marc! I've already applied it to 1.3.x branch -r1098897.

Fix merged to trunk in -r1099425 with additional changes:

• added a factory for action validator manager since ObjectFactory.setObjectFactory(..) was removed in struts 2.1 so the unit tests for validation would work
• fixed conflicts

Fixed failing selenium test in trunk after merge in -r1099671.

Hi,

Issue can still be reproducible when adding a managed repository through xmlrpc.

Attaching screenshot in my local.

Thanks

screenshot in local when adding a repository through xmlrpc with <script>alert("hello");</script> as my arguments.

Re-opening issue based on Gwen's comments. Thanks for the catch!

Added validation when adding a managed repository via xmlrpc + unit tests in Archiva trunk -r1100956.