From owner-freebsd-hackers@FreeBSD.ORG Sat Mar 21 20:05:06 2015 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id BDF0C8D7 for ; Sat, 21 Mar 2015 20:05:06 +0000 (UTC) Received: from mail-wg0-x22b.google.com (mail-wg0-x22b.google.com [IPv6:2a00:1450:400c:c00::22b]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CC1B2EB for ; Sat, 21 Mar 2015 20:05:06 +0000 (UTC) Received: by wgs2 with SMTP id 2so8523905wgs.1 for ; Sat, 21 Mar 2015 13:05:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=qs4lH2w9gRWC/Dd/SfOYYuGJL94AJa78ItQtG7Uia/0=; b=cEmZmuZJL+Mud5VIMs3SgKB9iWbAp/3JwCfIY1mNgPJCvtOyOoQOzi5R1xt2J/daKD /8fDyWjOCBu7qKFc7BON8mG+tW6+Z4Gd6eXH0RwFys+EZy69RV0yC2nG70OD/pqArMrV Hu1jdiu87vSjC+5K8r53IZI4gjI/SRZI1kcWgscfRyNHq0OGGs5a9PstnuOZXwcG7INo lHcTqjOYKqsnrhP/La8bPzhoTye4KdUDoxA+5D+VGVC397oHbaWuZVRUzZoYaSD5hNYN I4yH6ucVkfGNm5M7uUrFnt+YGaiR6q7YqJiJ5xm72wXoc3cvtXLalLuPbnaIvmO+TWrT SVRw== X-Received: by 10.181.13.42 with SMTP id ev10mr6632843wid.69.1426968304035; Sat, 21 Mar 2015 13:05:04 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id lu13sm3683690wic.10.2015.03.21.13.05.02 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sat, 21 Mar 2015 13:05:03 -0700 (PDT) Date: Sat, 21 Mar 2015 21:05:00 +0100 From: Mateusz Guzik To: Tiwei Bie Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <20150321200500.GC14650@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Tiwei Bie , freebsd-hackers@freebsd.org References: <1426946345-67889-1-git-send-email-btw@mail.ustc.edu.cn> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1426946345-67889-1-git-send-email-btw@mail.ustc.edu.cn> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-hackers@freebsd.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 21 Mar 2015 20:05:06 -0000 On Sat, Mar 21, 2015 at 09:59:05PM +0800, Tiwei Bie wrote: > Hi, Mateusz! > > I have finished the task: Validate coredump format string [1]. > > Following is my patch: > > --- > sys/kern/kern_sig.c | 31 ++++++++++++++++++++++++++++--- > 1 file changed, 28 insertions(+), 3 deletions(-) > > diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c > index 8410d9d..52f05be 100644 > --- a/sys/kern/kern_sig.c > +++ b/sys/kern/kern_sig.c > @@ -3099,13 +3099,38 @@ static char corefilename[MAXPATHLEN] = {"%N.core"}; > static int > sysctl_kern_corefile(SYSCTL_HANDLER_ARGS) > { > - int error; > + char *format; > + int i, error; > + > + format = malloc(MAXPATHLEN, M_TEMP, M_WAITOK); > + > + sx_slock(&corefilename_lock); > + strncpy(format, corefilename, MAXPATHLEN); > + sx_sunlock(&corefilename_lock); > + > + error = sysctl_handle_string(oidp, format, MAXPATHLEN, req); > + if (error != 0 || strcmp(format, corefilename) == 0) > + goto out; > + > + for (i = 0; format[i] != '\0'; i++) { > + if (format[i] == '%') { > + char ch = format[++i]; > + if (ch != '%' && ch != 'H' && ch != 'I' && > + ch != 'N' && ch != 'P' && ch != 'U') { > + error = EINVAL; > + printf("Unknown format character %c in " > + "corename `%s'\n", ch, format); > + goto out; > + } > + } > + } Code traversing the string uses 'switch'. Any reason to deviate from that? It also uses log(LOG_ERR,) so why is printf is used here? corefilename can be also set with a bootloader tunable, so we have to validate what is being passed there and possibly reject it. When we know that the string we have set in corefilename is valid, there is no reason to have aforementioned log() in corefile_open(). As a side note 'I' more than once in the format is not really supported, so I would check for that too. > > sx_xlock(&corefilename_lock); > - error = sysctl_handle_string(oidp, corefilename, sizeof(corefilename), > - req); > + strncpy(corefilename, format, sizeof(corefilename)); > sx_xunlock(&corefilename_lock); > > +out: > + free(format, M_TEMP); > return (error); > } > SYSCTL_PROC(_kern, OID_AUTO, corefile, CTLTYPE_STRING | CTLFLAG_RWTUN | > -- > 2.1.2 > > [1] https://wiki.freebsd.org/JuniorJobs#Validate_coredump_format_string > > Best regards, > Tiwei Bie > -- Mateusz Guzik