On Wed, Aug 7, 2024 at 5:55 AM Heikki Linnakangas <hlinn...@iki.fi> wrote: > On 06/08/2024 03:58, Thomas Munro wrote: > > On Tue, Aug 6, 2024 at 2:41 AM Heikki Linnakangas <hlinn...@iki.fi> wrote: > >> What if the message contains multiple attribute of the same type? If > >> there's a duplicate Message-Authenticator, we should surely reject the > >> packet. I don't know if duplicate attributes are legal in general. > > > > Duplicate attributes are legal in general per RFC 2865, which has a > > table of attributes and their possible quantity; unfortunately this > > one is an extension from RFC 2869, and I didn't find where it pins > > that down.
There's a similar table near the end of 2869: https://datatracker.ietf.org/doc/html/rfc2869#section-5.19 > > I suppose if we wanted to be extra fastidious, we could also test with > > a gallery of malformed packets crafted by a Perl script, but that > > feels like overkill. On the other hand it would be bad if you could > > crash a server by lobbing UDP packets at it because of some dumb > > thinko. > > This would also be a easy target for fuzz testing. I'm not too worried > though, the packet format is pretty simple. Still, bugs happen. (Not a > requirement for this patch in any case) <tangent> I've been working on fuzzing JSON, and I spent some time adapting that to test this RADIUS code. No pressure to use any of it (the refactoring to pull out the response validation is cowboy-quality at best), but it might help anyone who wants to pursue it in the future? This fuzzer hasn't been able to find anything in the response parser. (But the modifications I made make that claim a little weaker, since I'm not testing what's actually shipping.) The attached response corpus could be used to seed a malformed-packet gallery like Thomas mentioned; it's built with the assumption that the request packet was all zeroes and the shared secret is `secret`. I was impressed with how quickly LLVM was able to find the packet shape, including valid signatures. The only thing I had to eventually add manually was a valid Message-Authenticator case; I assume the interdependency between the authenticator and the packet checksum was a little too much for libfuzzer to figure out on its own. </tangent> --Jacob
From f04087abbe3381ee3a546750a217b980143441d9 Mon Sep 17 00:00:00 2001 From: Jacob Champion <jacob.champ...@enterprisedb.com> Date: Thu, 8 Aug 2024 11:12:05 -0700 Subject: [PATCH] WIP: try to fuzz RADIUS implementation - extract validate_radius_response() - add LLVM fuzzer implementation - suppress logging by default, let --log_min_messages override it - support const response buffer (and improve fuzzability) by computing Message-Authenticator piecewise --- meson-clang.ini | 4 + src/backend/libpq/auth.c | 336 +++++++++++++------------ src/backend/meson.build | 18 ++ src/common/meson.build | 16 ++ src/include/libpq/auth.h | 21 ++ src/test/modules/fuzzers/fuzz_radius.c | 74 ++++++ src/test/modules/fuzzers/meson.build | 24 ++ src/test/modules/meson.build | 1 + 8 files changed, 330 insertions(+), 164 deletions(-) create mode 100644 meson-clang.ini create mode 100644 src/test/modules/fuzzers/fuzz_radius.c create mode 100644 src/test/modules/fuzzers/meson.build diff --git a/meson-clang.ini b/meson-clang.ini new file mode 100644 index 0000000000..560cffe8e7 --- /dev/null +++ b/meson-clang.ini @@ -0,0 +1,4 @@ +[binaries] +c = 'clang-15' +cpp = 'clang++-15' +llvm-config = 'llvm-config-15' diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 40f0cabf3a..d97fd6e3f2 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -2769,13 +2769,9 @@ CheckCertAuth(Port *port) * RADIUS authentication is described in RFC2865 (and several others). */ -#define RADIUS_VECTOR_LENGTH 16 #define RADIUS_HEADER_LENGTH 20 #define RADIUS_MAX_PASSWORD_LENGTH 128 -/* Maximum size of a RADIUS packet we will create or accept */ -#define RADIUS_BUFFER_SIZE 1024 - typedef struct { uint8 attribute; @@ -2783,16 +2779,6 @@ typedef struct uint8 data[FLEXIBLE_ARRAY_MEMBER]; } radius_attribute; -typedef struct -{ - uint8 code; - uint8 id; - uint16 length; - uint8 vector[RADIUS_VECTOR_LENGTH]; - /* this is a bit longer than strictly necessary: */ - char pad[RADIUS_BUFFER_SIZE - RADIUS_VECTOR_LENGTH]; -} radius_packet; - /* RADIUS packet types */ #define RADIUS_ACCESS_REQUEST 1 #define RADIUS_ACCESS_ACCEPT 2 @@ -2993,11 +2979,9 @@ PerformRadiusTransaction(const char *server, const char *secret, const char *por uint8 message_authenticator_key[RADIUS_VECTOR_LENGTH]; uint8 message_authenticator[RADIUS_VECTOR_LENGTH]; uint8 *message_authenticator_location; - uint8 message_authenticator_size; radius_packet radius_send_pack; radius_packet radius_recv_pack; radius_packet *packet = &radius_send_pack; - radius_packet *receivepacket = &radius_recv_pack; char *radius_buffer = (char *) &radius_send_pack; char *receive_buffer = (char *) &radius_recv_pack; int32 service = pg_hton32(RADIUS_AUTHENTICATE_ONLY); @@ -3216,7 +3200,7 @@ PerformRadiusTransaction(const char *server, const char *secret, const char *por struct timeval timeout; struct timeval now; int64 timeoutval; - const char *errstr = NULL; + int status; gettimeofday(&now, NULL); timeoutval = (endtime.tv_sec * 1000000 + endtime.tv_usec) - (now.tv_sec * 1000000 + now.tv_usec); @@ -3285,167 +3269,191 @@ PerformRadiusTransaction(const char *server, const char *secret, const char *por continue; } - if (packetlength < RADIUS_HEADER_LENGTH) + if (validate_radius_response(&status, receive_buffer, packet, packetlength, secret, server, user_name, requirema)) { - ereport(LOG, - (errmsg("RADIUS response from %s too short: %d", server, packetlength))); - continue; + closesocket(sock); + return status; } - if (packetlength != pg_ntoh16(receivepacket->length)) - { - ereport(LOG, - (errmsg("RADIUS response from %s has corrupt length: %d (actual length %d)", - server, pg_ntoh16(receivepacket->length), packetlength))); - continue; - } + /* otherwise, keep trying */ + } /* while (true) */ +} - if (packet->id != receivepacket->id) - { - ereport(LOG, - (errmsg("RADIUS response from %s is to a different request: %d (should be %d)", - server, receivepacket->id, packet->id))); - continue; - } +bool +validate_radius_response(int *status, + const char *receive_buffer, radius_packet *packet, + size_t packetlength, const char *secret, + const char *server, const char *user_name, bool requirema) +{ + radius_packet *receivepacket = (radius_packet *) receive_buffer; + uint8 *cryptvector; + uint8 encryptedpassword[RADIUS_MAX_PASSWORD_LENGTH]; + uint8 *message_authenticator_location; + uint8 message_authenticator_size; + uint8 message_authenticator[RADIUS_VECTOR_LENGTH]; + uint8 message_authenticator_key[RADIUS_VECTOR_LENGTH] = {0}; + pg_hmac_ctx *hmac_context; + const char *errstr = NULL; - /* - * Verify the response authenticator, which is calculated as - * MD5(Code+ID+Length+RequestAuthenticator+Attributes+Secret) - */ - cryptvector = palloc(packetlength + strlen(secret)); - - memcpy(cryptvector, receivepacket, 4); /* code+id+length */ - memcpy(cryptvector + 4, packet->vector, RADIUS_VECTOR_LENGTH); /* request - * authenticator, from - * original packet */ - if (packetlength > RADIUS_HEADER_LENGTH) /* there may be no - * attributes at all */ - memcpy(cryptvector + RADIUS_HEADER_LENGTH, receive_buffer + RADIUS_HEADER_LENGTH, packetlength - RADIUS_HEADER_LENGTH); - memcpy(cryptvector + packetlength, secret, strlen(secret)); - - if (!pg_md5_binary(cryptvector, - packetlength + strlen(secret), - encryptedpassword, &errstr)) - { - ereport(LOG, - (errmsg("could not perform MD5 encryption of received packet: %s", - errstr))); - pfree(cryptvector); - continue; - } - pfree(cryptvector); + if (packetlength < RADIUS_HEADER_LENGTH) + { + ereport(LOG, + (errmsg("RADIUS response from %s too short: %d", server, (int) packetlength))); + return false; + } - if (memcmp(receivepacket->vector, encryptedpassword, RADIUS_VECTOR_LENGTH) != 0) - { - ereport(LOG, - (errmsg("RADIUS response from %s has incorrect MD5 signature", - server))); - continue; - } + if (packetlength != pg_ntoh16(receivepacket->length)) + { + ereport(LOG, + (errmsg("RADIUS response from %s has corrupt length: %d (actual length %d)", + server, pg_ntoh16(receivepacket->length), (int) packetlength))); + return false; + } - /* Search for the Message-Authenticator attribute. */ - if (!radius_find_attribute((uint8 *) receive_buffer, - packetlength, - RADIUS_MESSAGE_AUTHENTICATOR, - &message_authenticator_location, - &message_authenticator_size)) - { - ereport(LOG, - (errmsg("RADIUS response from %s has malformed attributes", - server))); - continue; - } - else if (message_authenticator_location == NULL) - { - ereport(LOG, - (errmsg("RADIUS response from %s has no Message-Authenticator", - server))); + if (packet->id != receivepacket->id) + { + ereport(LOG, + (errmsg("RADIUS response from %s is to a different request: %d (should be %d)", + server, receivepacket->id, packet->id))); + return false; + } - /* We'll ignore this message, unless pg_hba.conf told us not to. */ - if (requirema) - continue; - } - else if (message_authenticator_size != RADIUS_VECTOR_LENGTH) - { - ereport(LOG, - (errmsg("RADIUS response from %s has unexpected Message-Authenticator size", - server))); - continue; - } - else - { - uint8 message_authenticator_copy[RADIUS_VECTOR_LENGTH]; + /* + * Verify the response authenticator, which is calculated as + * MD5(Code+ID+Length+RequestAuthenticator+Attributes+Secret) + */ + cryptvector = palloc(packetlength + strlen(secret)); - /* - * Save a copy of the received HMAC, and zero out the one in the - * message so that we have input required to recompute it. - */ - memcpy(message_authenticator_copy, - message_authenticator_location, - RADIUS_VECTOR_LENGTH); - memset(message_authenticator_location, - 0, - RADIUS_VECTOR_LENGTH); + memcpy(cryptvector, receivepacket, 4); /* code+id+length */ + memcpy(cryptvector + 4, packet->vector, RADIUS_VECTOR_LENGTH); /* request + * authenticator, from + * original packet */ + if (packetlength > RADIUS_HEADER_LENGTH) /* there may be no + * attributes at all */ + memcpy(cryptvector + RADIUS_HEADER_LENGTH, receive_buffer + RADIUS_HEADER_LENGTH, packetlength - RADIUS_HEADER_LENGTH); + memcpy(cryptvector + packetlength, secret, strlen(secret)); - /* - * Compute the expected value. Note that the HMAC for - * Access-Accept and Access-Reject message uses the authenticator - * from the original Access-Request message, so we have to do a - * bit of splicing. - */ - hmac_context = pg_hmac_create(PG_MD5); - if (hmac_context == NULL || - pg_hmac_init(hmac_context, - message_authenticator_key, - lengthof(message_authenticator_key)) < 0 || - pg_hmac_update(hmac_context, - (uint8 *) receive_buffer, - offsetof(radius_packet, vector)) < 0 || - pg_hmac_update(hmac_context, - packet->vector, - RADIUS_VECTOR_LENGTH) < 0 || - pg_hmac_update(hmac_context, - (uint8 *) receive_buffer + RADIUS_HEADER_LENGTH, - packetlength - RADIUS_HEADER_LENGTH) < 0 || - pg_hmac_final(hmac_context, - message_authenticator, - lengthof(message_authenticator)) < 0) - { - ereport(LOG, (errmsg("could not compute RADIUS Message-Authenticator: %s", - pg_hmac_error(hmac_context)))); - pg_hmac_free(hmac_context); - closesocket(sock); - return STATUS_ERROR; - } - pg_hmac_free(hmac_context); + if (!pg_md5_binary(cryptvector, + packetlength + strlen(secret), + encryptedpassword, &errstr)) + { + ereport(LOG, + (errmsg("could not perform MD5 encryption of received packet: %s", + errstr))); + pfree(cryptvector); + return false; + } + pfree(cryptvector); - /* Verify. */ - if (memcmp(message_authenticator_copy, - message_authenticator, - RADIUS_VECTOR_LENGTH) != 0) - { - ereport(LOG, (errmsg("RADIUS response from %s has invalid Message-Authenticator", - server))); - continue; - } - } + if (memcmp(receivepacket->vector, encryptedpassword, RADIUS_VECTOR_LENGTH) != 0) + { + ereport(LOG, + (errmsg("RADIUS response from %s has incorrect MD5 signature", + server))); + return false; + } - if (receivepacket->code == RADIUS_ACCESS_ACCEPT) - { - closesocket(sock); - return STATUS_OK; - } - else if (receivepacket->code == RADIUS_ACCESS_REJECT) + /* Search for the Message-Authenticator attribute. */ + if (!radius_find_attribute((uint8 *) receive_buffer, + packetlength, + RADIUS_MESSAGE_AUTHENTICATOR, + &message_authenticator_location, + &message_authenticator_size)) + { + ereport(LOG, + (errmsg("RADIUS response from %s has malformed attributes", + server))); + return false; + } + else if (message_authenticator_location == NULL) + { + ereport(LOG, + (errmsg("RADIUS response from %s has no Message-Authenticator", + server))); + + /* We'll ignore this message, unless pg_hba.conf told us not to. */ + if (requirema) + return false; + } + else if (message_authenticator_size != RADIUS_VECTOR_LENGTH) + { + ereport(LOG, + (errmsg("RADIUS response from %s has unexpected Message-Authenticator size", + server))); + return false; + } + else + { + uint8 message_authenticator_zero[RADIUS_VECTOR_LENGTH] = {0}; + const uint8 *attrs = (uint8 *) receive_buffer + RADIUS_HEADER_LENGTH; + const uint8 *end = (uint8 *) receive_buffer + packetlength; + + strncpy((char *) message_authenticator_key, secret, sizeof(message_authenticator_key)); + + /* + * Compute the expected value. Note that the HMAC for + * Access-Accept and Access-Reject message uses the authenticator + * from the original Access-Request message, so we have to do a + * bit of splicing. + */ + hmac_context = pg_hmac_create(PG_MD5); + if (hmac_context == NULL || + pg_hmac_init(hmac_context, + message_authenticator_key, + lengthof(message_authenticator_key)) < 0 || + pg_hmac_update(hmac_context, + (uint8 *) receive_buffer, + offsetof(radius_packet, vector)) < 0 || + pg_hmac_update(hmac_context, + packet->vector, + RADIUS_VECTOR_LENGTH) < 0 || + pg_hmac_update(hmac_context, + attrs, + message_authenticator_location - attrs) < 0 || + pg_hmac_update(hmac_context, + message_authenticator_zero, + RADIUS_VECTOR_LENGTH) < 0 || + pg_hmac_update(hmac_context, + message_authenticator_location + RADIUS_VECTOR_LENGTH, + end - (message_authenticator_location + RADIUS_VECTOR_LENGTH)) < 0 || + pg_hmac_final(hmac_context, + message_authenticator, + lengthof(message_authenticator)) < 0) { - closesocket(sock); - return STATUS_EOF; + ereport(LOG, (errmsg("could not compute RADIUS Message-Authenticator: %s", + pg_hmac_error(hmac_context)))); + pg_hmac_free(hmac_context); + *status = STATUS_ERROR; + return true; } - else + pg_hmac_free(hmac_context); + + /* Verify. */ + if (memcmp(message_authenticator_location, + message_authenticator, + RADIUS_VECTOR_LENGTH) != 0) { - ereport(LOG, - (errmsg("RADIUS response from %s has invalid code (%d) for user \"%s\"", - server, receivepacket->code, user_name))); - continue; + ereport(LOG, (errmsg("RADIUS response from %s has invalid Message-Authenticator", + server))); + return false; } - } /* while (true) */ + } + + if (receivepacket->code == RADIUS_ACCESS_ACCEPT) + { + *status = STATUS_OK; + return true; + } + else if (receivepacket->code == RADIUS_ACCESS_REJECT) + { + *status = STATUS_EOF; + return true; + } + else + { + ereport(LOG, + (errmsg("RADIUS response from %s has invalid code (%d) for user \"%s\"", + server, receivepacket->code, user_name))); + return false; + } } diff --git a/src/backend/meson.build b/src/backend/meson.build index 78c5726814..3db6a64baf 100644 --- a/src/backend/meson.build +++ b/src/backend/meson.build @@ -70,6 +70,24 @@ postgres_lib = static_library('postgres_lib', kwargs: internal_lib_args, ) +fuzzer_backend_sources = [] +foreach item : backend_sources + if item not in main_file + fuzzer_backend_sources += item + endif +endforeach + +postgres_lib_fuzzer = static_library('postgres_lib_fuzzer', + fuzzer_backend_sources + timezone_sources + generated_backend_sources, + link_whole: backend_link_with, + dependencies: backend_build_deps, + c_pch: pch_postgres_h, + kwargs: internal_lib_args + { + 'c_args': ['-fsanitize=address,undefined,fuzzer-no-link'], + 'link_args': ['-fsanitize=address,undefined,fuzzer-no-link'], + }, +) + if cc.get_id() == 'msvc' postgres_def = custom_target('postgres.def', command: [perl, files('../tools/msvc_gendef.pl'), diff --git a/src/common/meson.build b/src/common/meson.build index de68e408fa..dee9beff1f 100644 --- a/src/common/meson.build +++ b/src/common/meson.build @@ -145,6 +145,17 @@ pgcommon_variants = { }, } +pgcommon_variants += { + '_fuzzer': pgcommon_variants[''] + { + 'c_args': pgcommon_variants['']['c_args'] + [ + '-fsanitize=address,undefined,fuzzer-no-link', + ], + 'link_args': [ + '-fsanitize=address,undefined,fuzzer-no-link', + ], + }, +} + foreach name, opts : pgcommon_variants # Build internal static libraries for sets of files that need to be built @@ -182,4 +193,9 @@ common_srv = pgcommon['_srv'] common_shlib = pgcommon['_shlib'] common_static = pgcommon[''] +common_fuzzer = declare_dependency( + link_with: pgcommon['_fuzzer'], + link_args: ['-fsanitize=address,undefined,fuzzer-no-link'] +) + subdir('unicode') diff --git a/src/include/libpq/auth.h b/src/include/libpq/auth.h index 227b41daf6..580f8422b3 100644 --- a/src/include/libpq/auth.h +++ b/src/include/libpq/auth.h @@ -34,4 +34,25 @@ typedef char *(*auth_password_hook_typ) (char *input); /* Default LDAP password mutator hook, can be overridden by a shared library */ extern PGDLLIMPORT auth_password_hook_typ ldap_password_hook; +#define RADIUS_VECTOR_LENGTH 16 + +/* Maximum size of a RADIUS packet we will create or accept */ +#define RADIUS_BUFFER_SIZE 1024 + +typedef struct +{ + uint8 code; + uint8 id; + uint16 length; + uint8 vector[RADIUS_VECTOR_LENGTH]; + /* this is a bit longer than strictly necessary: */ + char pad[RADIUS_BUFFER_SIZE - RADIUS_VECTOR_LENGTH]; +} radius_packet; + +extern bool +validate_radius_response(int *status, + const char *receive_buffer, radius_packet *packet, + size_t packetlength, const char *secret, + const char *server, const char *user_name, bool requirema); + #endif /* AUTH_H */ diff --git a/src/test/modules/fuzzers/fuzz_radius.c b/src/test/modules/fuzzers/fuzz_radius.c new file mode 100644 index 0000000000..7955f76e42 --- /dev/null +++ b/src/test/modules/fuzzers/fuzz_radius.c @@ -0,0 +1,74 @@ +#include "postgres.h" + +#include <getopt.h> +#include <unistd.h> + +#include "libpq/auth.h" +#include "postmaster/postmaster.h" +#include "utils/guc.h" +#include "utils/memutils.h" +#include "utils/resowner.h" + +const char *progname; /* to replace the export from main.c */ + +int +LLVMFuzzerInitialize(int *pargc, char ***pargv) +{ + static const struct option opts[] = { + { "log_min_messages", required_argument, NULL, 1000 }, + { 0 }, + }; + + int c; + char *log_min_messages = "fatal"; + + opterr = 0; + while ((c = getopt_long(*pargc, *pargv, "", opts, NULL)) != -1) + { + char *arg; + + if (c == 1000) /* log_min_messages */ + { + log_min_messages = optarg; + continue; + } + else if (c != '?' || optopt != 0) + { + /* Ignore other valid options and unrecognized short options. */ + continue; + } + + arg = (*pargv)[optind - 1]; + fprintf(stderr, "unrecognized argument: %s\n", arg); + exit(1); + } + + /* Fake server startup. */ + progname = "postgres"; + MemoryContextInit(); + CreateAuxProcessResourceOwner(); + InitializeGUCOptions(); + SetConfigOption("log_min_messages", log_min_messages, + PGC_POSTMASTER, PGC_S_ARGV); + + return 0; +} + +int +LLVMFuzzerTestOneInput(const uint8_t *data, size_t len) +{ + int status; + radius_packet packet = {0}; + + validate_radius_response(&status, + (const char *) data, /* input response */ + &packet, /* request packet already sent */ + len, /* length of input response */ + "secret", /* shared secret */ + NULL, /* server name */ + NULL, /* user name */ + false /* require Message-Authenticator? */ + ); + + return 0; +} diff --git a/src/test/modules/fuzzers/meson.build b/src/test/modules/fuzzers/meson.build new file mode 100644 index 0000000000..3dfc257fc1 --- /dev/null +++ b/src/test/modules/fuzzers/meson.build @@ -0,0 +1,24 @@ +# Copyright (c) 2024, PostgreSQL Global Development Group + +can_fuzz = cc.has_argument('-fsanitize=fuzzer') +if can_fuzz + fuzz_radius_sources = files( + 'fuzz_radius.c', + ) + + fuzz_radius = executable('fuzz_radius', + fuzz_radius_sources + generated_headers, + c_args: [ + '-fsanitize=address,fuzzer', + '-Wno-missing-prototypes', + '-fprofile-instr-generate', '-fcoverage-mapping' + ], + link_args: ['-fsanitize=address,fuzzer'], + include_directories: [postgres_inc], + link_with: [postgres_lib_fuzzer, pgport_static], + dependencies: [os_deps, libintl], + kwargs: default_bin_args + { + 'install': false, + }, + ) +endif diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build index d8fe059d23..9765e0522a 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -5,6 +5,7 @@ subdir('commit_ts') subdir('delay_execution') subdir('dummy_index_am') subdir('dummy_seclabel') +subdir('fuzzers') subdir('gin') subdir('injection_points') subdir('ldap_password_func') -- 2.34.1
corpus.tar.gz
Description: GNU Zip compressed data