From: pcarana Date: Thu, 24 Oct 2019 15:46:26 +0000 (-0500) Subject: Validate SignedObject DER encoding using incidences schema. X-Git-Tag: v1.2.0~65 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=892e37dd9907238b473f530090f4f7423beefecb;p=thirdparty%2FFORT-validator.git Validate SignedObject DER encoding using incidences schema. -asn1c doesn't have a DER decoder, but it has DER encoders. Once the data is BER decoded, encode it again as DER and compare against the original data, the difference (if there's one) will be at ASN1 TLVs. -Create new incidence 'incid-obj-not-der-encoded' to handle such error. -Update docs: RFC 6488 100% compliance, new incidence description. -Add new incidence to configuration example (examples/config.json). --- diff --git a/docs/incidence.md b/docs/incidence.md index 236ea96b..dc722ce1 100644 --- a/docs/incidence.md +++ b/docs/incidence.md @@ -10,6 +10,7 @@ title: Incidence 2. [`incidences` definition](#incidences-definition) 3. [Incidence types](#incidence-types) 1. [Signed Object's hash algorithm has NULL object as parameters](#signed-objects-hash-algorithm-has-null-object-as-parameters) + 2. [Object isn't DER encoded](#object-isnt-der-encoded) ## Introduction @@ -42,7 +43,7 @@ Some incidences are `ignore`d by default, because they stem from bad practices ( ## Incidence types -Presently, there is only one incidence type defined. This list is expected to grow when strict DER-parsing is implemented, and might also evolve further over time, depending on the state of the global RPKI and user demand. +Presently, there is only a pair of incidence types defined. This list might evolve further over time, depending on the state of the global RPKI and user demand. ### Signed Object's hash algorithm has NULL object as parameters @@ -86,3 +87,23 @@ If not `ignore`d, Fort will report this incidence with the following error messa ``` This only applies to digest parameters that have been defined as NULL objects; any other type of non-absent digest parameters will yield a different error message, and will therefore not be silenced. + +### Object isn't DER encoded + +- **Name:** `incid-obj-not-der-encoded` +- **Default action:** `ignore` + + +[RFC 6488](https://tools.ietf.org/html/rfc6488) mandates that all signed objects must be DER-encoded (see [section 3](https://tools.ietf.org/html/rfc6488#section-3)): + +``` + l. The signed object is DER encoded. +``` + +Altough this is mandatory, quite a few signed objects in the global RPKI ignore this rule and are simply BER-encoded. + +If not `ignore`d, Fort will report this incidence with the following error message: + +``` +: : '' isn't DER encoded +``` diff --git a/docs/intro-fort.md b/docs/intro-fort.md index d4449c54..34794c82 100644 --- a/docs/intro-fort.md +++ b/docs/intro-fort.md @@ -25,7 +25,7 @@ Further information can be found in the subsections below. | [6482](https://tools.ietf.org/html/rfc6482) (ROA) | 100% | | [6486](https://tools.ietf.org/html/rfc6486) (Manifests) | 75% | | [6487](https://tools.ietf.org/html/rfc6487) (Resource Certificates & CRLs) | 100% | -| [6488](https://tools.ietf.org/html/rfc6488) (Signed Objects) | 90% | +| [6488](https://tools.ietf.org/html/rfc6488) (Signed Objects) | 100% | | [6493](https://tools.ietf.org/html/rfc6493) (Ghostbusters) | 100% | | [6810](https://tools.ietf.org/html/rfc6810) (RTR Version 0) | 100% | | [7318](https://tools.ietf.org/html/rfc7318) (Policy Qualifiers) | 100% | @@ -53,12 +53,6 @@ This RFC states a bunch of rules that allow some level of tolerance to missing, These constitute the approximate missing 25%. -### RFC 6488 (Signed Objects) - -6488 mandates that all signed objects must be DER-encoded. Fort's current parser cannot tell the difference between DER and (its superset) BER. - -Unfortunately, the parser also currently unavoidably [rejects certain technically valid BER objects](https://github.com/vlm/asn1c/blob/master/skeletons/ber_decoder.c#L215-L221). (Although these are not valid DER.) - ### RFC 8182 (RRDP) RRDP is a protocol intended to replace RSYNC in the RPKI. Fort only implements RSYNC, currently. diff --git a/docs/usage.md b/docs/usage.md index 0dd65ff6..7c8e4b13 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -525,6 +525,10 @@ The configuration options are mostly the same as the ones from the `argv` interf { "name": "incid-hashalg-has-params", "action": "ignore" + }, + { + "name": "incid-obj-not-der-encoded", + "action": "ignore" } ], diff --git a/examples/config.json b/examples/config.json index 0c5d114d..6e1b7f1f 100644 --- a/examples/config.json +++ b/examples/config.json @@ -45,6 +45,10 @@ { "name": "incid-hashalg-has-params", "action": "ignore" + }, + { + "name": "incid-obj-not-der-encoded", + "action": "ignore" } ], "output": { diff --git a/man/fort.8 b/man/fort.8 index 79dcf5f0..5f18690e 100644 --- a/man/fort.8 +++ b/man/fort.8 @@ -125,6 +125,13 @@ of objects, each with two members "name" and "action", eg: .br "action": "warn" .RE +}, +{ +.RS 2 +"name": "incid-obj-not-der-encoded", +.br +"action": "error" +.RE } .RE ] @@ -147,10 +154,11 @@ nothing happened. happened. .RE .P -By default, all the incidences have an action of \fIignore\fR. Currently there's -only one registered incidence: +By default, all the incidences have an action of \fIignore\fR. Currently there +are two registered incidences: \fIincid-hashalg-has-params\fR (Signed Object's hash algorithm has NULL object as parameters). +\fIincid-obj-not-der-encoded\fR (Object isn't DER encoded). .P More information about incidences can be consulted at FORT's web docs. .RE @@ -628,6 +636,10 @@ to a specific value: { "name": "incid-hashalg-has-params", "action": "ignore" + }, + { + "name": "incid-obj-not-der-encoded", + "action": "ignore" } ], "output": { diff --git a/src/asn1/content_info.c b/src/asn1/content_info.c index 3dfb0403..4cf50223 100644 --- a/src/asn1/content_info.c +++ b/src/asn1/content_info.c @@ -33,8 +33,9 @@ decode(struct file_contents *fc, struct ContentInfo **result) struct ContentInfo *cinfo; int error; + /* Validate DER encoding rfc6488#section3 bullet 1.l */ error = asn1_decode_fc(fc, &asn_DEF_ContentInfo, (void **) &cinfo, - true); + true, true); if (error) return error; diff --git a/src/asn1/decode.c b/src/asn1/decode.c index a3ed7172..cc971ab4 100644 --- a/src/asn1/decode.c +++ b/src/asn1/decode.c @@ -4,6 +4,15 @@ #include "common.h" #include "log.h" +#define COND_LOG(log, pr) (log ? pr : -EINVAL) + +/* Decoded BER data */ +struct ber_data { + const void *src; + size_t src_size; + size_t consumed; +}; + static int validate(asn_TYPE_descriptor_t const *descriptor, void *result, bool log) { @@ -16,16 +25,77 @@ validate(asn_TYPE_descriptor_t const *descriptor, void *result, bool log) error = asn_check_constraints(descriptor, result, error_msg, &error_msg_size); if (error == -1) - return log ? - pr_err("Error validating ASN.1 object: %s", error_msg) : - -EINVAL; + return COND_LOG(log, + pr_err("Error validating ASN.1 object: %s", error_msg)); + + return 0; +} + +static void +der_debug(size_t size, const void *der, const void *orig, size_t consumed) +{ + size_t i; + + for (i = 0; i < size; i++) + if (((uint8_t *)der)[i] != ((uint8_t *)orig)[consumed + i]) { + pr_debug("Diff starts at byte %ld: BER '%02x', DER '%02x'", + consumed + i, + ((uint8_t *)der)[i], + ((uint8_t *)orig)[consumed + i]); + return; + } +} + +static int +der_coder(const void *buf, size_t size, void *app_key) +{ + struct ber_data *data = app_key; + + if (data->consumed + size > data->src_size) { + pr_debug("DER encoding will consume more bytes than expected (expected %lu, will get %lu)", + data->consumed + size, data->src_size); + return -1; + } + + if (memcmp(data->src + data->consumed, buf, size) != 0) { + if (log_debug_enabled()) + der_debug(size, buf, data->src, data->consumed); + return -1; + } + data->consumed += size; + return 0; +} + +static int +validate_der(size_t ber_consumed, asn_TYPE_descriptor_t const *descriptor, + const void *original, void *result) +{ + struct ber_data data; + asn_enc_rval_t eval; + + data.src = original; + data.src_size = ber_consumed; + data.consumed = 0; + + eval = der_encode(descriptor, result, der_coder, &data); + if (eval.encoded == -1) + return incidence(INID_OBJ_NOT_DER, + "'%s' isn't DER encoded", eval.failed_type->name); + + if (ber_consumed != eval.encoded) { + pr_debug("DER encoding consumed less bytes than expected (expected %lu, got %lu)", + ber_consumed, eval.encoded); + return incidence(INID_OBJ_NOT_DER, "'%s' isn't DER encoded", + descriptor->name); + } return 0; } int asn1_decode(const void *buffer, size_t buffer_size, - asn_TYPE_descriptor_t const *descriptor, void **result, bool log) + asn_TYPE_descriptor_t const *descriptor, void **result, bool log, + bool dec_as_der) { asn_dec_rval_t rval; int error; @@ -38,10 +108,19 @@ asn1_decode(const void *buffer, size_t buffer_size, /* Must free partial object according to API contracts. */ ASN_STRUCT_FREE(*descriptor, *result); /* We expect the data to be complete; RC_WMORE is an error. */ - return log ? + return COND_LOG(log, pr_err("Error '%u' decoding ASN.1 object around byte %zu", - rval.code, rval.consumed) : - -EINVAL; + rval.code, rval.consumed)); + } + + /* Validate DER encoding */ + if (dec_as_der) { + error = validate_der(rval.consumed, descriptor, buffer, + *result); + if (error) { + ASN_STRUCT_FREE(*descriptor, *result); + return error; + } } error = validate(descriptor, *result, log); @@ -55,16 +134,19 @@ asn1_decode(const void *buffer, size_t buffer_size, int asn1_decode_any(ANY_t *any, asn_TYPE_descriptor_t const *descriptor, - void **result, bool log) + void **result, bool log, bool dec_as_der) { - return asn1_decode(any->buf, any->size, descriptor, result, log); + return asn1_decode(any->buf, any->size, descriptor, result, log, + dec_as_der); } int asn1_decode_octet_string(OCTET_STRING_t *string, - asn_TYPE_descriptor_t const *descriptor, void **result, bool log) + asn_TYPE_descriptor_t const *descriptor, void **result, bool log, + bool dec_as_der) { - return asn1_decode(string->buf, string->size, descriptor, result, log); + return asn1_decode(string->buf, string->size, descriptor, result, log, + dec_as_der); } /* @@ -74,8 +156,9 @@ asn1_decode_octet_string(OCTET_STRING_t *string, */ int asn1_decode_fc(struct file_contents *fc, - asn_TYPE_descriptor_t const *descriptor, void **result, bool log) + asn_TYPE_descriptor_t const *descriptor, void **result, bool log, + bool dec_as_der) { return asn1_decode(fc->buffer, fc->buffer_size, descriptor, result, - log); + log, dec_as_der); } diff --git a/src/asn1/decode.h b/src/asn1/decode.h index ca5f6928..bd3a0241 100644 --- a/src/asn1/decode.h +++ b/src/asn1/decode.h @@ -7,11 +7,12 @@ #include "asn1/asn1c/constr_TYPE.h" int asn1_decode(const void *, size_t, asn_TYPE_descriptor_t const *, void **, + bool, bool); +int asn1_decode_any(ANY_t *, asn_TYPE_descriptor_t const *, void **, bool, bool); -int asn1_decode_any(ANY_t *, asn_TYPE_descriptor_t const *, void **, bool); int asn1_decode_octet_string(OCTET_STRING_t *, asn_TYPE_descriptor_t const *, - void **, bool); + void **, bool, bool); int asn1_decode_fc(struct file_contents *, asn_TYPE_descriptor_t const *, - void **, bool); + void **, bool, bool); #endif /* SRC_ASN1_DECODE_H_ */ diff --git a/src/asn1/signed_data.c b/src/asn1/signed_data.c index 5bf7bd86..b40eb31e 100644 --- a/src/asn1/signed_data.c +++ b/src/asn1/signed_data.c @@ -128,7 +128,7 @@ validate_content_type_attribute(CMSAttributeValue_t *value, int error; error = asn1_decode_any(value, &asn_DEF_OBJECT_IDENTIFIER, - (void **) &attrValues, true); + (void **) &attrValues, true, false); if (error) return error; eContentType = &eci->eContentType; @@ -151,7 +151,7 @@ validate_message_digest_attribute(CMSAttributeValue_t *value, return pr_err("There's no content being signed."); error = asn1_decode_any(value, &asn_DEF_MessageDigest, - (void **) &digest, true); + (void **) &digest, true, false); if (error) return error; @@ -404,7 +404,7 @@ signed_data_decode_pkcs7(ANY_t *coded, struct SignedData **result) int error; error = asn1_decode_any(coded, &asn_DEF_SignedDataPKCS7, - (void **) &sdata_pkcs7, true); + (void **) &sdata_pkcs7, true, false); if (error) return error; @@ -417,7 +417,7 @@ signed_data_decode_pkcs7(ANY_t *coded, struct SignedData **result) /* Parse content as OCTET STRING */ error = asn1_decode_any(sdata_pkcs7->encapContentInfo.eContent, &asn_DEF_ContentTypePKCS7, - (void **) &sdata->encapContentInfo.eContent, true); + (void **) &sdata->encapContentInfo.eContent, true, false); if (error) goto release_sdata; @@ -451,10 +451,8 @@ signed_data_decode(struct signed_data *sdata, ANY_t *coded) sdata->encoded = coded; - /* rfc6488#section-3.1.l */ - /* TODO (next iteration) this is BER, not guaranteed to be DER. */ error = asn1_decode_any(coded, &asn_DEF_SignedData, - (void **) &sdata->decoded, false); + (void **) &sdata->decoded, false, false); if (error) { /* Try to decode as PKCS content (RFC 5652 section 5.2.1) */ error = signed_data_decode_pkcs7(coded, &sdata->decoded); @@ -517,7 +515,7 @@ get_content_type_attr(struct SignedData *sdata, OBJECT_IDENTIFIER_t **result) return -EINVAL; return asn1_decode_any(attr->attrValues.list.array[0], &asn_DEF_OBJECT_IDENTIFIER, - (void **) result, true); + (void **) result, true, false); } } diff --git a/src/incidence/incidence.c b/src/incidence/incidence.c index 336206ec..74f8effa 100644 --- a/src/incidence/incidence.c +++ b/src/incidence/incidence.c @@ -23,6 +23,12 @@ static struct incidence incidences[__INID_MAX] = { "Signed Object's hash algorithm has NULL object as parameters", INAC_IGNORE, }, + { + INID_OBJ_NOT_DER, + "incid-obj-not-der-encoded", + "Object isn't DER encoded", + INAC_IGNORE, + }, }; static int @@ -64,6 +70,7 @@ init_action(json_t *json) enum incidence_action action; int error; + id = __INID_MAX; error = json_get_string(json, "name", &name); if (error) return error; diff --git a/src/incidence/incidence.h b/src/incidence/incidence.h index 819a48f5..3656a9c7 100644 --- a/src/incidence/incidence.h +++ b/src/incidence/incidence.h @@ -9,6 +9,7 @@ */ enum incidence_id { INID_HASHALG_HAS_PARAMS, + INID_OBJ_NOT_DER, __INID_MAX, }; diff --git a/src/object/certificate.c b/src/object/certificate.c index 02eb92b5..c941b18e 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -740,7 +740,7 @@ handle_ip_extension(X509_EXTENSION *ext, struct resources *resources) string = X509_EXTENSION_get_data(ext); error = asn1_decode(string->data, string->length, &asn_DEF_IPAddrBlocks, - (void **) &blocks, true); + (void **) &blocks, true, false); if (error) return error; @@ -788,7 +788,7 @@ handle_asn_extension(X509_EXTENSION *ext, struct resources *resources, string = X509_EXTENSION_get_data(ext); error = asn1_decode(string->data, string->length, - &asn_DEF_ASIdentifiers, (void **) &ids, true); + &asn_DEF_ASIdentifiers, (void **) &ids, true, false); if (error) return error; diff --git a/src/object/manifest.c b/src/object/manifest.c index af910170..02b7f274 100644 --- a/src/object/manifest.c +++ b/src/object/manifest.c @@ -22,7 +22,8 @@ decode_manifest(struct signed_object *sobj, struct Manifest **result) sobj->sdata.decoded->encapContentInfo.eContent, &asn_DEF_Manifest, (void **) result, - true + true, + false ); } diff --git a/src/object/roa.c b/src/object/roa.c index 55da9ead..4407f769 100644 --- a/src/object/roa.c +++ b/src/object/roa.c @@ -19,7 +19,8 @@ decode_roa(struct signed_object *sobj, struct RouteOriginAttestation **result) sobj->sdata.decoded->encapContentInfo.eContent, &asn_DEF_RouteOriginAttestation, (void **) result, - true + true, + false ); }