diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 431e7b5..5189c76 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -18,7 +18,21 @@ jobs: container: image: fedora:latest - name: Test compile + # Build and test under each supported C compiler. CTest, checksec, and the + # coverage target in test/CMakeLists.txt already branch on the compiler, so + # selecting CC here exercises the full pipeline for both toolchains. + strategy: + fail-fast: false + matrix: + compiler: + - gcc + - clang + + env: + # Forward the chosen compiler to CMake via CC. + CC: ${{ matrix.compiler }} + + name: Test compile (${{ matrix.compiler }}) steps: - name: Install build dependencies @@ -37,6 +51,10 @@ jobs: bats \ hostname \ lcov + # clang + LLVM coverage tools. + if [ "${{ matrix.compiler }}" = "clang" ]; then + dnf -y install clang clang-devel llvm + fi - name: Checkout uses: actions/checkout@v6 diff --git a/src/C/CMakeLists.txt b/src/C/CMakeLists.txt index 6022dab..2a117c7 100644 --- a/src/C/CMakeLists.txt +++ b/src/C/CMakeLists.txt @@ -114,6 +114,8 @@ add_compile_options( -Walloca # Warn on suspicious alloca usage -Wcast-qual # Warn on cast removing const/volatile -Wcast-align # Warn on alignment-affecting casts + -Wdouble-promotion # Implicit float-to-double promotion + -Wmissing-field-initializers # Struct fields left uninitialized -Wwrite-strings # String literals are const -Wundef # Undefined macro in #if -Wmissing-prototypes # Missing function prototypes @@ -140,6 +142,9 @@ if(CMAKE_C_COMPILER_ID STREQUAL "GNU") -Wduplicated-branches # Duplicated branches -Wnull-dereference # Potential NULL dereferences -Wjump-misses-init # Jump skips variable initialization + -Warith-conversion # Implicit arithmetic conversions + -Wstringop-overflow # String ops writing past buffer end + -Wtrampolines # Nested functions needing exec stack ) check_c_compiler_flag(-fplugin=annobin SET_ANNOBIN) @@ -222,12 +227,20 @@ else () message(WARNING "Stack clash protection not supported by compiler") endif(STACK_CLASH) -# Disable stack slot reuse +# Disable stack slot reuse. +# This is a GCC-only hardening flag: it prevents the compiler from +# reusing a single stack frame for nested calls, which would let a +# stack-clash attack defeat -fstack-clash-protection. Clang provides +# no equivalent option, so on Clang -fstack-clash-protection (enabled +# above) is the sole defense and this flag is skipped instead of aborting. CHECK_C_COMPILER_FLAG(-fstack-reuse=none STACK_REUSE) if (STACK_REUSE) add_compile_options(-fstack-reuse=none) -else () + message(STATUS "Stack slot reuse disabled: enabled") +elseif(CMAKE_C_COMPILER_ID STREQUAL "GNU") message(FATAL_ERROR "Required compiler flag '-fstack-reuse=none' not supported") +else () + message(WARNING "Clang does not support '-fstack-reuse=none'; relying on -fstack-clash-protection") endif(STACK_REUSE) # Zero call-used registers on return diff --git a/src/C/init-kerberos-client-dir.c b/src/C/init-kerberos-client-dir.c index ce181ae..91da78b 100644 --- a/src/C/init-kerberos-client-dir.c +++ b/src/C/init-kerberos-client-dir.c @@ -89,7 +89,11 @@ static void free_buffers(char *keytab, char *keytab_dirname, char *keytab_filena } } -static int mkdir_if_missing(const char *dir, uid_t owner, gid_t group, mode_t mode) __attribute__((access(read_only, 1))) __attribute__((warn_unused_result)); +static int mkdir_if_missing(const char *dir, uid_t owner, gid_t group, mode_t mode) +#if !defined(__clang__) + __attribute__((access(read_only, 1))) +#endif + __attribute__((warn_unused_result)); static int mkdir_if_missing(const char *dir, uid_t owner, gid_t group, mode_t mode) { const cap_value_t caps_dac_override[] = {CAP_DAC_OVERRIDE}; const int num_caps_dac_override = sizeof(caps_dac_override) / sizeof(cap_value_t); diff --git a/src/C/kcron_keytab_file.h b/src/C/kcron_keytab_file.h index b09bb98..4ecbe46 100644 --- a/src/C/kcron_keytab_file.h +++ b/src/C/kcron_keytab_file.h @@ -91,7 +91,11 @@ int validate_client_dirname(const char *client_keytab_dirname) { * Security Note: lstat() is used first (not stat()) to avoid a TOCTOU symlink * race that could substitute an attacker-controlled path. */ -int validate_keytab_directory(const char *keytab_dirname) __attribute__((access(read_only, 1))) __attribute__((warn_unused_result)); +int validate_keytab_directory(const char *keytab_dirname) +#if !defined(__clang__) + __attribute__((access(read_only, 1))) +#endif + __attribute__((warn_unused_result)); int validate_keytab_directory(const char *keytab_dirname) { struct stat lst = {0}; struct stat st = {0}; diff --git a/src/C/kcron_seccomp.h b/src/C/kcron_seccomp.h index a2cc59c..8b99fca 100644 --- a/src/C/kcron_seccomp.h +++ b/src/C/kcron_seccomp.h @@ -73,6 +73,13 @@ * * Security Principle: Default deny with explicit allow. If it's not in the list, * it can't be used. This is defense-in-depth against exploitation. + * + * Note on argument-restricted rules: SCMP_A0()/SCMP_A1() build a + * "struct scmp_arg_cmp" from only three fields (arg, op, datum_a). datum_b is + * now supplied explicitly as 0 so the call compiles identically under GCC and + * Clang; Clang otherwise reports -Wmissing-field-initializers on the omitted + * trailing field (GCC tolerates it, which is exactly why the omission is not + * obvious on the host build). datum_b is unused for SCMP_CMP_EQ. */ int set_kcron_seccomp(void) __attribute__((warn_unused_result)) __attribute__((flatten)); int set_kcron_seccomp(void) { @@ -151,14 +158,14 @@ int set_kcron_seccomp(void) { */ /* Write to stdout - for printing keytab path */ - if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(write), 1, SCMP_A0(SCMP_CMP_EQ, 1)) != 0) { + if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(write), 1, SCMP_A0(SCMP_CMP_EQ, 1, 0)) != 0) { (void)fprintf(stderr, "%s: Cannot allowlist 'write' to stdout: %s\n", __PROGRAM_NAME, strerror(errno)); (void)seccomp_release(ctx); exit(EXIT_FAILURE); } /* Write to stderr - for error messages */ - if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(write), 1, SCMP_A0(SCMP_CMP_EQ, 2)) != 0) { + if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(write), 1, SCMP_A0(SCMP_CMP_EQ, 2, 0)) != 0) { (void)fprintf(stderr, "%s: Cannot allowlist 'write' to stderr: %s\n", __PROGRAM_NAME, strerror(errno)); (void)seccomp_release(ctx); exit(EXIT_FAILURE); @@ -180,7 +187,7 @@ int set_kcron_seccomp(void) { } /* Close directory handle (fd 3) */ - if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(close), 1, SCMP_A0(SCMP_CMP_EQ, 3)) != 0) { + if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(close), 1, SCMP_A0(SCMP_CMP_EQ, 3, 0)) != 0) { (void)fprintf(stderr, "%s: Cannot allowlist 'close' for fd 3: %s\n", __PROGRAM_NAME, strerror(errno)); (void)seccomp_release(ctx); exit(EXIT_FAILURE); @@ -192,21 +199,21 @@ int set_kcron_seccomp(void) { */ /* Write to keytab file (fd 4) */ - if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(write), 1, SCMP_A0(SCMP_CMP_EQ, 4)) != 0) { + if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(write), 1, SCMP_A0(SCMP_CMP_EQ, 4, 0)) != 0) { (void)fprintf(stderr, "%s: Cannot allowlist 'write' to fd 4: %s\n", __PROGRAM_NAME, strerror(errno)); (void)seccomp_release(ctx); exit(EXIT_FAILURE); } /* Close keytab file (fd 4) */ - if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(close), 1, SCMP_A0(SCMP_CMP_EQ, 4)) != 0) { + if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(close), 1, SCMP_A0(SCMP_CMP_EQ, 4, 0)) != 0) { (void)fprintf(stderr, "%s: Cannot allowlist 'close' for fd 4: %s\n", __PROGRAM_NAME, strerror(errno)); (void)seccomp_release(ctx); exit(EXIT_FAILURE); } /* Sync keytab file to disk (fd 4) */ - if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(fsync), 1, SCMP_A0(SCMP_CMP_EQ, 4)) != 0) { + if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(fsync), 1, SCMP_A0(SCMP_CMP_EQ, 4, 0)) != 0) { (void)fprintf(stderr, "%s: Cannot allowlist 'fsync' for fd 4: %s\n", __PROGRAM_NAME, strerror(errno)); (void)seccomp_release(ctx); exit(EXIT_FAILURE); @@ -216,7 +223,7 @@ int set_kcron_seccomp(void) { * Set permissions on keytab file (fd 4) to 0600. * Restricts to exactly mode 0600 (user read/write only). */ - if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(fchmod), 2, SCMP_A0(SCMP_CMP_EQ, 4), SCMP_A1(SCMP_CMP_EQ, _0600)) != 0) { + if (seccomp_rule_add(ctx, SCMP_ACT_ALLOW, SCMP_SYS(fchmod), 2, SCMP_A0(SCMP_CMP_EQ, 4, 0), SCMP_A1(SCMP_CMP_EQ, _0600, 0)) != 0) { (void)fprintf(stderr, "%s: Cannot allowlist 'fchmod' for mode 0600: %s\n", __PROGRAM_NAME, strerror(errno)); (void)seccomp_release(ctx); exit(EXIT_FAILURE); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 21b2167..38eb4af 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -25,10 +25,12 @@ set(TEST_INCLUDES "${CMAKE_SOURCE_DIR}/src/C" # ============================================================================ set(HAVE_COVERAGE_TOOLS OFF) -if(CMAKE_C_COMPILER_ID STREQUAL "GNU") +# genhtml ships with lcov and is required by both toolchains, so search it +# once here (not only inside the GCC branch) so the Clang branch can use it. +find_program(LCOV_EXEC lcov) +find_program(GENHTML_EXEC genhtml) - find_program(LCOV_EXEC lcov) - find_program(GENHTML_EXEC genhtml) +if(CMAKE_C_COMPILER_ID STREQUAL "GNU") if(LCOV_EXEC AND GENHTML_EXEC) set(HAVE_COVERAGE_TOOLS ON) @@ -256,14 +258,26 @@ if(BUILD_TESTING AND HAVE_COVERAGE_TOOLS) elseif(CMAKE_C_COMPILER_ID MATCHES "Clang") - set(PROFILE_DATA ${CMAKE_BINARY_DIR}/default.profdata) - set_property(DIRECTORY APPEND PROPERTY ADDITIONAL_CLEAN_FILES ${PROFILE_DATA}) + set(PROFILE_DATA ${CMAKE_BINARY_DIR}/coverage/default.profdata) + # Instrumented processes write their sparse .profraw here. CTest changes + # into the subdirectory that holds its CTestTestfile.cmake, so a relative + # LLVM_PROFILE_FILE would land in build/test (not build) and the merge step + # below could not find it; use an absolute pattern instead. + set(COVERAGE_PROF_RAW ${CMAKE_BINARY_DIR}/coverage-prof) + set_property(DIRECTORY APPEND PROPERTY ADDITIONAL_CLEAN_FILES ${PROFILE_DATA} + ${COVERAGE_PROF_RAW}) add_custom_target( coverage - COMMAND ${CMAKE_COMMAND} -E env LLVM_PROFILE_FILE=default.profraw + COMMAND ${CMAKE_COMMAND} -E rm -rf ${COVERAGE_PROF_RAW} + COMMAND ${CMAKE_COMMAND} -E make_directory ${COVERAGE_PROF_RAW} + COMMAND ${CMAKE_COMMAND} -E make_directory ${COVERAGE_OUTPUT_DIR} + # %p-%m gives each process (and each child spawned under it, e.g. the + # instrumented client-keytab-name) a unique file so runs never clobber + # one another, and the absolute path makes the cwd irrelevant. + COMMAND ${CMAKE_COMMAND} -E env LLVM_PROFILE_FILE=${COVERAGE_PROF_RAW}/default-%p-%m.profraw ${CMAKE_CTEST_COMMAND} --output-on-failure - COMMAND ${LLVM_PROFDATA_EXEC} merge -sparse default.profraw -o ${PROFILE_DATA} + COMMAND ${LLVM_PROFDATA_EXEC} merge -sparse ${COVERAGE_PROF_RAW}/*.profraw -o ${PROFILE_DATA} COMMAND ${LLVM_COV_EXEC} show ${UNIT_TEST_BINARIES} -instr-profile=${PROFILE_DATA} -format=html -output-dir=${COVERAGE_OUTPUT_DIR} COMMAND ${LLVM_COV_EXEC} report ${UNIT_TEST_BINARIES}