Enforce Stream validity - Security Enhancement

[eyalitki]

Input parsing of incoming messages is extremely fragile, and once the pointer passed the buffer's end, the sanity check macros will return a huge size_t size instead of a negative value, thus bypassing all future checks in the code.

Change the checks to use ssize_t and upon every release/free make sure that the Stream didn't reach a corrupted state. This ensures that an attacker triggering a read/write out-of-bounds will terminate the program instead of allowing the attacker to continue their exploit.

Would have blocked CVE-2020-9497 (all 3 variants), CVE-2020-13396, CVE-2020-11526, CVE-2020-11088 and more.

[freerdp-bot]

Can one of the admins verify this patch?

[eyalitki]

Just adding that while this is my first commit to your project, I worked with you in the past regarding several security vulnerabilities that I found in the project in 2018 and later again in 2020. This commit aims to block a vulnerability class in FreeRDP, hoping to improve the robustness of the project.

[akallabeth]

@eyalitki nice idea, but I´d prefer the checks to stay internal to Stream_* functions. (e.g. check if the wStream* is actually valid is already done with WINPR_ASSERT, so it should be possible to add checks for s->pointer and s->buffer along with s->capacity without the need to modify the external API (e.g. return values)

(they are only as inline in header due to performance reasons)

[eyalitki]

Thanks for the fast response.

The reason I changed the return values from size_t to ssize_t instead of adding checks inside it, is that there are already plenty of code checks using Stream_GetRemainingLength and the current return value type damages their effectiveness (As described here #6012). My goal was to maximize the efficiency of the current checks, instead of adding a performance penalty from adding an additional check per invocation of these API functions.

The commit adds protection against very common vulnerabilities in this project as is seen from my experience of finding vulns in this project, and from vulnerabilities found by other researchers in recent years. The protection itself is divided into 2 lines of defense:

1. First Layer - Leverage existing code checks that use Stream_GetRemainingLength and enable them to catch a stream that already went out of bounds. In the current state, once some check was missing / used a wrong value, all the later checks will be pointless because the returned value is of type size_t and a negative value will be returned as a huge positive number, thus bypassing the input check. This is exactly the case pointed at in stream pointer out of bounds in update_recv_secondary_order could lead out of bounds read later #6012. This layer of defense has 0 performance impact, and it allows the code to use the existing error handling / logging, just as intended.
2. Second Layer - Once finished with a given stream, we verify that it is still valid and that no out-of-bound read/write operation occurred. Instead of updating all code places that have such logic (some of which are in other projects such as Apache Guacamole, which uses FreeRDP), the commit placed a check inside the "dtors" of the Stream. This check has a negligible performance impact as it adds a single check per Stream/message and it also defines a new error case which logs the error and aborts the program.

From an error handling perspective, it is easy to see that it is preferable to leverage the existing code checks & error handling instead of aborting the program for every stream that went out of bounds (assuming additional code checks are already in place and will be able to handle it gracefully). This is the goal of the first layer, and sadly the only way to achieve it is by changing the return values from size_t to ssize_t.

I'll just add that from a compatibility point of view, this API change doesn't break the source or binary level compatibility of the project. If there are additional considerations for keeping the API unchanged, I would be happy to learn about them so I would be able to check how can this protection be redesigned to fit the requirements (if possible).

Thanks again for your time.

[akallabeth]

@eyalitki I find your idea intriguing and have added a commit in #7322 that can handle such checks (it removes the API that can create broken internal pointers and validates the internal state on every access, when WINPR_ASSERT is available)

[eyalitki]

Seeing that #7322 handles the need to return ssize_t in the above functions, the remaining gap is out-of-bounds read/write without a following call to Stream_GetRemainingLength (A case we had in Apache Guacamole for instance - CVE-2020-9497).

If I'll update my PR to still include a verify() call in the Stream "dtors()" to catch such out-of-bounds read/writes, will it be OK?

[akallabeth]

@eyalitki yes, I´ve added it to #7322