From a260a0ecd41318b1680527fefab5383c67d9e30d Mon Sep 17 00:00:00 2001 From: Gabriel Kihlman Date: Sat, 28 Dec 2019 21:20:14 +0100 Subject: [PATCH 01/10] Adding a static code analysis github workflow --- .github/workflows/scan.yml | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 .github/workflows/scan.yml diff --git a/.github/workflows/scan.yml b/.github/workflows/scan.yml new file mode 100644 index 00000000..65d6b0bc --- /dev/null +++ b/.github/workflows/scan.yml @@ -0,0 +1,32 @@ +name: static code analysis + +on: [push] +env: + SCAN_IMG: + yes-docker-local.artifactory.in.yubico.org/static-code-analysis/c:v1 + COMPILE_DEPS: "libfido2-dev" + +jobs: + build: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@master + + - name: Prep scan + run: | + docker login yes-docker-local.artifactory.in.yubico.org/ \ + -u svc-static-code-analysis-reader \ + -p ${{ secrets.ARTIFACTORY_READER_TOKEN }} + docker pull ${SCAN_IMG} + + - name: Scan but do not fail on warnings + run: | + docker run -v${PWD}:/k -e COMPILE_DEPS="${COMPILE_DEPS}" \ + -e PROJECT_NAME=${GITHUB_REPOSITORY#Yubico/} -t ${SCAN_IMG} || true + + - uses: actions/upload-artifact@master + if: failure() + with: + name: suppression_files + path: suppression_files From 5d76e3028a39b668aee27ac4fd08bfe3c2bce5e7 Mon Sep 17 00:00:00 2001 From: Gabriel Kihlman Date: Fri, 17 Jan 2020 15:58:11 +0100 Subject: [PATCH 02/10] Re-schedule scans every week o Point to documentation for the scanner o Use continue-on-error: true instead of || true --- .github/workflows/scan.yml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/scan.yml b/.github/workflows/scan.yml index 65d6b0bc..3eb7c8dd 100644 --- a/.github/workflows/scan.yml +++ b/.github/workflows/scan.yml @@ -1,6 +1,11 @@ name: static code analysis +# Documentation: https://github.com/Yubico/yes-static-code-analysis + +on: + push: + schedule: + - cron: '0 0 * * 1' -on: [push] env: SCAN_IMG: yes-docker-local.artifactory.in.yubico.org/static-code-analysis/c:v1 @@ -23,7 +28,8 @@ jobs: - name: Scan but do not fail on warnings run: | docker run -v${PWD}:/k -e COMPILE_DEPS="${COMPILE_DEPS}" \ - -e PROJECT_NAME=${GITHUB_REPOSITORY#Yubico/} -t ${SCAN_IMG} || true + -e PROJECT_NAME=${GITHUB_REPOSITORY#Yubico/} -t ${SCAN_IMG} + continue-on-error: true - uses: actions/upload-artifact@master if: failure() From 52db556be0943fcf488904e407ac4d22e4a77b39 Mon Sep 17 00:00:00 2001 From: Gabriel Kihlman Date: Mon, 2 Mar 2020 14:56:43 +0100 Subject: [PATCH 03/10] Check explicitly for FIDO_OPT_TRUE --- util.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/util.c b/util.c index 05645866..b64e7c00 100644 --- a/util.c +++ b/util.c @@ -730,7 +730,8 @@ int do_authentication(const cfg_t *cfg, const device_t *devices, if (pin_verification == FIDO_OPT_TRUE) pin = converse(pamh, PAM_PROMPT_ECHO_OFF, "Please enter the PIN: "); - if (user_presence || user_verification) { + if (user_presence == FIDO_OPT_TRUE || + user_verification == FIDO_OPT_TRUE) { if (cfg->manual == 0 && cfg->cue && !cued) { cued = 1; converse(pamh, PAM_TEXT_INFO, From 870fe34386248e7128983e351d64bd1cec9a14f0 Mon Sep 17 00:00:00 2001 From: Christos Zoulas Date: Tue, 3 Mar 2020 09:43:44 -0500 Subject: [PATCH 04/10] Avoid const cast-away --- b64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/b64.c b/b64.c index 3a3e90f5..0649c1e3 100644 --- a/b64.c +++ b/b64.c @@ -75,7 +75,7 @@ int b64_decode(const char *in, void **ptr, size_t *len) { if (bio_b64 == NULL) goto fail; - bio_mem = BIO_new_mem_buf((void *) in, -1); + bio_mem = BIO_new_mem_buf((const void *) in, -1); if (bio_mem == NULL) goto fail; From eb707e1f553ee07b06f3405aca68880a4baa7401 Mon Sep 17 00:00:00 2001 From: Christos Zoulas Date: Tue, 3 Mar 2020 09:44:19 -0500 Subject: [PATCH 05/10] - cast -1 to the appropriate types - split lines --- drop_privs.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drop_privs.h b/drop_privs.h index b2a886bd..a5256dbd 100644 --- a/drop_privs.h +++ b/drop_privs.h @@ -55,8 +55,13 @@ struct _ykpam_privs { #define PAM_MODUTIL_DEF_PRIVS(n) \ gid_t n##_saved_groups[SAVED_GROUPS_MAX_LEN]; \ - struct _ykpam_privs n = {-1, -1, n##_saved_groups, SAVED_GROUPS_MAX_LEN, \ - cfg->debug_file} + struct _ykpam_privs n = { \ + (uid_t)-1, \ + (gid_t)-1, \ + n##_saved_groups, \ + SAVED_GROUPS_MAX_LEN, \ + cfg->debug_file, \ + } int pam_modutil_drop_priv(pam_handle_t *, struct _ykpam_privs *, struct passwd *); From 45e79d69300ebb4f7e264e5b49ba80633214470d Mon Sep 17 00:00:00 2001 From: Christos Zoulas Date: Tue, 3 Mar 2020 09:45:02 -0500 Subject: [PATCH 06/10] - move PAM_MODUTIL_DEF_PRIVS below parse_cfg, so that cfg->debug_file gets initialized first (this is c99, statement before decl). - introduce free_const() macro to avoid const cast-away warnings. - Add PAM_MODULE_ENTRY() macro used by OpenPAM. --- pam-u2f.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/pam-u2f.c b/pam-u2f.c index 40cf40c3..6a29f413 100644 --- a/pam-u2f.c +++ b/pam-u2f.c @@ -168,10 +168,11 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, int should_free_appid = 0; int should_free_auth_file = 0; int should_free_authpending_file = 0; - PAM_MODUTIL_DEF_PRIVS(privs); parse_cfg(flags, argc, argv, cfg); + PAM_MODUTIL_DEF_PRIVS(privs); + if (!cfg->origin) { strcpy(buffer, DEFAULT_ORIGIN_PREFIX); @@ -427,24 +428,24 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, free(buf); buf = NULL; } - +#define free_const(a) free((void *)(uintptr_t)(a)) if (should_free_origin) { - free((char *) cfg->origin); + free_const(cfg->origin); cfg->origin = NULL; } if (should_free_appid) { - free((char *) cfg->appid); + free_const(cfg->appid); cfg->appid = NULL; } if (should_free_auth_file) { - free((char *) cfg->auth_file); + free_const(cfg->auth_file); cfg->auth_file = NULL; } if (should_free_authpending_file) { - free((char *) cfg->authpending_file); + free_const(cfg->authpending_file); cfg->authpending_file = NULL; } @@ -470,3 +471,7 @@ PAM_EXTERN int pam_sm_setcred(pam_handle_t *pamh, int flags, int argc, return PAM_SUCCESS; } + +#ifdef PAM_MODULE_ENTRY +PAM_MODULE_ENTRY("pam_u2f"); +#endif From 3fea138d9a6dcdeab3e2068d86a0fad13b79a1cf Mon Sep 17 00:00:00 2001 From: Christos Zoulas Date: Tue, 3 Mar 2020 09:50:32 -0500 Subject: [PATCH 07/10] Provide an empty statement for non-debugging code. --- util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util.h b/util.h index 2ca61546..7dc0fcd2 100644 --- a/util.h +++ b/util.h @@ -25,7 +25,7 @@ #if defined(DEBUG_PAM) #define D(file, ...) _debug(file, __FILE__, __LINE__, __func__, __VA_ARGS__) #else -#define D(file, ...) +#define D(file, ...) ((void)0) #endif /* DEBUG_PAM */ typedef struct { From 1c5a8055c44f99f668eff0bb5b23fc7ac1f19283 Mon Sep 17 00:00:00 2001 From: Christos Zoulas Date: Tue, 3 Mar 2020 09:51:11 -0500 Subject: [PATCH 08/10] - explicitly check against NULL to avoid some compiler warnings - fix const cast-away. - Since we include unconditionally allow syslog use on other unixes, not just linux. We check that we provide a LOG_DEBUG macro instead. --- util.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/util.c b/util.c index b64e7c00..a4b95341 100644 --- a/util.c +++ b/util.c @@ -236,7 +236,7 @@ int get_devices_from_authfile(const char *authfile, const char *username, *n_devs = 0; i = 0; - while ((s_token = strtok_r(NULL, ",", &saveptr))) { + while ((s_token = strtok_r(NULL, ",", &saveptr)) != NULL) { if ((*n_devs)++ > max_devs - 1) { *n_devs = max_devs; if (verbose) @@ -1131,7 +1131,7 @@ static int _converse(pam_handle_t *pamh, int nargs, char *converse(pam_handle_t *pamh, int echocode, const char *prompt) { const struct pam_message msg = {.msg_style = echocode, - .msg = (char *) prompt}; + .msg = (char *)(uintptr_t)prompt}; const struct pam_message *msgs = &msg; struct pam_response *resp = NULL; int retval = _converse(pamh, 1, &msgs, &resp); @@ -1163,7 +1163,7 @@ void _debug(FILE *debug_file, const char *file, int line, const char *func, const char *fmt, ...) { va_list ap; va_start(ap, fmt); -#ifdef __linux__ +#ifdef LOG_DEBUG if (debug_file == (FILE *) -1) { syslog(LOG_AUTHPRIV | LOG_DEBUG, DEBUG_STR, file, line, func); vsyslog(LOG_AUTHPRIV | LOG_DEBUG, fmt, ap); From 8a3b1b0a513ea7e9dd7dfb3b3589c9eab9fe9457 Mon Sep 17 00:00:00 2001 From: Christos Zoulas Date: Wed, 4 Mar 2020 08:09:51 -0500 Subject: [PATCH 09/10] Include for intptr_t on linux --- pam-u2f.c | 1 + 1 file changed, 1 insertion(+) diff --git a/pam-u2f.c b/pam-u2f.c index 6a29f413..8eed511d 100644 --- a/pam-u2f.c +++ b/pam-u2f.c @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include From 781c31b05f7a741da49c9cea53a64998f3dc0484 Mon Sep 17 00:00:00 2001 From: Christos Zoulas Date: Wed, 4 Mar 2020 08:47:03 -0500 Subject: [PATCH 10/10] Appease the formatting gods. --- drop_privs.h | 9 +++------ pam-u2f.c | 4 ++-- util.c | 2 +- util.h | 2 +- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/drop_privs.h b/drop_privs.h index a5256dbd..03121e1f 100644 --- a/drop_privs.h +++ b/drop_privs.h @@ -55,12 +55,9 @@ struct _ykpam_privs { #define PAM_MODUTIL_DEF_PRIVS(n) \ gid_t n##_saved_groups[SAVED_GROUPS_MAX_LEN]; \ - struct _ykpam_privs n = { \ - (uid_t)-1, \ - (gid_t)-1, \ - n##_saved_groups, \ - SAVED_GROUPS_MAX_LEN, \ - cfg->debug_file, \ + struct _ykpam_privs n = { \ + (uid_t) -1, (gid_t) -1, n##_saved_groups, \ + SAVED_GROUPS_MAX_LEN, cfg->debug_file, \ } int pam_modutil_drop_priv(pam_handle_t *, struct _ykpam_privs *, diff --git a/pam-u2f.c b/pam-u2f.c index 8eed511d..ab1b85ca 100644 --- a/pam-u2f.c +++ b/pam-u2f.c @@ -429,7 +429,7 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, free(buf); buf = NULL; } -#define free_const(a) free((void *)(uintptr_t)(a)) +#define free_const(a) free((void *) (uintptr_t)(a)) if (should_free_origin) { free_const(cfg->origin); cfg->origin = NULL; @@ -474,5 +474,5 @@ PAM_EXTERN int pam_sm_setcred(pam_handle_t *pamh, int flags, int argc, } #ifdef PAM_MODULE_ENTRY -PAM_MODULE_ENTRY("pam_u2f"); +PAM_MODULE_ENTRY("pam_u2f"); #endif diff --git a/util.c b/util.c index a4b95341..77aeb88c 100644 --- a/util.c +++ b/util.c @@ -1131,7 +1131,7 @@ static int _converse(pam_handle_t *pamh, int nargs, char *converse(pam_handle_t *pamh, int echocode, const char *prompt) { const struct pam_message msg = {.msg_style = echocode, - .msg = (char *)(uintptr_t)prompt}; + .msg = (char *) (uintptr_t) prompt}; const struct pam_message *msgs = &msg; struct pam_response *resp = NULL; int retval = _converse(pamh, 1, &msgs, &resp); diff --git a/util.h b/util.h index 7dc0fcd2..6b41aa25 100644 --- a/util.h +++ b/util.h @@ -25,7 +25,7 @@ #if defined(DEBUG_PAM) #define D(file, ...) _debug(file, __FILE__, __LINE__, __func__, __VA_ARGS__) #else -#define D(file, ...) ((void)0) +#define D(file, ...) ((void) 0) #endif /* DEBUG_PAM */ typedef struct {