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

ComboBox doesn't allow a separate description so render html entities or be vulnerable to Javascript Injection #8731

Closed
xenoterracide opened this Issue Mar 2, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@xenoterracide
Copy link

xenoterracide commented Mar 2, 2017

using vaadin 7.7.6
using example https://vaadin.com/docs/-/part/framework/components/components-combobox.html but with malicious text that assumes humans are adding the planet names via a form.

// List of planets
List<Planet> planets = new ArrayList<>();
planets.add(new Planet(1, "<iframe width="560" height="315" src="https://www.youtube.com/embed/dQw4w9WgXcQ?autoplay=1&rel=0" frameborder="0" allowfullscreen></iframe>"));


ComboBox<Planet> select =
    new ComboBox<>("Select or Add a Planet");
select.setItems(planets);
select.setDescription(""); // doesn't do anything

the combobox will "rick roll" you, or you can input <div onmouseenter="alert('hello')">:Goog</div>, and get the hello alert. Obviously these are both innocuous, but more dangerous exploits could be written. If you encode it first converting < to &lt; then the list itself will have the ugly &lt; which isn't what you want to show for <E> Corp (Mr. Robot :D). The best way to fix this issue is to have an ItemDescriptionGenerator to allow a separate tooltip to be generated from the contents of the list.

@xenoterracide xenoterracide changed the title ComboBox doesn't allow a separate description so render html entities or be vulnerable to xss ComboBox doesn't allow a separate description so render html entities or be vulnerable to Javascript Injection Mar 2, 2017

@xenoterracide

This comment has been minimized.

Copy link
Author

xenoterracide commented Mar 2, 2017

alse see #8310

@Legioth

This comment has been minimized.

Copy link
Member

Legioth commented Mar 6, 2017

Slightly related to #2132 about allowing ComboBox items to be interpreted as HTML.

I think this was kind-of fixed in Framework 8 since all tooltips should now by be rendered as plain text (and if not, then that's a specific bug that should be fixed asap), which means that the item and the tooltip will be rendered in the same way.

@xenoterracide

This comment has been minimized.

Copy link
Author

xenoterracide commented Jun 22, 2017

This vulnerability still exists in 7.7.9

@ilatypov

This comment has been minimized.

Copy link

ilatypov commented Jun 23, 2017

The argument of the planets.add statement contains an iframe bringing up a Youtube video. This does not agree with the div tag referred to by the description. I failed to reproduce this at demo.vaadin.com, and I guess this is because the demo site runs the latest version (major number 8) of the product.

@xenoterracide

This comment has been minimized.

Copy link
Author

xenoterracide commented Jun 23, 2017

does not agree with the div tag referred to by the description.

ah sorry about that, I must have forgot to update the description when I decided to use the iframe (because the rick roll amuses me), however, both strings could be inserted at that position and then would show up in that text. I updated the text to be clearer.

However, I am not sure if this impacts v8, it sounds like it doesn't but I haven't tested that.

@tyrel

This comment has been minimized.

Copy link

tyrel commented Jul 5, 2017

Why is this known vulnerability still just sitting here open, not even targeted?

@Legioth

This comment has been minimized.

Copy link
Member

Legioth commented Jul 13, 2017

Seems like I read this ticket description carelessly the last time around and assumed that this was an inconsistency related to tooltips, which is apparently not the case since ComboBox doesn't show tooltips for individual items.

I wasn't able to reproduce any kind of vulnerability with variations of the code shown in this ticket or in the referred documentation. All I managed to achieve was an ugly popup: screen shot 2017-07-13 at 14 32 52

The regular item description that is shown in the popup is escaped in Framework 8 and Framework 7. There might of course still be some other part of the code that causes something similar, but nothing that was obvious from looking at the component's implementation.

Maybe some part of the application uses the selected item description to update a Label with ContentMode.HTML or to set the value as the tooltip of some component using setDescription(String)?

Could someone share a full code example that actually causes a ComboBox item description to be interpreted as HTML?

I would furthermore like to point out that the original description is invalid: it refers to Vaadin Framework 7.7.6, but the ComboBox class in that version doesn't have any type parameter and doesn't have a setItems method. Both those features were used in the example code but weren't introduced until 8.0.

@xenoterracide

This comment has been minimized.

Copy link
Author

xenoterracide commented Jul 17, 2017

so... after working through to produce a full example, yes no setItems this is more or less what we're doing... except we're doing it through convoluted subclasses. This may not be a vulnerability (well any more than most of vaadin's descriptions supporting unsafe tooltips has been, and the only way to make this safe is to sanitize it ourselves. The Value change listener calling setDescription is the key here

@SpringBootApplication
public class Application {


    public static void main( final String... args ) {
        SpringApplication.run( Application.class, args );
    }

    public static class Planet {
        private final long id;
        private final String name;

        public Planet( final long id, final String name ) {
            this.id = id;
            this.name = name;
        }

        public String getName() {
            return name;
        }

        public long getId() {
            return id;
        }
    }

    @SpringUI
    @Theme( "reindeer" )
    public static class VaadinUI extends UI {

        private static final long serialVersionUID = -7843692311814677343L;

        @Override
        protected void init( VaadinRequest request ) {

            List<Planet> planets = Arrays.asList(
                    new Planet( 1, "<script>alert('hello')</script>" ),
                    new Planet( 2, "<a href=\"\" onmouseenter=\"alert('hello')\"/>hello</a>" )
            );

            BeanContainer<Object, Planet> container = new BeanContainer<>( Planet.class );
            container.setBeanIdProperty( "id" );
            container.addAll( planets );

            ComboBox comboBox = new ComboBox();
            comboBox.setWidth( 100, Unit.PIXELS );
            comboBox.setImmediate( true );
            comboBox.setEnabled( true );
            comboBox.setItemCaptionMode( AbstractSelect.ItemCaptionMode.PROPERTY );
            comboBox.setItemCaptionPropertyId( "name" );
            comboBox.setContainerDataSource( container );
            comboBox.addValueChangeListener( event -> {
                long id = (long) event.getProperty().getValue();
                setDescription( comboBox.getItemCaption( id ) );
            });
            setContent( comboBox );
        }
    }
}

if you need the maven pom I can supply that

@Legioth

This comment has been minimized.

Copy link
Member

Legioth commented Jul 17, 2017

setDescription is explicitly documented to treat the contents as HTML instead of plain text in Vaadin Framework 7.x. The original reason for this potentially unsafe approach is that the tooltip is expected to contain formatted text that is part of the application's implementation. We have since then realized that developers will occasionally also show user-provided data in tooltips without proper escaping.

Starting from version 8.0, the default has been changed so that setDescription(String) automatically escapes the contents. We won't introduce this change in the 7.x series since that would break applications that use application-provided formatted contents in tooltips.

In this case, you can thus update to 8.x or explicitly escape the value passed to setDescription in the value change handler. When using Spring, there's a handy org.springframework.web.util.HtmlUtils.htmlEscape method that you can use, e.g. setDescription( HtmlUtils.htmlEscape( comboBox.getItemCaption( id ) ) );.

For those not using Spring, there are also similar helpers in e.g. Apache Commons and Guava. The JSoup library that is bundled with the framework also provides Jsoup.clean() that removes any dangerous HTML instead of escaping it.

@Legioth Legioth closed this Jul 17, 2017

@Legioth Legioth added this to the Invalid milestone Jul 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment