CVE-2021-33609: fix: Add row limit to DataCommunicator row data requests by TatuLund · Pull Request #12415 · vaadin/framework

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

Conversation 19 Commits 6 Checks 0 Files changed

Conversation

OlliTietavainenVaadin pushed a commit to OlliTietavainenVaadin/framework that referenced this pull request

Oct 5, 2021

* Add row limit to DataCommunicator row data requests

* Add missing constant

* Add unit test

* Add test for extending Grid

* Fixed test

TatuLund added a commit that referenced this pull request

Oct 5, 2021

* Improve thread safety (#12395)

See: https://vaadin.com/forum/thread/17522264/concurrentmodificationexception-in-vaadin-shared-on-karaf-4-2-x

* Fix incompatible selenium version in test module. (#12397)

* Fixed a dependency version in a karaf test module. (#12399)

* Checkstyle tweaks to DateField widgets. (#12400)

• Added and updated JavaDocs.
• Updated comments.
• Updated to use non-deprecated method calls.
• Removed unnecessary warning suppressions.
• Suppressed warnings for unavoidable deprecation.

* fix: set Vaadin session attribute using lock in reinitializeSession (#12401)

* Cherry picked unit test from Flow

See vaadin/flow#11538

* Fix missing import

* Cherry pick fix from Flow

* deprecate vaadin-snasphots repo (#12405)

* deprecate vaadin-snasphots repo

* Update chrome version to 93

* add more screenshots

* fix: Add MPR UI id request parameter (#12412)

* fix: Add MPR UI id request parameter

Related-to vaadin/multiplatform-runtime#85

* test: Remove redundant non-empty param test

* test: Remove leftovers

* fix: Init window.mprUiId earlier than window.vaadin

* Add missing ‘=’

* Update links shown by license checker (#12402)

vaadin.com/pro does no longer have the info

* fix: Add row limit to DataCommunicator row data requests (#12415)

* Add row limit to DataCommunicator row data requests

* Add missing constant

* Add unit test

* Add test for extending Grid

* Fixed test

Co-authored-by: Tatu Lund [email protected] Co-authored-by: Anna Koskinen [email protected] Co-authored-by: Zhe Sun [email protected] Co-authored-by: Mikhail Shabarov [email protected]

Same issue here with TwinColSelect component. We do need to use more than 500 rows, is there anyway to override this?

Same issue here with TwinColSelect component. We do need to use more than 500 rows, is there anyway to override this?

@gbougiakas yes, check the GridTest.java in this very pull request

Same issue here with TwinColSelect component. We do need to use more than 500 rows, is there anyway to override this?

@gbougiakas yes, check the GridTest.java in this very pull request

Thanks for the reply, I checked GridTest.java and Grid appears to have a constructor that takes a DataCommunicator as an argument. TwinColSelect does not have such a constructor. I need to override the setItems method but it’s not obvious to me of what I should do in there.

Can you help me?

Create a new DataCommunicator subclass like in GridTest. Override getDataCommunicator in your TwinColSelect subclass and return an instance of your custom DataCommunicator.

Copy link

Contributor Author

****TatuLund** commented Nov 2, 2021**

With TwinColSelect it looks like this

    public class MyDataCommunicator extends DataCommunicator<String> {

        @Override
        protected int getMaximumAllowedRows() {
            return 1000; // Return here limit that fits your application better than the default
        }
    }
    
    public class MyTwinSelect extends TwinColSelect<String> {
        private MyDataCommunicator dataCommunicator;

        public MyTwinSelect() {
            dataCommunicator = new MyDataCommunicator();
            addExtension(dataCommunicator);
        }
        
        @Override
        public DataCommunicator<String> getDataCommunicator() {
            return dataCommunicator;
        }
    }

Create a new DataCommunicator subclass like in GridTest. Override getDataCommunicator in your TwinColSelect subclass and return an instance of your custom DataCommunicator.

With TwinColSelect it looks like this

    public class MyDataCommunicator extends DataCommunicator<String> {

        @Override
        protected int getMaximumAllowedRows() {
            return 1000; // Return here limit that fits your application better than the default
        }
    }
    
    public class MyTwinSelect extends TwinColSelect<String> {
        private MyDataCommunicator dataCommunicator;

        public MyTwinSelect() {
            dataCommunicator = new MyDataCommunicator();
            addExtension(dataCommunicator);
        }
        
        @Override
        public DataCommunicator<String> getDataCommunicator() {
            return dataCommunicator;
        }
    }

Thank you both for the swift replies. Here is my custom TwinColSelect

`
public class TwinColSelectWithCustomRowLimit extends TwinColSelect {

private DataCommunicatorWithCustomRowLimit<T> dataCommunicatorWithCustomRowLimit;

public TwinColSelectWithCustomRowLimit() {
    dataCommunicatorWithCustomRowLimit = new DataCommunicatorWithCustomRowLimit<T>();
    addExtension(dataCommunicatorWithCustomRowLimit);
}

@Override
public DataCommunicator<T> getDataCommunicator() {
    return dataCommunicatorWithCustomRowLimit;
}

public class DataCommunicatorWithCustomRowLimit<T> extends DataCommunicator<T> {
    @Override
    protected int getMaximumAllowedRows() {
        return 2000;
    }
}

}

`

Unfortunately now I get a NullPointerException upon calling the constructor of the TwinColSelectWithCustomRowLimit.

Any further help is much appreciated.

Seems like the the overriden getDataCommunicator() is called before the constructor for some reason, hence the custom DataCommunicator instance is null.

Copy link

Contributor Author

****TatuLund** commented Nov 2, 2021**

I, see some components are trickier to extend. I opened PR for easier configurability

#12466

I, see some components are trickier to extend. I opened PR for easier configurability

#12466

Thanks for the swift response. I will remain at 8.14.0 until there is an easy way to bypass this for the components we need more than 500 rows in our dataset.

Regards

As I’ve just commented in #12428, this limit to 500 items in ListSelect, TwinColSelect, ComboBox broke potentially a lot of code without proper warning that a 8.14.1 made a breaking change to previously legitimate and working code.

We have potentially many places were such crash may happen with backend data providers of unknown size.
Having to manually made hacky overrides to (what limit size ?) throughout the application is quite a bad idea.

Could the CVE be solved with proper way, without breaking code working fine for many years ?

Copy link

Contributor Author

****TatuLund** commented Nov 3, 2021**

Could the CVE be solved with proper way, without breaking code working fine for many years ?

No, that is not the good approach in this case.

We want to follow the same principle here as we have for XSS for example. By default our components do not allow HTML content (say Label, tooltip etc.), and you need to specifically with the API define that HTML content is allowed. I.e. then application developer takes the responsibility to take care that HTML content does not possess harm. Here we are working analogous way. By default framework limits the size, but you can override it. See the #12466, we will introduce API take makes the override less hacky.

I don’t see relation to XSS here.
Until 8.14.1 it was totally fine to have e.g. ListSelect with 501 items.
Now it is not possible without explicitly allowing that.
And why actually 500 ? Why not 100 or 1000 ?

And even if there is such limit (which I still thing is a bad idea), it needs to be properly documented in all the possible places.
And a big warning in release notes.

Copy link

Contributor Author

****TatuLund** commented Nov 3, 2021**

@TatuLund Thanks for the release notes adjustment, but again I’m not sure this is sufficient.
Most of the Vaadin application developers don’t even know there exists a DataComunicator, it’s rather low-level framework class.
I suggest to notify framework users that any UI component subclassing AbstractListing (i.e. ComboBoxes, ListSelects, Grids …) have now this limitation.

Copy link

Contributor Author

****TatuLund** commented Nov 3, 2021**

@mletenay We will update some documentation when #12466 is released. After that the following will be possible

@Theme("mytheme")
public class MyUI extends UI {

    @Override
    protected void init(VaadinRequest vaadinRequest) {
        final VerticalLayout layout = new VerticalLayout();
        
        TwinColSelect<String> twin = new TwinColSelect<>();
        twin.getDataCommunicator().setMaximumAllowedRows(1000);

        ComboBox<String> combo = new ComboBox<>();
        combo.getDataCommunicator().setMaximumAllowedRows(1000);
        
        List<String> items = new ArrayList<>();
        for (int i=0;i<800;i++) {
            items.add("Item "+i);
        }
        
        twin.setItems(items);
        twin.setItemCaptionGenerator(item -> item.toUpperCase());
        combo.setItems(items);
        combo.setScrollToSelectedItem(true);
        
        layout.addComponents(twin, combo);
        
        setContent(layout);
    }

    @WebServlet(urlPatterns = "/*", name = "MyUIServlet", asyncSupported = true)
    @VaadinServletConfiguration(ui = MyUI.class, productionMode = false)
    public static class MyUIServlet extends VaadinServlet {
    }
}

We also ran in this problem and I understand the point of @mletenay very well. We and our customers have some ComboBos with a lot of entries and we disabled pagination because our users did not see easily how to go down to the next page (no real scrollbar, buttons nearly unvisible). However even with pagination there would be the problem if I understood #12428 correctly.

However I do not see why it was not fixed without breaking the behavior at least for ListDataProvider (or maybe other InMemoryDataProvider). In a ListDataProvider the server knows exactly how many Items there are at most and could set this number of items automatically. When do you plan to release the fix to specify the allowed rowes?

Copy link

Contributor Author

****TatuLund** commented Nov 24, 2021**

Vaadin 8.14.3 was released today with the API to override the limit if so needed.

I’m sorry but this fix is stupid. It’s not fixing the core problem of the attack but makes life for developers harder. So now we have to estimate a number which could be possible. And if the number is big enough we reenabled the weakness to be attackable.

We have combo boxes in our application, where we do not really have the size under control.
A user can create new data and a distinct set is shown in the combobox.
We expected, that paging would solve the problem, but only a page-size ~ 100 will work (the client fetches the rows 40-460 in a second step)
so we decide to increase maximum allowed rows to 5000 (configurable via system property)