Skip to content
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

Add a security consideration about partial messages #728

Merged
merged 1 commit into from Feb 22, 2021

Conversation

tfpauly
Copy link
Contributor

@tfpauly tfpauly commented Jan 29, 2021

Closes #717

@mwelzl
Copy link
Contributor

mwelzl commented Jan 29, 2021

If I understood this right:
https://en.wikipedia.org/wiki/Security_of_Transport_Layer_Security#Truncation_attack
...and if this is even a reasonable source ... then what this attack does, is to take the TCP connection away underneath the application's feet. Then, an application may still expect parts of a message but they may in fact never arrive because the Connection is in fact gone.

Doesn't this mean that this exploits a detachment between TCP and a protocol atop TCP, which doesn't inform the application of the arrival of a FIN? In this case, I don't understand the concern raised by @squarooticus in #717: the delivery of "outstanding partial messages" means that the receiver has them in the buffer, and then a FIN arrives - upon reception of the FIN, these "outstanding partial messages" should be delivered. That seems right to me.

I may easily be completely off track! But either way, @gorryfair also noted in #717 that the attack should be described, so I don't think that this PR satisfies the issue as it stands.

@squarooticus
Copy link
Collaborator

A contrived example of the problem is if you intend to execute "rm -rf /some/path" and it somehow gets truncated to "rm -rf /". Protocols need to be designed in such a way that they deal properly with partial messages. I think the proposed text is fine.

@mwelzl
Copy link
Contributor

mwelzl commented Jan 29, 2021

Aha! That's completely different than my reading of the Wikipedia page. Then that's fine, I also agree.

@gorryfair
Copy link
Contributor

My 2c: You can always design bad apps: I don't think transports should be designed to do application data checks... applications should do that on the data they send.

@gorryfair
Copy link
Contributor

The new text seems to say what it should, although I don't see what this is an "attack", it would happen naturally when communications breaks.

@gorryfair gorryfair closed this Jan 29, 2021
@squarooticus
Copy link
Collaborator

Maybe we should discuss this. I see the need for something here as twofold:

  1. Warn users that they should not ignore the API-enabled distinction between full and partial messages.
  2. Make sure implementors properly classify partial messages as partial, so truncated messages (whether resulting from an attack or not) are presented to the application in a way that it can respond appropriately.

Specifically because TAPS provides message and layer abstractions (for security and framing), it's imperative that the implementor of each stage of the pipeline preserve the partial/full message distinction because unlike applications built on top of TCP sockets the application author may not have the ability to detect a partial message because windowing/framing at the lower layers aren't exposed to the application.

Based on this conversation, it sounds like we do need the text to make this a bit clearer.

@squarooticus squarooticus reopened this Jan 29, 2021
@squarooticus squarooticus self-requested a review January 29, 2021 15:20
@tfpauly
Copy link
Contributor Author

tfpauly commented Jan 29, 2021

I think this is useful to keep in. Security considerations are always good to point out.

Happy to take suggestions for clarity.


Applications should also take care to not assume that all data received using the Transport Services API is always
complete or well-formed. Specifically, messages that are received partially {{receive-partial}} could be a source
of truncation attacks if applications do not distinguish between partial messages and complete messages.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
of truncation attacks if applications do not distinguish between partial messages and complete messages.
of truncation attacks if applications do not distinguish between partial messages and complete messages.
The Transport Services system MUST NOT deliver partial data without correctly marking it as partial.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@squarooticus does this work?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

fallback or re-establishment is supported in the Transport Services system.

Applications should also take care to not assume that all data received using the Transport Services API is always
complete or well-formed. Specifically, messages that are received partially {{receive-partial}} could be a source
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarify that complete receives are complete.

Copy link
Contributor

@mwelzl mwelzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM


Applications should also take care to not assume that all data received using the Transport Services API is always
complete or well-formed. Specifically, messages that are received partially {{receive-partial}} could be a source
of truncation attacks if applications do not distinguish between partial messages and complete messages.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@philsbln philsbln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT with @tfpauly's additions

@mwelzl mwelzl merged commit 498c08b into master Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Partial final message
5 participants