From: pcarana Date: Tue, 9 Apr 2019 17:06:26 +0000 (-0500) Subject: Complete bgpsec parsing (decode base64 using crypto functions) X-Git-Tag: v0.0.2~49^2~10 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=d6fbcc9c48807321a6e55b43405df956c24fe11f;p=thirdparty%2FFORT-validator.git Complete bgpsec parsing (decode base64 using crypto functions) --- diff --git a/configure.ac b/configure.ac index 2a6aa36a..c4510386 100644 --- a/configure.ac +++ b/configure.ac @@ -23,12 +23,52 @@ AC_CHECK_HEADER_STDBOOL # Checks for library functions. AC_FUNC_MALLOC AC_CHECK_FUNCS([memset socket]) -AC_SEARCH_LIBS([pthread_create], [pthread]) +AC_SEARCH_LIBS([pthread_create], [pthread], [], + [AC_MSG_ERROR([unable to find the pthread() function])] +) +AC_SEARCH_LIBS([BIO_new], [crypto], [], + [AC_MSG_ERROR([unable to find the BIO_new() function])] +) +AC_SEARCH_LIBS([json_load_file], [jansson], [], + [AC_MSG_ERROR([unable to find the json_load_file() function])] +) -# Uhhh... this one starts with "PKG_" so it's probably different. -# No idea. -PKG_CHECK_MODULES([CHECK], [check]) -PKG_CHECK_MODULES([JANSSON], [jansson]) +# Dependencies managed by pkg-config + +# By the way: Apparently PKG_CHECK_MODULES is poor practice now. I can't tell; +# it's always the same guy complaining about it in Stack Overflow. +# (Main one: https://stackoverflow.com/questions/10220946) +# But I couldn't make check work with AC_SEARCH_LIBS, and (probably due to +# typical obscure bullshit autotools reasoning) I have no idea why. + +AC_ARG_WITH( + [unit-tests], + AS_HELP_STRING( + [--with-unit-tests], + [Generate unit tests. (Requires pkg-config and check)] + ) +) + +# https://www.gnu.org/software/automake/manual/html_node/Conditional-Subdirectories.html +# "Subdirectories with AM_CONDITIONAL" never worked for me. The problem might +# be that it puts `MAYBE_TESTS` in `.PRECIOUS`, which maybe is a bug in the +# autotools. (I honestly can't tell. They are so incredibly poorly designed.) +# The code below is implemented as "Subdirectories with AC_SUBST." +# +# https://autotools.io/pkgconfig/pkg_check_modules.html#pkgconfig.pkg_check_modules.optional +# Note: Since Check is the only current user of pkg-config, I don't want to +# force pkg-config as a dependency; it should only be needed by people who want +# to unit test. That's why I chose to use an `if` (which skips the +# `PKG_CHECK_MODULES` on false) instead of an `AS_IF` (which evaluates all paths +# apparently). In other words, if you ever want to add another pkg-config +# dependency, you will need the `AS_IF` version. +if test "x$with_unit_tests" = "xyes"; then + PKG_CHECK_MODULES([CHECK], [check]) + MAYBE_TESTS=test +else + MAYBE_TESTS= +fi +AC_SUBST([MAYBE_TESTS]) # Spit out the makefiles. AC_OUTPUT(Makefile src/Makefile man/Makefile test/Makefile) diff --git a/src/Makefile.am b/src/Makefile.am index 39cd7c8e..2d6b627f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -26,6 +26,8 @@ rtr_server_SOURCES += rtr/rtr.c rtr/rtr.h rtr_server_SOURCES += slurm/slurm_parser.c slurm/slurm_parser.h +rtr_server_SOURCES += crypto/base64.c crypto/base64.h + rtr_server_LDADD = ${JANSSON_LIBS} # I'm tired of scrolling up, but feel free to comment this out. diff --git a/src/configuration.c b/src/configuration.c index febe6e97..422d433c 100644 --- a/src/configuration.c +++ b/src/configuration.c @@ -112,6 +112,8 @@ config_cleanup(void) free(config.port); if (config.vrps_location != NULL) free(config.vrps_location); + if (config.slurm_location != NULL) + free(config.slurm_location); } static int diff --git a/src/crypto/base64.c b/src/crypto/base64.c new file mode 100644 index 00000000..15535594 --- /dev/null +++ b/src/crypto/base64.c @@ -0,0 +1,122 @@ +#include "base64.h" + +#include +#include +#include + +/* + * TODO This is a copy of fort-validator/src/crypto/base64.c with a small + * modification at 'base64_decode' (new parameter 'has_nl' to indicate if the + * encoded string has newlines in it) + */ + +/** + * Converts error from libcrypto representation to this project's + * representation. + */ +static int +error_ul2i(unsigned long error) +{ + /* I'm assuming int has at least 32 bits. Don't mess with the sign. */ + int interror = error & 0x7FFFFFFFul; + return interror ? interror : -EINVAL; +} + +/* + * Reference: openbsd/src/usr.bin/openssl/enc.c + * + * @in: The BIO that will stream the base64 encoded string you want to decode. + * @out: Buffer where this function will write the decoded string. + * @has_nl: Indicate if the encoded string has newline char. + * @out_len: Total allocated size of @out. It's supposed to be the result of + * EVP_DECODE_LENGTH(). + * @out_written: This function will write the actual number of decoded bytes + * here. + * + * Returns error status. (Nonzero = error code, zero = success) + * + * If this returns error, do visit ERR_print_errors(), but also print an + * additional error message anyway. Functions such as BIO_new() don't always + * register a libcrypto stack error. + */ +int +base64_decode(BIO *in, unsigned char **out, bool has_nl, size_t out_len, + size_t *out_written) +{ + BIO *b64; + size_t offset = 0; + int written = 0; + unsigned long error; + + /* + * BTW: The libcrypto API was clearly designed by fucking idiots. + * Peeking at the error stack is the only way I found to figure out + * whether some of the functions error'd. + * But since it's not documented that it's supposed to work this way, + * there's no guarantee that it will catch all errors. + * But it will have to do. It's better than nothing. + */ + + /* Assume that the caller took care of handling any previous errors. */ + ERR_clear_error(); + + /* + * BIO_f_base64() cannot fail because it's dead-simple by definition. + * BIO_new() can, and it will lead to NULL. But only *some* errors will + * populate the error stack. + */ + b64 = BIO_new(BIO_f_base64()); + if (b64 == NULL) { + error = ERR_peek_last_error(); + return error ? error_ul2i(error) : -ENOMEM; + } + + /* + * BIO_push() can technically fail through BIO_ctrl(), but it ignores + * the error. This will not cause it to revert the push, so we have to + * do it ourselves. + * Should we ignore this error? BIO_ctrl(BIO_CTRL_PUSH) performs some + * "internal, used to signify change" thing, whose importance is + * undefined due to BIO_ctrl()'s callback spaghetti. + * I'm not risking it. + */ + in = BIO_push(b64, in); + if (!has_nl) + BIO_set_flags(in, BIO_FLAGS_BASE64_NO_NL); + + error = ERR_peek_last_error(); + if (error) + goto end; + + do { + /* + * Do not move this after BIO_read(). + * BIO_read() can return negative, which does not necessarily + * imply error, and which ruins the counter. + */ + offset += written; + /* + * According to the documentation, the first argument should + * be b64, not in. + * But this is how it's written in enc.c. + * It doesn't seem to make a difference either way. + */ + written = BIO_read(in, *out + offset, (out_len - offset)); + } while (written > 0); + + /* BIO_read() can fail. It does not return status. */ + error = ERR_peek_last_error(); + *out_written = offset; + +end: + /* + * BIO_pop() can also fail due to BIO_ctrl(), but we will ignore this + * because whatever "signify change" crap happens, it can't possibly be + * damaging enough to prevent us from releasing b64. I hope. + */ + BIO_pop(b64); + /* Returns 0 on failure, but that's only if b64 is NULL. Meaningless. */ + BIO_free(b64); + + return error ? error_ul2i(error) : 0; +} diff --git a/src/crypto/base64.h b/src/crypto/base64.h new file mode 100644 index 00000000..5aebce22 --- /dev/null +++ b/src/crypto/base64.h @@ -0,0 +1,10 @@ +#ifndef SRC_BASE64_H_ +#define SRC_BASE64_H_ + +#include +#include +#include + +int base64_decode(BIO *, unsigned char **, bool, size_t, size_t *); + +#endif /* SRC_BASE64_H_ */ diff --git a/src/slurm/slurm_parser.c b/src/slurm/slurm_parser.c index 8d102203..71b7ee10 100644 --- a/src/slurm/slurm_parser.c +++ b/src/slurm/slurm_parser.c @@ -6,9 +6,11 @@ #include #include #include +#include #include "../address.h" #include "../configuration.h" +#include "../crypto/base64.h" /* JSON members */ #define SLURM_VERSION "slurmVersion" @@ -49,7 +51,8 @@ struct slurm_prefix { struct slurm_bgpsec { u_int8_t data_flag; u_int32_t asn; - /* TODO Add SKI (Base64) and routerPublicKey */ + unsigned char *ski; + unsigned char *routerPublicKey; char const *comment; }; @@ -150,6 +153,21 @@ set_asn(json_t *object, bool is_assertion, u_int32_t *result, return 0; } +static int +set_comment(json_t *object, char const **comment, u_int8_t *flag) +{ + int error; + + error = json_get_string(object, COMMENT, comment); + if (error) + return error; + + if (*comment != NULL) + *flag = *flag | SLURM_COM_FLAG_COMMENT; + + return 0; +} + static int set_prefix(json_t *object, bool is_assertion, struct slurm_prefix *result) { @@ -220,21 +238,6 @@ set_prefix(json_t *object, bool is_assertion, struct slurm_prefix *result) return 0; } -static int -set_comment(json_t *object, char const **comment, u_int8_t *flag) -{ - int error; - - error = json_get_string(object, COMMENT, comment); - if (error) - return error; - - if (*comment != NULL) - *flag = *flag | SLURM_COM_FLAG_COMMENT; - - return 0; -} - static int set_max_prefix_length(json_t *object, bool is_assertion, u_int8_t addr_fam, u_int8_t *result, u_int8_t *flag) @@ -266,6 +269,178 @@ set_max_prefix_length(json_t *object, bool is_assertion, u_int8_t addr_fam, } +static int +decode_base64url(char const *str_encoded, unsigned char **result) +{ + BIO *encoded; /* base64 encoded. */ + char *str_copy; + size_t encoded_len, alloc_size, dec_len; + int error, pad, i; + + /* + * RFC 8416, sections 3.3.2 (SKI member), and 3.4.2 (SKI and + * routerPublicKey members): "{..} whose value is the Base64 encoding + * without trailing '=' (Section 5 of [RFC4648])" + */ + if (strrchr(str_encoded, '=') != NULL) { + warnx("The base64 encoded value has trailing '='"); + return -EINVAL; + } + + /* + * IMHO there's an error at RFC 8416 regarding the use of base64 + * encoding. The RFC cites "RFC 4648 section 5" to justify the + * removal of trailing pad char '=', a section that refers to base64url + * encoding. So, at the same RFC 4648 section, there's this paragraph: + * "This encoding may be referred to as "base64url". This encoding + * should not be regarded as the same as the "base64" encoding and + * should not be referred to as only "base64". Unless clarified + * otherwise, "base64" refers to the base 64 in the previous section." + * + * Well, I believe that the RFC 8416 must say something like: + * "{..} whose value is the Base64url encoding without trailing '=' + * (Section 5 of [RFC4648])" + */ + + /* + * Apparently there isn't a base64url decoder, and there isn't + * much difference between base64 codification and base64url, just as + * stated in RFC 4648 section 5: "This encoding is technically + * identical to the previous one, except for the 62:nd and 63:rd + * alphabet character, as indicated in Table 2". + * + * The existing base64 can be used if the 62:nd and 63:rd base64url + * alphabet chars are replaced with the corresponding base64 chars, and + * also if we add the optional padding that the member should have. + */ + encoded_len = strlen(str_encoded); + pad = (encoded_len % 4) > 0 ? 4 - (encoded_len % 4) : 0; + + str_copy = malloc(encoded_len + pad + 1); + if (str_copy == NULL) + return -ENOMEM; + /* Set all with pad char, then replace with the original string */ + memset(str_copy, '=', encoded_len + pad); + memcpy(str_copy, str_encoded, encoded_len); + str_copy[encoded_len + pad] = '\0'; + + for (i = 0; i < encoded_len; i++) { + if (str_copy[i] == '-') + str_copy[i] = '+'; + else if (str_copy[i] == '_') + str_copy[i] = '/'; + } + + /* Now decode as regular base64 */ + encoded = BIO_new_mem_buf(str_copy, -1); + if (encoded == NULL) { + warnx("BIO_new() returned NULL"); + error = -EINVAL; + goto free_copy; + } + + alloc_size = EVP_DECODE_LENGTH(strlen(str_copy)); + *result = malloc(alloc_size + 1); + if (*result == NULL) { + error = -ENOMEM; + goto free_enc; + } + memset(*result, 0, alloc_size); + (*result)[alloc_size] = '\0'; + + error = base64_decode(encoded, result, false, alloc_size, &dec_len); + if (error) + goto free_all; + + if (dec_len == 0) { + warnx("'%s' couldn't be decoded", str_encoded); + error = -EINVAL; + goto free_all; + } + + free(str_copy); + BIO_free(encoded); + return 0; +free_all: + free(*result); +free_enc: + BIO_free(encoded); +free_copy: + free(str_copy); + return error; +} + +static int +set_ski(json_t *object, bool is_assertion, struct slurm_bgpsec *result) +{ + char const *str_encoded; + int error; + + error = json_get_string(object, SKI, &str_encoded); + if (error) + return error; + + if (str_encoded == NULL) { + /* Optional for filters */ + if(is_assertion) { + warnx("SLURM assertion %s is required", SKI); + return -EINVAL; + } else + return 0; + } + + error = decode_base64url(str_encoded, &result->ski); + if (error) + return error; + + /* TODO persist, free later */ + free(result->ski); + + result->data_flag = result->data_flag | SLURM_BGPS_FLAG_SKI; + return 0; +} + +static int +set_router_pub_key(json_t *object, bool is_assertion, + struct slurm_bgpsec *result) +{ + char const *str_encoded; + int error; + + /* Ignore for filters */ + if (!is_assertion) + return 0; + + error = json_get_string(object, ROUTER_PUBLIC_KEY, &str_encoded); + if (error) + return error; + + /* Required for assertions */ + if (str_encoded == NULL) { + warnx("SLURM assertion %s is required", ROUTER_PUBLIC_KEY); + return -EINVAL; + } + + /* TODO The public key may contain NULL chars as part of the string */ + error = decode_base64url(str_encoded, &result->routerPublicKey); + if (error) + return error; + + /* + * TODO Validate that 'routerPublicKey' is: "the equivalent to the + * subjectPublicKeyInfo value of the router certificate's public key, + * as described in [RFC8208]. This is the full ASN.1 DER encoding of + * the subjectPublicKeyInfo, including the ASN.1 tag and length values + * of the subjectPublicKeyInfo SEQUENCE. + */ + + /* TODO persist, free later */ + free(result->routerPublicKey); + + result->data_flag = result->data_flag | SLURM_BGPS_FLAG_ROUTER_KEY; + return 0; +} + static int load_single_prefix(json_t *object, bool is_assertion) { @@ -326,10 +501,54 @@ load_prefix_array(json_t *array, bool is_assertion) return 0; } +static int +load_single_bgpsec(json_t *object, bool is_assertion) +{ + struct slurm_bgpsec result; + int error; + + if (!json_is_object(object)) { + warnx("Not a valid JSON object"); + return -EINVAL; + } + + result.data_flag = SLURM_COM_FLAG_NONE; + + error = set_asn(object, is_assertion, &result.asn, &result.data_flag); + if (error) + return error; + + error = set_ski(object, is_assertion, &result); + if (error) + return error; + + error = set_router_pub_key(object, is_assertion, &result); + if (error) + return error; + + error = set_comment(object, &result.comment, &result.data_flag); + if (error) + return error; + + return 0; +} + static int load_bgpsec_array(json_t *array, bool is_assertion) { - /* TODO Complete me */ + json_t *element; + int index, error; + + json_array_foreach(array, index, element) { + error = load_single_bgpsec(element, is_assertion); + if (error) { + warnx( + "Error at bgpsec %s, element %d, ignoring content", + (is_assertion ? "assertions" : "filters"), + index + 1); + } + } + return 0; }