From owner-freebsd-current@FreeBSD.ORG Fri Jul 13 22:14:26 2012 Return-Path: Delivered-To: current@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id AD7B8106566C; Fri, 13 Jul 2012 22:14:26 +0000 (UTC) (envelope-from gibbs@FreeBSD.org) Received: from aslan.scsiguy.com (mail.scsiguy.com [70.89.174.89]) by mx1.freebsd.org (Postfix) with ESMTP id 459148FC0A; Fri, 13 Jul 2012 22:14:26 +0000 (UTC) Received: from [192.168.6.131] (207-225-98-3.dia.static.qwest.net [207.225.98.3]) (authenticated bits=0) by aslan.scsiguy.com (8.14.5/8.14.5) with ESMTP id q6DMEPYC051819 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Fri, 13 Jul 2012 16:14:25 -0600 (MDT) (envelope-from gibbs@FreeBSD.org) From: "Justin T. Gibbs" Content-Type: multipart/mixed; boundary="Apple-Mail=_410A4854-04E8-4B9A-B54B-38F6603F628C" Date: Fri, 13 Jul 2012 16:14:17 -0600 Message-Id: To: current@FreeBSD.org Mime-Version: 1.0 (Apple Message framework v1278) X-Mailer: Apple Mail (2.1278) X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (aslan.scsiguy.com [70.89.174.89]); Fri, 13 Jul 2012 16:14:25 -0600 (MDT) Cc: des@FreeBSD.org Subject: PAM passwdqc, strict aliasing, and WARNS X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Jul 2012 22:14:26 -0000 --Apple-Mail=_410A4854-04E8-4B9A-B54B-38F6603F628C Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=us-ascii Someone who has yet to confess added -Werror to the global CFLAGS (via /etc/make.conf) for one of our systems at work. Before I figured out that this was the cause of builds failing, I hacked up pam_passwdc to resolve the problem. This gets the module to WARNS=2, but to go farther, the "logically const" issues with this code will need to be sorted out. Is this change worth committing? Is this the best way to resolve the strict aliasing issues in this code? Thanks, Justin --Apple-Mail=_410A4854-04E8-4B9A-B54B-38F6603F628C Content-Disposition: attachment; filename=pam_passwdqc.diff Content-Type: application/octet-stream; name="pam_passwdqc.diff" Content-Transfer-Encoding: 7bit Change 575701 by justing@justing_ns1_spectrabsd on 2012/07/13 15:57:57 Make the PAM password strength checking module WARNS=2 safe. lib/libpam/modules/pam_passwdqc/Makefile: Bump WARNS to 2. contrib/pam_modules/pam_passwdqc/pam_passwdqc.c: Bump _XOPEN_SOURCE and _XOPEN_VERSION from 500 to 600 so that vsnprint() is declared. Use the two new union types (pam_conv_item_t and pam_text_item_t) to resolve strict aliasing violations caused by casts to comply with the pam_get_item() API taking a "const void **" for all item types. Correct a CLANG "printf-like function with more arguments than format" warning. Affected files ... ... //SpectraBSD/stable/contrib/pam_modules/pam_passwdqc/pam_passwdqc.c#2 edit ... //SpectraBSD/stable/lib/libpam/modules/pam_passwdqc/Makefile#2 edit Differences ... ==== //SpectraBSD/stable/contrib/pam_modules/pam_passwdqc/pam_passwdqc.c#2 (text) ==== @@ -2,9 +2,9 @@ * Copyright (c) 2000-2002 by Solar Designer. See LICENSE. */ -#define _XOPEN_SOURCE 500 +#define _XOPEN_SOURCE 600 #define _XOPEN_SOURCE_EXTENDED -#define _XOPEN_VERSION 500 +#define _XOPEN_VERSION 600 #include #include #include @@ -38,7 +38,16 @@ #else #define lo_const const #endif -typedef lo_const void *pam_item_t; + +typedef union { + struct pam_conv *conv; + lo_const void *item; +} pam_conv_item_t; + +typedef union { + char *text; + lo_const void *item; +} pam_text_item_t; #include "passwdqc.h" @@ -132,21 +141,21 @@ static int converse(pam_handle_t *pamh, int style, lo_const char *text, struct pam_response **resp) { - struct pam_conv *conv; + pam_conv_item_t conv; struct pam_message msg, *pmsg; int status; - status = pam_get_item(pamh, PAM_CONV, (pam_item_t *)&conv); + status = pam_get_item(pamh, PAM_CONV, &conv.item); if (status != PAM_SUCCESS) return status; pmsg = &msg; msg.msg_style = style; - msg.msg = text; + msg.msg = (char *)text; *resp = NULL; - return conv->conv(1, (lo_const struct pam_message **)&pmsg, resp, - conv->appdata_ptr); + return conv.conv->conv(1, (lo_const struct pam_message **)&pmsg, resp, + conv.conv->appdata_ptr); } #ifdef __GNUC__ @@ -294,8 +303,11 @@ } if (argc) { - say(pamh, PAM_ERROR_MSG, getuid() != 0 ? - MESSAGE_MISCONFIGURED : MESSAGE_INVALID_OPTION, *argv); + if (getuid() != 0) { + say(pamh, PAM_ERROR_MSG, MESSAGE_MISCONFIGURED); + } else { + say(pamh, PAM_ERROR_MSG, MESSAGE_INVALID_OPTION, *argv); + } return PAM_ABORT; } @@ -311,7 +323,7 @@ #ifdef HAVE_SHADOW struct spwd *spw; #endif - char *user, *oldpass, *newpass, *randompass; + pam_text_item_t user, oldpass, newpass, randompass; const char *reason; int ask_oldauthtok; int randomonly, enforce, retries_left, retry_wanted; @@ -353,34 +365,34 @@ if (flags & PAM_PRELIM_CHECK) return status; - status = pam_get_item(pamh, PAM_USER, (pam_item_t *)&user); + status = pam_get_item(pamh, PAM_USER, &user.item); if (status != PAM_SUCCESS) return status; - status = pam_get_item(pamh, PAM_OLDAUTHTOK, (pam_item_t *)&oldpass); + status = pam_get_item(pamh, PAM_OLDAUTHTOK, &oldpass.item); if (status != PAM_SUCCESS) return status; if (params.flags & F_NON_UNIX) { pw = &fake_pw; - pw->pw_name = user; + pw->pw_name = user.text; pw->pw_gecos = ""; } else { - pw = getpwnam(user); + pw = getpwnam(user.text); endpwent(); if (!pw) return PAM_USER_UNKNOWN; if ((params.flags & F_CHECK_OLDAUTHTOK) && getuid() != 0) { - if (!oldpass) + if (!oldpass.text) status = PAM_AUTH_ERR; else #ifdef HAVE_SHADOW if (!strcmp(pw->pw_passwd, "x")) { - spw = getspnam(user); + spw = getspnam(user.text); endspent(); if (spw) { - if (strcmp(crypt(oldpass, spw->sp_pwdp), - spw->sp_pwdp)) + if (strcmp(crypt(oldpass.text, + spw->sp_pwdp), spw->sp_pwdp)) status = PAM_AUTH_ERR; memset(spw->sp_pwdp, 0, strlen(spw->sp_pwdp)); @@ -388,7 +400,7 @@ status = PAM_AUTH_ERR; } else #endif - if (strcmp(crypt(oldpass, pw->pw_passwd), + if (strcmp(crypt(oldpass.text, pw->pw_passwd), pw->pw_passwd)) status = PAM_AUTH_ERR; } @@ -405,13 +417,14 @@ enforce = params.flags & F_ENFORCE_ROOT; if (params.flags & F_USE_AUTHTOK) { - status = pam_get_item(pamh, PAM_AUTHTOK, - (pam_item_t *)&newpass); + status = pam_get_item(pamh, PAM_AUTHTOK, &newpass.item); if (status != PAM_SUCCESS) return status; - if (!newpass || (check_max(¶ms, pamh, newpass) && enforce)) + if (!newpass.text || + (check_max(¶ms, pamh, newpass.text) && enforce)) return PAM_AUTHTOK_ERR; - reason = _passwdqc_check(¶ms.qc, newpass, oldpass, pw); + reason = _passwdqc_check(¶ms.qc, newpass.text, + oldpass.text, pw); if (reason) { say(pamh, PAM_ERROR_MSG, MESSAGE_WEAKPASS, reason); if (enforce) @@ -457,13 +470,13 @@ return status; } - randompass = _passwdqc_random(¶ms.qc); - if (randompass) { + randompass.text = _passwdqc_random(¶ms.qc); + if (randompass.text) { status = say(pamh, PAM_TEXT_INFO, randomonly ? - MESSAGE_RANDOMONLY : MESSAGE_RANDOM, randompass); + MESSAGE_RANDOMONLY : MESSAGE_RANDOM, randompass.text); if (status != PAM_SUCCESS) { - _pam_overwrite(randompass); - randompass = NULL; + _pam_overwrite(randompass.text); + randompass.text = NULL; } } else if (randomonly) { @@ -477,29 +490,30 @@ status = PAM_AUTHTOK_ERR; if (status != PAM_SUCCESS) { - if (randompass) _pam_overwrite(randompass); + if (randompass.text) _pam_overwrite(randompass.text); return status; } - newpass = strdup(resp->resp); + newpass.text = strdup(resp->resp); _pam_drop_reply(resp, 1); - if (!newpass) { - if (randompass) _pam_overwrite(randompass); + if (!newpass.text) { + if (randompass.text) _pam_overwrite(randompass.text); return PAM_AUTHTOK_ERR; } - if (check_max(¶ms, pamh, newpass) && enforce) { + if (check_max(¶ms, pamh, newpass.text) && enforce) { status = PAM_AUTHTOK_ERR; retry_wanted = 1; } reason = NULL; if (status == PAM_SUCCESS && - (!randompass || !strstr(newpass, randompass)) && + (!randompass.text || !strstr(newpass.text, randompass.text)) && (randomonly || - (reason = _passwdqc_check(¶ms.qc, newpass, oldpass, pw)))) { + (reason = _passwdqc_check(¶ms.qc, newpass.text, + oldpass.text, pw)))) { if (randomonly) say(pamh, PAM_ERROR_MSG, MESSAGE_NOTRANDOM); else @@ -515,7 +529,7 @@ PROMPT_NEWPASS2, &resp); if (status == PAM_SUCCESS) { if (resp && resp->resp) { - if (strcmp(newpass, resp->resp)) { + if (strcmp(newpass.text, resp->resp)) { status = say(pamh, PAM_ERROR_MSG, MESSAGE_MISTYPED); if (status == PAM_SUCCESS) { @@ -529,11 +543,11 @@ } if (status == PAM_SUCCESS) - status = pam_set_item(pamh, PAM_AUTHTOK, newpass); + status = pam_set_item(pamh, PAM_AUTHTOK, newpass.text); - if (randompass) _pam_overwrite(randompass); - _pam_overwrite(newpass); - free(newpass); + if (randompass.text) _pam_overwrite(randompass.text); + _pam_overwrite(newpass.text); + free(newpass.text); if (retry_wanted && --retries_left > 0) { status = say(pamh, PAM_TEXT_INFO, MESSAGE_RETRY); ==== //SpectraBSD/stable/lib/libpam/modules/pam_passwdqc/Makefile#2 (text) ==== @@ -7,7 +7,7 @@ SRCS= pam_passwdqc.c passwdqc_check.c passwdqc_random.c wordset_4k.c MAN= pam_passwdqc.8 -WARNS?= 0 +WARNS?= 2 CFLAGS+= -I${SRCDIR} DPADD= ${LIBCRYPT} --Apple-Mail=_410A4854-04E8-4B9A-B54B-38F6603F628C--