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

Mutated Chain with Trust Parameter #73

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions proto/asn1-pdu/mutated_x509_chain.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// Copyright 2020 Google Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
////////////////////////////////////////////////////////////////////////////////

// This protobuf represents an X.509 certificate chain with a set of operations
// to mutate the chain to create structural relationships between the
// certificates.

syntax = "proto2";

import "x509_certificate.proto";

package x509_certificate;

message MutatedChain {
// An X.509 Certificate Chain comprises a list of certificates.
repeated x509_certificate.X509Certificate chain = 1;
repeated TrustParameter trust_parameters = 2;
Copy link

Choose a reason for hiding this comment

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

Sorry, I was out of the office on vacation, which means I've definitely forgotten more than I remember, but to try and recap:

  • We talked about how 'trusted' isn't really a property of the DER, it's more a property of the target API being fuzzed
    • That is, the target API has a notion of "trusted" or not, as well as things like purpose, hostname to be verified, etc
  • This is why it doesn't belong as a property of the X509Certificate; it's not really an encoding option
  • Because it's not a property of the X509Certificate, it also doesn't lend itself to being in mutations`
    • This is because when we apply a Mutation, what we're doing is structurally modifying one (or more) X509Certificates in chain to expressing something in the proto schema that is intended to match the ASN.1

Does this match your memory as well?

What I'm wondering here is whether we should split further, based on separating out the "encoding" (which may fail to decode on the target-under-fuzzing side) and the API parameters. I think we talked a little about this in your final week, about the tension between target-specific API parameters (e.g. BoringSSL vs NSS API parameters) and those generic to RFC 5280, but that are still not encoding parameters (such as the set of trust anchors, or trusted policy OIDs, etc).

Concretely, I'm curious for your thoughts on

message MutatedChain {
  repeated x509_certificate.X509Certificate chain = 1;
  repeated Mutation mutations = 2;
}

message TrustedChain {
  MutatedChain chain = 1;
  repeated TrustedCertificate certs = 2;
}

message TrustedCertificate {
  required CertIndex index = 1;
}

I'm not 100% sold on this; it might be coupling the Protobuf layout too much to how the code would be structured, so I'm curious your thoughts. But the reason for that proposal above is:

  1. MutatedChain only handles "things expressed via DER"
  2. TrustedChain (for lack of a better naming) contains the set of generic-to-all-libraries API parameters, like those talked about in RFC 5280, Section 6.1.1 (handwave handwave)
  3. We let TrustedCertificate act as all the trust parameter-specific configuration (6.1.1 (d) from RFC 5280)

The reason I'm not entirely sold is I'm trying to work backwards, from a "here's specifically how a target will use this" (e.g. BoringSSL, NSS), and then trying to make sure we have an API that makes sense for that, and if we have an API that makes sense, that we have the Protobufs that make sense for it. That is, from a design perspective, we "work backwards" to work out the design, but since I'm reviewing it, I'm having to "work forward" (first the protobuf, then the API, then the target integration). So there's a lot of state to keep in mind, and I haven't paged it all in, but wanted to share my initial thoughts.

Note: Not asking you to make changes yet, especially as I page in all the reviews, but wanted to share initial thoughts. If we do keep it as-is, which could be perfectly fine, I think we want to add documentation here that talks about a "Here's how you use this" that also reflects how we'll extend it, so we've got a bit of a coherent design.

repeated Mutation mutations = 3;
}

message TrustParameter {
// Allow certificate to be trusted or not-trusted.
required bool trusted = 1;
Copy link

Choose a reason for hiding this comment

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

With the design I mentioned above, I think we can omit trusted here, since its mere preference implies a certificate is trusted.

That said, with your current API, I think it's not unreasonable to keep, so that the fuzzer won't just make every cert trusted :)

// |index| represents the position of the certificate that will be mutated.
required CertIndex index = 2;
}

// Mutate the certificate chain protobuf to force structural relationships
// between the certificates.
message Mutation {
oneof types {
MutateSignature mutate_signature = 1;
}
}

message MutateSignature {
// Allow certificate to be valid or invalid for fuzzing.
required bool valid = 1;
// |index| represents the position of the certificate that will be mutated.
required CertIndex index = 2;
}

// Contains the positions of the certiciate chain that will be mutated.
enum CertIndex {
CERT_0 = 0;
CERT_1 = 1;
CERT_2 = 2;
CERT_3 = 3;
CERT_4 = 4;
Copy link

Choose a reason for hiding this comment

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

Suggested change
CERT_4 = 4;
CERT_4 = 4;
CERT_5 = 5;
CERT_6 = 6;
CERT_7 = 7;
CERT_8 = 8;
CERT_9 = 9;

"10 certs should be enough for anyone" - Bill Gates

}
106 changes: 106 additions & 0 deletions proto/asn1-pdu/mutated_x509_chain_to_der.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
// Copyright 2020 Google Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
////////////////////////////////////////////////////////////////////////////////

#include "mutated_x509_chain_to_der.h"
#include "x509_certificate_to_der.h"
Copy link

Choose a reason for hiding this comment

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

This include goes to line 24, per https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes (that is, it's number 8 on that list, while mutated_x509_chain_to_der is number 1)


#include <google/protobuf/text_format.h>
Copy link

Choose a reason for hiding this comment

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

This moves to line 24 as well; this is number 7 on https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes (so after the system headers, before your headers)


#include <stdlib.h>
#include <utility>

namespace x509_certificate {

Copy link

Choose a reason for hiding this comment

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

Suggested change
namespace {

We should move all the 'utility' functions (those not part of the header) within an unnamed namespace, to prevent the symbols from triggering the One Definition Rule

That is, it's a nested unnamed namespace within the x509_certificate namespace.

This is discussed in the Google style guide at https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Static_Variables

// DER encodes each |certificate| in |chain| and returns
// the encoded certificates in |der|.
Comment on lines +27 to +28
Copy link

Choose a reason for hiding this comment

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

Suggested change
// DER encodes each |certificate| in |chain| and returns
// the encoded certificates in |der|.
// Encode |chain| to DER.
// The returned vector will be the same length as |chain|, and contain
// a DER-encoded sequence of bytes representing each |certificate|
// in |chain|.

That's perhaps too wordy, but things that stand out to me with this comment:

  1. I'm not sure if it's wrapped correctly
  2. I think we can split the "What it does" and "what it returns" into two complete sentences.
  3. Returns the encoded certificates in |der| isn't quite right. We can just strike in |der|, because that's a variable name, but we're talking about a function return.
  4. "DER encodes" feels a little... off. We have DER-encoded, or we encode to DER, but "DER encodes"... just feels weird.

None of this is really major, and this probably feels like nitpicky, but offering suggestions to explore if there is a better wording.

std::vector<std::vector<uint8_t>> EncodeChain(
const google::protobuf::RepeatedPtrField<X509Certificate>& chain) {
std::vector<std::vector<uint8_t>> der;

for (const auto& cert : chain) {
der.push_back(X509CertificateToDER(cert));
}

return der;
}

void SetTrust(
Copy link

Choose a reason for hiding this comment

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

Suggested change
void SetTrust(
void ApplyTrustParameters(

Handwavy naming, but "SetTrust" felt a little less descriptive.

std::vector<X509>& encoded_mutated_chain,
google::protobuf::RepeatedPtrField<TrustParameter>& trust_parameters) {
for (const auto& trust_parameter : trust_parameters) {
if (trust_parameter.index() >= encoded_mutated_chain.size()) {
continue;
}

encoded_mutated_chain[trust_parameter.index()].trusted =
trust_parameter.trusted();
}
}

void Mutate(const MutateSignature& mutation,
Copy link

Choose a reason for hiding this comment

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

Suggested change
void Mutate(const MutateSignature& mutation,
void MutateSignature(const MutateSignature& mutation,

Combined with my suggestion below, trying to make it explicitly clear in the code that these are different mutations that are happening; that is, trying not to use overloads.

google::protobuf::RepeatedPtrField<X509Certificate>& chain) {
// An |operation| on a certiciate cannot be executed if the |index| is greater
// than or euqal to the |size| of the certificate chain.
Comment on lines +55 to +56
Copy link

Choose a reason for hiding this comment

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

There's a typo here (euqal), but I'm wondering whether this is something we can abstract, given that you repeat a similar check on line 44.

For example:

// Validates that |index| is a valid index into a certificate chain that
// is |chain_size| certificates in length.
// Mutations reference certificates in the chain, but the fuzzer may generate
// indices that are out of range (e.g. a chain of length 2, but reference a
// mutation to happen at index 3). This should be called anytime a CertIndex
// is referenced, to validate that it is within bounds.
bool IsValidIndex(const CertIndex& index, size_t chain_size) {
  return index < chain_size;
}

Then that would let you more meaningfully annotate the code, in that line 44
and line 57 become

if (!IsValidIndex(mutation.index(), chain.size()) {
  return;  // Or continue, for line 45
}

if (mutation.index() >= chain.size()) {
return;
}

auto* signature_value = chain[mutation.index()].mutable_signature_value();
signature_value->clear_pdu();
signature_value->mutable_value()->set_unused_bits(
asn1_universal_types::UnusedBits::VAL0);
// Represent a valid signature value with 1 and invalid with 0.
if (mutation.valid()) {
signature_value->mutable_value()->set_val("1");
} else {
signature_value->mutable_value()->set_val("0");
}
}

void Mutate(const Mutation& mutation,
Copy link

Choose a reason for hiding this comment

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

Suggested change
void Mutate(const Mutation& mutation,
void ApplyMutation(const Mutation& mutation,

google::protobuf::RepeatedPtrField<X509Certificate>& chain) {
switch (mutation.types_case()) {
case Mutation::TypesCase::kMutateSignature:
Mutate(mutation.mutate_signature(), chain);
break;
Comment on lines +77 to +78
Copy link

Choose a reason for hiding this comment

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

Suggested change
Mutate(mutation.mutate_signature(), chain);
break;
return Mutate(mutation.mutate_signature(), chain);

Both functions return void, and certainly, returning "void" probably seems odd, but it allows the code to be a little more self-documenting that nothing can happen after the case.

case Mutation::TypesCase::TYPES_NOT_SET:
return;
}
}

Copy link

Choose a reason for hiding this comment

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

Suggested change
} // namespace

This is where the unnamed namespace would end, since only lines 84 - 104 are relevant.

std::vector<X509> MutatedChainToDER(const MutatedChain& mutated_chain) {
auto chain = mutated_chain.chain();
auto trust_parameters = mutated_chain.trust_parameters();
auto mutations = mutated_chain.mutations();

if (chain.empty()) {
return {{}};
}

for (const auto& mutation : mutations) {
Mutate(mutation, chain);
}

std::vector<X509> encoded_mutated_chain;
for (const auto& encoded_cert : EncodeChain(chain)) {
encoded_mutated_chain.push_back({encoded_cert, false});
}
SetTrust(encoded_mutated_chain, trust_parameters);
Comment on lines +85 to +101
Copy link

Choose a reason for hiding this comment

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

Suggested change
auto chain = mutated_chain.chain();
auto trust_parameters = mutated_chain.trust_parameters();
auto mutations = mutated_chain.mutations();
if (chain.empty()) {
return {{}};
}
for (const auto& mutation : mutations) {
Mutate(mutation, chain);
}
std::vector<X509> encoded_mutated_chain;
for (const auto& encoded_cert : EncodeChain(chain)) {
encoded_mutated_chain.push_back({encoded_cert, false});
}
SetTrust(encoded_mutated_chain, trust_parameters);
std::vector<X509> encoded_chain;
// Copy the chain, to allow for local modification
auto chain = mutated_chain.chain();
if (chain.empty()) {
return encoded_chain;
}
// Apply all pending mutations to the local copy.
for (const auto& mutation : mutated_chain.mutations()) {
ApplyMutation(mutation, chain);
}
// Encode the local copy to DER.
for (const auto& encoded_cert : EncodeChain(chain)) {
encoded_chain.push_back({encoded_cert, false});
}
// Update trust settings.
ApplyTrustParameters(encoded_chain, mutated_chain.trust_parameters());

Mostly, I'm restructuring according to https://google.github.io/styleguide/cppguide.html#Local_Variables , to try to declare it before it's used, as well as avoid unnecessary locals, especially when they may end up copying fields (which I don't believe is what you intended, except for chain).


return encoded_mutated_chain;
}

} // namespace x509_certificate
38 changes: 38 additions & 0 deletions proto/asn1-pdu/mutated_x509_chain_to_der.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright 2020 Google Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
////////////////////////////////////////////////////////////////////////////////

#ifndef PROTO_ASN1_PDU_MUTATE_X509_CHAIN_H_
#define PROTO_ASN1_PDU_MUTATE_X509_CHAIN_H_

#include <stdint.h>

#include <vector>

#include "mutated_x509_chain.pb.h"

namespace x509_certificate {

struct X509 {
Copy link

Choose a reason for hiding this comment

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

Suggested change
struct X509 {
struct FuzzedCertificate {

I don't recall if we used that name yet? Just thought it might be a better name? Definitely one of the hard problems of computer science, so take what you will.

My concern with "X509" is that it isn't that descriptive, especially with the numbers, and so trying to find something easier for folks to comprehend. Of course, this may change in time.

std::vector<uint8_t> cert;
bool trusted;
};

// Applies |operations| to |x509_chain| and returns the DER encoded chain.
std::vector<X509> MutatedChainToDER(const MutatedChain& mutated_chain);

} // namespace x509_certificate

#endif // PROTO_ASN1_PDU_X509_CERTIFICATE_TO_DER_H_