From deef7b7823f21914b17838f152a8bd510a348f54 Mon Sep 17 00:00:00 2001 From: Alberto Leiva Popper Date: Tue, 14 May 2024 19:11:19 -0600 Subject: [PATCH] Remove the DER validator rfc6488#3.1.l states we need to check "the signed object is DER encoded." But that's not what this code was doing. First, the validation was only kicking in specifically during the decoding of the ContentInfo, which is just the outermost layer of the signed object. Second, the validation was incorrect. This seems to be the intended algorithm in pseudocode: boolean is_der_encoded(original_bytes): der_bytes = der_encode(ber_decode(original_bytes)); return (original_bytes equal der_bytes); This is what the code was actually doing: boolean is_der_encoded(original_bytes): der_bytes = der_encode(ber_decode(original_bytes)); return (original_bytes.length equals der_bytes.length); These two quirks made the validation mostly a no-op. There's also the issue that this implementation seems inefficient, especially since Fort doesn't need to DER-encode anywhere else. By checking the encoding while parsing, I would save a lot of memory in addition to being able to delete that mess of encoding functions. But I'm going to have to push that to the future. This is growing more ambitious than I can afford during a release review, and given that the code wasn't really doing anything productive in the first place, I'm not losing much by simply axing it for now. --- docs/incidence.md | 15 +------- src/asn1/content_info.c | 5 ++- src/asn1/decode.c | 79 ++++------------------------------------ src/asn1/decode.h | 7 ++-- src/asn1/signed_data.c | 12 +++--- src/object/certificate.c | 4 +- src/object/manifest.c | 3 +- src/object/roa.c | 3 +- 8 files changed, 24 insertions(+), 104 deletions(-) diff --git a/docs/incidence.md b/docs/incidence.md index 4fd56c0f..e389ea8d 100644 --- a/docs/incidence.md +++ b/docs/incidence.md @@ -98,20 +98,7 @@ This only applies to digest parameters that have been defined as NULL objects; a - **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 at the validation log: - -``` -: : '' isn't DER encoded -``` +Deprecated. `incid-obj-not-der-encoded` does nothing as of Fort 1.6.2. ### File listed at manifest doesn't exist diff --git a/src/asn1/content_info.c b/src/asn1/content_info.c index 1aacb793..fa6900bb 100644 --- a/src/asn1/content_info.c +++ b/src/asn1/content_info.c @@ -32,12 +32,13 @@ 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; + /* TODO (asn1c) rfc6488#3.1.l: Validate DER encoding */ + error = validate(cinfo); if (error) { content_info_free(cinfo); diff --git a/src/asn1/decode.c b/src/asn1/decode.c index b416e339..7b0a2bec 100644 --- a/src/asn1/decode.c +++ b/src/asn1/decode.c @@ -32,58 +32,9 @@ validate(asn_TYPE_descriptor_t const *descriptor, void *result, bool log) return 0; } -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_val_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) - return -1; - - data->consumed += size; - return 0; -} - -/* - * TODO (performance) This isn't efficient, consider implement DER decoding - * or something better. - */ -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 = (unsigned char *) 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_val_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, - bool dec_as_der) + asn_TYPE_descriptor_t const *descriptor, void **result, bool log) { asn_codec_ctx_t s_codec_ctx; asn_dec_rval_t rval; @@ -103,17 +54,6 @@ asn1_decode(const void *buffer, size_t buffer_size, rval.code, rval.consumed)); } - /* Validate DER encoding, only if wanted and incidence isn't ignored */ - if (dec_as_der && - incidence_get_action(INID_OBJ_NOT_DER) != INAC_IGNORE) { - error = validate_der(rval.consumed, descriptor, buffer, - *result); - if (error) { - ASN_STRUCT_FREE(*descriptor, *result); - return error; - } - } - error = validate(descriptor, *result, log); if (error) { ASN_STRUCT_FREE(*descriptor, *result); @@ -125,19 +65,16 @@ 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, bool dec_as_der) + void **result, bool log) { - return asn1_decode(any->buf, any->size, descriptor, result, log, - dec_as_der); + return asn1_decode(any->buf, any->size, descriptor, result, log); } int asn1_decode_octet_string(OCTET_STRING_t *string, - asn_TYPE_descriptor_t const *descriptor, void **result, bool log, - bool dec_as_der) + asn_TYPE_descriptor_t const *descriptor, void **result, bool log) { - return asn1_decode(string->buf, string->size, descriptor, result, log, - dec_as_der); + return asn1_decode(string->buf, string->size, descriptor, result, log); } /* @@ -147,9 +84,7 @@ 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, - bool dec_as_der) + asn_TYPE_descriptor_t const *descriptor, void **result, bool log) { - return asn1_decode(fc->buffer, fc->buffer_size, descriptor, result, - log, dec_as_der); + return asn1_decode(fc->buffer, fc->buffer_size, descriptor, result, log); } diff --git a/src/asn1/decode.h b/src/asn1/decode.h index 977742e5..217a4fb1 100644 --- a/src/asn1/decode.h +++ b/src/asn1/decode.h @@ -6,12 +6,11 @@ #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, bool); + void **, bool); int asn1_decode_fc(struct file_contents *, asn_TYPE_descriptor_t const *, - void **, bool, bool); + void **, bool); #endif /* SRC_ASN1_DECODE_H_ */ diff --git a/src/asn1/signed_data.c b/src/asn1/signed_data.c index 0fbaf89b..8dd1bcff 100644 --- a/src/asn1/signed_data.c +++ b/src/asn1/signed_data.c @@ -126,7 +126,7 @@ validate_content_type_attribute(CMSAttributeValue_t *value, int error; error = asn1_decode_any(value, &asn_DEF_OBJECT_IDENTIFIER, - (void **) &attrValues, true, false); + (void **) &attrValues, true); if (error) return error; eContentType = &eci->eContentType; @@ -149,7 +149,7 @@ validate_message_digest_attribute(CMSAttributeValue_t *value, return pr_val_err("There's no content being signed."); error = asn1_decode_any(value, &asn_DEF_MessageDigest, - (void **) &digest, true, false); + (void **) &digest, true); if (error) return error; @@ -406,7 +406,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, false); + (void **) &sdata_pkcs7, true); if (error) return error; @@ -415,7 +415,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, false); + (void **) &sdata->encapContentInfo.eContent, true); if (error) goto release_sdata; @@ -447,7 +447,7 @@ signed_data_decode(ANY_t *encoded, struct SignedData **decoded) int error; error = asn1_decode_any(encoded, &asn_DEF_SignedData, - (void **) decoded, false, false); + (void **) decoded, false); if (error) { /* Try to decode as PKCS content (RFC 5652 section 5.2.1) */ error = signed_data_decode_pkcs7(encoded, decoded); @@ -494,7 +494,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, false); + (void **) result, true); } } diff --git a/src/object/certificate.c b/src/object/certificate.c index 67c328a2..58a6359c 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -1025,7 +1025,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, false); + (void **) &blocks, true); if (error) return error; @@ -1073,7 +1073,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, false); + &asn_DEF_ASIdentifiers, (void **) &ids, true); if (error) return error; diff --git a/src/object/manifest.c b/src/object/manifest.c index 9d42dc72..d3267644 100644 --- a/src/object/manifest.c +++ b/src/object/manifest.c @@ -35,8 +35,7 @@ decode_manifest(struct signed_object *sobj, struct Manifest **result) sobj->sdata->encapContentInfo.eContent, &asn_DEF_Manifest, (void **) result, - true, - false + true ); } diff --git a/src/object/roa.c b/src/object/roa.c index eeda8108..99ff75f9 100644 --- a/src/object/roa.c +++ b/src/object/roa.c @@ -15,8 +15,7 @@ decode_roa(struct signed_object *sobj, struct RouteOriginAttestation **result) sobj->sdata->encapContentInfo.eContent, &asn_DEF_RouteOriginAttestation, (void **) result, - true, - false + true ); } -- 2.47.3