Security, sample application: Data validation issues

[marpuch]

There are some security issues with the provided sample application of OASP:

1. The data received from the client is always fully trusted.
2. The client gets always whole objects containing many attributes which are neither needed or displayed (which in combination with point 1 leads to an unnecessary big attack surface).

Some examples of what is possible:

Waiter and cook can do with the order actually everything. One can argue, that without a specification this kind of functionality is allowed, but still proper examples for developers could be helpful.

[marpuch]

I think that to deal with point 1 we need some validation routines. Good validation solves 100% of problems but well... it's a boring thing to implement and it's easy to make mistakes.

So reducing the attack surface area and a bit of defence in depth (point 2) could be a good idea. The client should usually never receive more data then necessary to perform its operations. And he should never send back more data then needed. The interface between the client and the server should be as thin as possible. Yet I have no good ideas how to code such thing in a sexy way. Any ideas?

[amarinso]

As you said, we should have an specification for the application. I think that the sample application has been growing with code-first style and that leads to incoherencies.

But validation recommendations are already in place on the platform guide (Bean Validation is the recommended way) and on some entry points of the sample application (save methods for Offer and Table management).

For your second concern I think that we should add more detail on the guide. Currently a "Service Transfer Object" is proposed if you need a different view on the data for the entities. But I coulnd't find an example of this on the restaurant application.

Also I would propose to name this "Service Transfer Objects" with the suffix Dto similar to the Eto or Cto.

[hohwille]

@marpuch :+1: thanks for sharing your findings and insight. IMHO this is simply a bug in the "use-case" to update an order-position. We need to verify that only fields are modified that are allowed to do so. That is the first thing to fix and is very easy to do so the vulnerability is solved.

Further, we should think about a general concept to avoid such mistakes. Options may be:

• separate "read-only" fields into a separate entity that is then never merged/updated with the persistence entity.
• Use annotations to mark properties as read-only, writable, or writeable only if a specific permission is present. This would imply some more "framework-like" code to support this. However, it will help to prevent security accidents as it lowers the effort and prevents manual mistakes. It would also be possible to enforce that always one of such annotations has to be present so that if the developer forgets to decide if something is fully writeable or restricted an error would be raised. The "framework code" can still be part of the sample and app template or could go to a module...

BTW: Editing the comment is officially fine and no vulnerability. Maybe the main issue here is that the current UI does not allow to properly edit the comment directly. There is lots to improve in the UI as well.

[hohwille]

FYI: We only copied the price to the order position as actually the price of the offer in the master-data can change over time and then the reporting, analysis and billing would be inconsistent. We could also solve this now with historization via envers that we have introduced by saving the revision of the offer instead. However, I still prefer the current data model as it is more efficient for analysis and also easier. So lets quickly fix the security hole and go further...

[hohwille]

Fix needs to go here: https://github.com/oasp/oasp4j/blob/develop/oasp4j-samples/oasp4j-sample-core/src/main/java/io/oasp/gastronomy/restaurant/salesmanagement/logic/impl/usecase/UcManageOrderPositionImpl.java#L112

[hohwille]

But I coulnd't find an example of this on the restaurant application.

I do not yet see an example where it would really make sense. Even in this case I would not use it.

Also I would propose to name this "Service Transfer Objects" with the suffix Dto similar to the Eto or Cto.

DTO stands for Data Transfer Object and also an ETO is a DTO in this manner. IMHO such objects should just carry the suffix TO, but I would rather not mind as long as we have a clear and documented guide.

[amarinso]

@hohwille would WTO (for Web Transfer Object) be better to describe it and to have a univoque name? I think TO is too generic and could cause confusion. And DTO as you said is the same concept and not enought explicit

[hohwille]

Jep. Good suggestion if we need a specific term for this WTO is perfect. BTW: we are getting out of scope for this issue... But feel free to add this to the guide on transfer-objects so it will not get lost.

[hohwille]

I do not see that the client receives too much data and many attributes are never used. Maybe the main problem is that the client is incomplete. We have a clear concept of exchanging business objects and having a RIA/SPA that can cache this data and performs logic to render the UI accordingly including transformations and so forth. Sending only minimal view objects per (sub-)dialog is more old-fashened and in the end not faster (then you can not reasonable address caching as you always need the data in the view for the current dialog). We send the data required for a use-case and this can be then performend on the client potentially in a series of dialogs. On the other side it is suggested/best practice to separate expensive (e.g. BLOB) or sensitive data into separate objects and do not transfer them or keep them separate.

[hohwille]

I hoped with pointing out the codeline that someone would fix this... However it seemed time for me to react on this to show that we care about security. IMHO the addressed errors are all fixed. To get sure we will need a new deployment and a re-test. I did some tidy up of strange or unused code as well. Also I had to fix test cases that we testing inconsistent scenarios (OMG). Finally the code around Bill (UcManageBillImpl) is most likely to have similar security bugs and this is also a very sensible and severe area. However, the bill stuff is IMHO crap in general and needs some revisiting also from the business perspective. In order to get this ticket closed we at least need to remove the bill stuff from the REST service or get it fixed.