Add basic "security rules" #4298
Conversation
lrstewart
force-pushed
the
fips_comp_1
branch
6 times, most recently
from
17d7aea to
fc72153
Compare
lrstewart
force-pushed
the
fips_comp_1
branch
from
fc72153 to
7e403da
Compare
| S2N_RESULT (*validate_cipher_suite)(const struct s2n_cipher_suite *cipher_suite, bool *valid); | ||
| S2N_RESULT (*validate_sig_scheme)(const struct s2n_signature_scheme *sig_scheme, bool *valid); | ||
| S2N_RESULT (*validate_cert_sig_scheme)(const struct s2n_signature_scheme *sig_scheme, bool *valid); | ||
| S2N_RESULT (*validate_curve)(const struct s2n_ecc_named_curve *curve, bool *valid); |
There was a problem hiding this comment.
If we encounter a standard that requires us to check multiple policy fields at once, we could add a more generic
validate_policyfunction
I wonder if another option could be to just pass in the security policy to each of these validate functions. That way each function still returns whether or not a given item is valid, it just has the option to check other security policy fields to make that decision.
There was a problem hiding this comment.
Ultimately I want to use these for more than just security policies. For example, I want to check the actual security parameters chosen by the connection for FIPS compliance.
If we run into a rule that requires checking multiple policy fields, we can reconsider. But I suspect that won't be common, so I'd like to try to keep these simple.
There was a problem hiding this comment.
Like, if you have a validate_cipher_suite(cipher, is_valid) method, it's going to be doing pretty simple checks and we're probably fine with the common security rule code handling logging the error to output just based on "is_valid". If you're going to do more complex checks though, we'll want to pass the result object too so that you can do your own logging, and now your implementation of your rule is getting more complicated :)
There was a problem hiding this comment.
Do we think these callbacks are going to scale with more complex usecases? For example, let's say a ciphersuite should only be used with a specific chosen version of TLS. Or a ciphersuite and curve should be chosen together.
There was a problem hiding this comment.
No, but I don't think that's an issue.
Both your hypotheticals are only relevant to checking a connection / handshake, not a security policy. A security policy can't assume which version of TLS will be chosen (it can check the policy's min version, but that's not really sufficient) or guarantee that two options (a cipher suite and a curve) will be chosen together. Like I call out in the description, we'd probably want a validate_handshake(struct s2n_connection *conn, struct s2n_security_rule_result *result) method to cover cases like that when specifically checking the actual negotiated parameters instead of a security policy. We also don't have any cases for checks like that yet-- both FIPS and RFC5191 don't need that. And even if they did, they'd likely still rely heavily on much simpler checks that can be more clearly implemented with these simple callbacks.
| /* Stuffer versions of sprintf methods, except: | ||
| * - They write bytes, not strings. They do not write a final '\0'. Unfortunately, | ||
| * they do still require enough space for a final '\0'-- we'd have to reimplement | ||
| * sprintf to avoid that. | ||
| * - vprintf does not consume the vargs. It calls va_copy before using | ||
| * the varg argument, so can be called repeatedly with the same vargs. | ||
| */ | ||
| int s2n_stuffer_printf(struct s2n_stuffer *stuffer, const char *format, ...); | ||
| int s2n_stuffer_vprintf(struct s2n_stuffer *stuffer, const char *format, va_list vargs); |
There was a problem hiding this comment.
IIRC all of the stuffer functions have CBMC proofs. It's probably good to have some for these new functions as well.
There was a problem hiding this comment.
Definitely agree, but I think getting CBMC to work with varargs is going to be a bit of an adventure. I'd rather that didn't block this PR, particularly because the only direct manipulation of the stuffer is the "tainted" flag logic. Other than that, we just use existing stuffer functions.
| } s2n_security_flag; | ||
|
|
||
| struct s2n_security_rule_result { | ||
| bool found_error; |
There was a problem hiding this comment.
I would prefer to invert this logic and have something like bool is_valid, bool passed or bool success.
There was a problem hiding this comment.
The trick there is that we then need to initialize is_valid to "true" before calling any of our validation methods. It'll default to incorrectly start as "false". If we use the negative case, it starts out correct and methods just need to set it if they find an issue. Like, our checks don't ever definitively mark a policy as "valid", they only ever flag it as "not valid", and if they never flag it then it's valid.
|
|
||
| struct s2n_security_rule_result { | ||
| bool found_error; | ||
| bool write_output; |
There was a problem hiding this comment.
Is this field needed if you just key'ed on the stuffer being initialized or not?
There was a problem hiding this comment.
I started out doing that, but it's a little messy. Either we assume the stuffer is always growable and check "output->growable", or we need a more complex check like "!s2n_stuffer_is_freed(output) || output->growable".
I like the flag because it explicitly indicates that we expect output. If the stuffer isn't configured properly but the flag is set, we'll fail. Otherwise, if the stuffer isn't configured properly, we just skip output.
| S2N_RESULT (*validate_cipher_suite)(const struct s2n_cipher_suite *cipher_suite, bool *valid); | ||
| S2N_RESULT (*validate_sig_scheme)(const struct s2n_signature_scheme *sig_scheme, bool *valid); | ||
| S2N_RESULT (*validate_cert_sig_scheme)(const struct s2n_signature_scheme *sig_scheme, bool *valid); | ||
| S2N_RESULT (*validate_curve)(const struct s2n_ecc_named_curve *curve, bool *valid); |
There was a problem hiding this comment.
Do we think these callbacks are going to scale with more complex usecases? For example, let's say a ciphersuite should only be used with a specific chosen version of TLS. Or a ciphersuite and curve should be chosen together.
Resolved issues:
Related to #4299
Description of changes:
Adds a mechanism to enforce "security rules" on policies. This will let us programmatically check that a policy we think meets a standard like "perfect forward secrecy" or "FIPS" actually does meet that standard. I'm initially just supporting perfect forward secrecy because it's a very simple standard to check for.
I'm starting with a fairly simple set of common functions for a "security rule", but we can add more flexible ones as needed. For example, to enforce FIPS rules on which extensions are received, I plan to add a more generic
validate_handshake(struct s2n_connection *conn, struct s2n_security_rule_result *result)function. If we encounter a standard that requires us to check multiple policy fields at once, we could add a more genericvalidate_policy(struct s2n_security_policy *policy, struct s2n_security_rule_result *result)functionFor detailed logging of why a policy doesn't meet the standard, I'm just using raw text in a stuffer. I think that's the most flexible option-- we can allocate a growable stuffer or limit the memory we're willing to allocate for reporting.
Call-outs:
This intentionally does not:
Testing:
New unit tests.
I've also done the next task (enforcing security rules in s2n_init and unit tests) and it behaves as expected.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.