javaee / mojarra

PLEASE NOTE: This project has moved to Eclipse Foundation and will be archived under the JavaEE GitHub Organization. After Feb. 1, 2021, the new location will be github.com/javaee/mojarra. Mojarra - Oracle's implementation of the JavaServer Faces specification
https://github.com/eclipse-ee4j/mojarra
Other
164 stars 58 forks source link

Partial rendering: insufficient CDATA encoding (XSS) #4370

Open cnsgithub opened 6 years ago

cnsgithub commented 6 years ago

1) Environment

2) Expected behavior

Using CDATA end tag ]]> within submitted ajax strings must not break things when included in partial response.

3) Actual behavior

Client side partial response processing gets aborted. This issue may lead to partial response (XML) injection/XSS as seen in https://github.com/primefaces/primefaces/issues/3623 and reported in https://github.com/primefaces/primefaces/issues/3629 also. This issue was also mentioned in https://github.com/javaserverfaces/mojarra/issues/3042 but has not yet been addressed.

4) XHTML

        <h:form id="form">
            <h:commandButton value="Submit" action="#{bean.submit}">
                <f:ajax event="click" />
            </h:commandButton>
            <h:panelGroup id="panel" />
        </h:form>

5) Bean

@ManagedBean
public class Bean {

    public void submit() {
        FacesContext ctx = FacesContext.getCurrentInstance();
        ExternalContext extContext = ctx.getExternalContext();
        if (ctx.getPartialViewContext().isAjaxRequest()) {
            try {
                extContext.setResponseContentType("text/xml");
                extContext.addResponseHeader("Cache-Control", "no-cache");
                PartialResponseWriter writer = ctx.getPartialViewContext().getPartialResponseWriter();
                writer.startDocument();
                writer.startUpdate("form:panel");
                writer.startElement("span", null);
                writer.writeAttribute("id", "form:panel", null);
                writer.startElement("script", null);
                writer.writeText("alert('User input: " + escapeJavascript(getUserInput()) + "')", null);
                writer.endElement("script");
                writer.endElement("span");
                writer.endUpdate();
                writer.endDocument();
                writer.flush();
                ctx.responseComplete();
            }
            catch (Exception e) {
                throw new FacesException(e);
            }
        }
    }

    private String getUserInput() {
        return "foo]]>bar";
    }

    private String escapeJavascript(String string) {
        // would use OWASP javascript escaping for example
        // no need to escape ]]> in javascript strings, so it remains unchanged
        return string;
    }
}

5) Misc

cnsgithub commented 6 years ago

As this might be a security risk, it should be addressed with higher priority. Just ignoring it again seems not to be the right solution.

Btw, you may have a look at owasp's solution to get an idea how to fix it: https://github.com/OWASP/owasp-java-encoder/blob/master/core/src/main/java/org/owasp/encoder/CDATAEncoder.java

marcodelpercio commented 5 years ago

This is quite critical for us as well. Several composite components and PrimeFaces components have CDATA sections with scripts and when such components are being re-rendered due to an ajax partial update everything breaks. We are on Mojarra 2.3.5.SP2 which comes with the very latest WildFly 14.0.1

cnsgithub commented 5 years ago

Please follow https://github.com/eclipse-ee4j/mojarra/issues/4392 at EE4J since mojarra has moved there.

trinidadmarcelo commented 5 years ago

This is quite critical for us as well. Several composite components and PrimeFaces components have CDATA sections with scripts and when such components are being re-rendered due to an ajax partial update everything breaks. We are on Mojarra 2.3.5.SP2 which comes with the very latest WildFly 14.0.1

Hi @marcodelpercio,

I have the same problem with Wildfly 14 that implements jsf 2.3.5, have you found a solution?

Thanks

cnsgithub commented 5 years ago

@trinidadmarcelo Would it be an option to replace the JSF implementation in WildFly modules with MyFaces? Things seem to get fixed faster there... And it does not suffer from this issue at all.

marcodelpercio commented 5 years ago

@trinidadmarcelo I do actually have a workaround. It's a simple WebFilter which intercepts CDATA comments inside AJAX requests/partial requests and replaces the wrong cdata comment with a working one.

It's a bit sad that this issue hasn't been fixed in the last 5 months. I am still hoping for an official solution from the Mojarra team.

@cnsgithub Have you actually tried going MyFaces in WildFly? Did it work?

CDataFix.java.txt

cnsgithub commented 5 years ago

@cnsgithub Have you actually tried going MyFaces in WildFly? Did it work?

Not yet, sorry. Thanks for sharing your workaround with the community.

trinidadmarcelo commented 5 years ago

@trinidadmarcelo Would it be an option to replace the JSF implementation in WildFly modules with MyFaces? Things seem to get fixed faster there... And it does not suffer from this issue at all.

I have replaced with the jsf impl 2.3.2 inside modules of Wildfly 14, the jsf spec also must be replaced and it works, the problem is since jsf impl 2.3.5 that implements since Wildfly 14

trinidadmarcelo commented 5 years ago

@trinidadmarcelo I do actually have a workaround. It's a simple WebFilter which intercepts CDATA comments inside AJAX requests/partial requests and replaces the wrong cdata comment with a working one.

It's a bit sad that this issue hasn't been fixed in the last 5 months. I am still hoping for an official solution from the Mojarra team.

@cnsgithub Have you actually tried going MyFaces in WildFly? Did it work?

CDataFix.java.txt

@marcodelpercio as a quick solution using Wildfly 14 with no modifications in his jsf modules, I replaced all javascript CDATA with the XHTML escape codes, hope Mojarra team fix it asap.

erickdeoliveiraleal commented 5 years ago

@trinidadmarcelo I do actually have a workaround. It's a simple WebFilter which intercepts CDATA comments inside AJAX requests/partial requests and replaces the wrong cdata comment with a working one.

It's a bit sad that this issue hasn't been fixed in the last 5 months. I am still hoping for an official solution from the Mojarra team.

@cnsgithub Have you actually tried going MyFaces in WildFly? Did it work?

CDataFix.java.txt

I installed MyFaces sucessfully, using multi-jsf-installer bundled in wildfly sources. But it just create the folder if using linux, I did it and after I could use on Windows. I did it with version 16. I attached the zip here (it contains mojarra 2.2.16 and myfaces 2.3.3 as modules) , you just extract it in modules root folder. And in standalone.xml you modify the line of jsf this way <subsystem xmlns="urn:jboss:domain:jsf:1.1" default-jsf-impl-slot="myfaces-2.3.3"/> com.zip

abelet commented 5 years ago

It is quite similar to https://github.com/eclipse-ee4j/mojarra/issues/4488. Unfortunately mojarra not just buggy, but after examining the implementation, the code quality seems to be very poor also.

kguelzau commented 4 years ago

Since this is fixed in the 2.3 branch, could this be backported to 2.2?