ber.go: Protocol Validation and Defense #2
Conversation
This commit attempts to address possible `panic` conditions due to access outside of the bounds of the slice. There are specifically three changes: * A couple log messages are commented out due to them attempting to do an out of bounds view into the slice. * Existing protocol validation was fixed. There was an attempt to guard against these conditions, however in many cases there was an off by one error, or other conditions had not been considered. * A PR from the parent fork which attempts to bring in a protocol fix for lengths between `0x80` and `0xFF` was also included as part of this: mozilla-services#68 The logic for slice access has been reviewed carefully in an attempt to make sure we are defensive, but still allowing the nuance of the PKCS7 structure. In addition extensive fuzzing has been conducted on these changes.
jentfoo
force-pushed
the
jent/protocol_defense
branch
from
a254857 to
7af100e
Compare
| {[]byte{0x30, 0x85}, "ber2der: cannot move offset forward, end of ber data reached"}, | ||
| {[]byte{0x30, 0x85, 0x00}, "tag length too long"}, |
There was a problem hiding this comment.
I haven't taken the time to dig into the ber format and grok the changes above. However, I'd say as long as we have test cases that caused the panic before, but no longer do, I'm happy. Is []byte{0x30, 0x85, 0x00} sufficient for this, or should we include other inputs?
There was a problem hiding this comment.
Short answer, no. Though this was considered some.
Right now I have that coverage in Teleport's fuzzing: https://github.com/gravitational/teleport/pull/29231/files#diff-9356f3c834eb7d4e79f4bce10f28cb7f959930c03fcbe02cecd44323173dd676
I considered moving the fuzzing coverage to this repo, but I feel like Teleport is a better place for it due to how we have it setup with oss-fuzz, as well as being consistent with how we are testing other libraries.
I can add additional test cases for each of the corrected panic's rather than deferring to Teleport's testing. That is probably a good recommendation.
I did a look around for other pkcs7 packages, and found that our kube agent updater already depends on github.com/digitorus/pkcs7 (indirectly via github.com/sigstore/cosign). This looks like a fork off the mozilla-services project that is more actively maintained, we could consider switching to that fork and submitting patches there |
Interesting, I did a little searching myself but I did not stumble across that one. I will go ahead and close this PR until I can finish investigating their fork. If it does not work I will reopen it with the testing @wadells recommended. Thank you! |
* pass VerifyOptions instead of one cert pool Signed-off-by: Meredith Lancaster <[email protected]> * add eku usage to test Signed-off-by: Meredith Lancaster <[email protected]> * add new method for non breaking changes Signed-off-by: Meredith Lancaster <[email protected]> * add default EKU settings Signed-off-by: Meredith Lancaster <[email protected]> * verifySignatureAtTime should be used Signed-off-by: Meredith Lancaster <[email protected]> Signed-off-by: Meredith Lancaster <[email protected]>
* Refactor verification to handle passing multiple cert pools (#2) * pass VerifyOptions instead of one cert pool Signed-off-by: Meredith Lancaster <[email protected]> * add eku usage to test Signed-off-by: Meredith Lancaster <[email protected]> * add new method for non breaking changes Signed-off-by: Meredith Lancaster <[email protected]> * add default EKU settings Signed-off-by: Meredith Lancaster <[email protected]> * verifySignatureAtTime should be used Signed-off-by: Meredith Lancaster <[email protected]> Signed-off-by: Meredith Lancaster <[email protected]> * remove print statements made during testing Signed-off-by: Meredith Lancaster <[email protected]> * fix tests that were accidentally updated Signed-off-by: Meredith Lancaster <[email protected]> * comment out use of more insecure algorithms Signed-off-by: Meredith Lancaster <[email protected]> * use GODEBUG so tests can run with sha1 algorithm Signed-off-by: Meredith Lancaster <[email protected]> * add sha1 algorithms back Signed-off-by: Meredith Lancaster <[email protected]> * update comment Signed-off-by: Meredith Lancaster <[email protected]> * Cleanup tests (#3) * remove print statements made during testing Signed-off-by: Meredith Lancaster <[email protected]> * comment out use of more insecure algorithms Signed-off-by: Meredith Lancaster <[email protected]> * use GODEBUG so tests can run with sha1 algorithm Signed-off-by: Meredith Lancaster <[email protected]> * add sha1 algorithms back Signed-off-by: Meredith Lancaster <[email protected]> * update comment Signed-off-by: Meredith Lancaster <[email protected]> Signed-off-by: Meredith Lancaster <[email protected]> --------- Signed-off-by: Meredith Lancaster <[email protected]>
|
Reopening this PR. I would like to get this reviewed so that we can get a fix in for Teleport 14. I have a PR with the recently active digitorus fork: digitorus#12. Once that is merged the plan is to switch to that fork. If the above PR is not merged by early next week I would like to go ahead and merge this so we can get a fix in for Teleport that does not require a @wadells The tests have been expanded to make sure we have coverage on these changes. |
|
It looks like I could not re-open, so the PR has been recreated here: #3 |
This commit attempts to address possible
panicconditions due to access outside of the bounds of the slice. There are specifically three changes:0x80and0xFFwas also included as part of this: fix BER tag length check when length is between 0x80 and 0xFF mozilla-services/pkcs7#68The logic for slice access has been reviewed carefully in an attempt to make sure we are defensive, but still allowing the nuance of the PKCS7 structure. In addition extensive fuzzing has been conducted on these changes.
These changes are motivated to fix this issue: https://github.com/gravitational/teleport-private/issues/572
Without using a
deferrecoveras originally proposed: gravitational/teleport#29231This was also submitted to the parent fork: mozilla-services#81 (however there is no expectation they will merge it)