-
Notifications
You must be signed in to change notification settings - Fork 751
Description
Problem:
The documentation says:
S2N_ERR_T_PROTO, /* Failure in some part of the TLS protocol. Ex: CBC verification failure */ S2N_ERR_T_INTERNAL, /* Error internal to s2n-tls. A precondition could have failed. */
however in reality a lot of places S2N_ERR_T_INTERNAL is used where the failure is driven by TLS protocol error , which should use S2N_ERR_T_PROTO instead. For example:
- encrypted blob smaller than IV will trigger
S2N_ERR_SAFETY - Incorrect payload length will trigger
S2N_ERR_SAFETY - binder size over what was actually provided will trigger
S2N_ERR_SAFETY - Some cases where client provide less data than expected results in
S2N_ERR_STUFFER_OUT_OF_DATA. - 2 alerts in a row will trigger
S2N_ERR_ALERT_PRESENT, which is treated as internal error.
These are just a few examples, but there are a lot more - pretty much every parser uses some of the safety macro which will result in S2N_ERR_SAFETY. This prevents S2N_ERR_T_INTERNAL to be used for actual internal error monitoring which may help catch bugs like #2967
Solution:
We can introduce a new set of macro to be used when parsing inputs and we can audit the existing code which parses TLS to replace existing macro with new one. The challenging part though is how do we ensure that we don't regress on this - we can potentially utilize fuzzing for that, but better ideas are welcome.
- Does this change what S2N sends over the wire? No
- Does this change any public APIs? No
- Which versions of TLS will this impact? ALL
Requirements / Acceptance Criteria:
No S2N_ERR_T_INTERNAL can be triggered by TLS traffic.