From patchwork Fri Apr 23 06:17:13 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Scott Lovenberg X-Patchwork-Id: 50790 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.samba.org (fn.samba.org [216.83.154.106]) by ozlabs.org (Postfix) with ESMTP id 38A63B7D16 for ; Fri, 23 Apr 2010 16:17:27 +1000 (EST) Received: from fn.samba.org (localhost [127.0.0.1]) by lists.samba.org (Postfix) with ESMTP id C6004AD258; Fri, 23 Apr 2010 00:17:28 -0600 (MDT) X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on fn.samba.org X-Spam-Level: X-Spam-Status: No, score=-0.6 required=3.8 tests=AWL,BAYES_00, HEADER_COUNT_CTYPE, MISSING_MIME_HB_SEP, NO_MORE_FUNN, SPF_PASS autolearn=no version=3.2.5 X-Original-To: linux-cifs-client@lists.samba.org Delivered-To: linux-cifs-client@lists.samba.org Received: from mail-vw0-f41.google.com (mail-vw0-f41.google.com [209.85.212.41]) by lists.samba.org (Postfix) with ESMTP id 0543DAD22E for ; Fri, 23 Apr 2010 00:17:22 -0600 (MDT) Received: by mail-vw0-f41.google.com with SMTP id 4so2203917vws.14 for ; Thu, 22 Apr 2010 23:17:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:from:to:cc:subject:date :message-id:x-mailer:in-reply-to:references:content-type :content-transfer-encoding:mime-version:content-type; bh=NMblC4YCbzCr2dFXrxeEp1YeJWrx5aYRIREej6wJJ8M=; b=qMKSK2NUrw2UJ4B87eVDPzncJ8oMzpVfvouKxG8/K4kBz5q0f4AE0WKgAc1j6NZdtc tuIZdGsTfdXSSAMWvkroNQWOXDU0TsAd6hV2wz9zR1bfpKpwsyijy+tjPJ6Es81h9a1U rSC0sW4a0ktJPOC5uf6fNYA3chd50RKooLSSs= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references :content-type:content-transfer-encoding:mime-version; b=pAsHL9cA24UMZRCqMQ/m0aH5sQzvLX7P72AN4nNBen//uZ3Kiz5kGSH9e88Q652Hhs sJrgG3qvY2u41kdLa7wMZpZmL/s9tLNAgk2X+qRxTGwOqbguVa+S6IbVC3ZHjtPpRNd8 yje3ixdfHUonZ/xc8HfPMfnFWwcPiWc5brLZw= Received: by 10.220.61.201 with SMTP id u9mr7560551vch.40.1272003440402; Thu, 22 Apr 2010 23:17:20 -0700 (PDT) Received: from localhost.localdomain (24.115.161.116.res-cmts.flt.ptd.net [24.115.161.116]) by mx.google.com with ESMTPS id a1sm3482639vcp.21.2010.04.22.23.17.18 (version=TLSv1/SSLv3 cipher=RC4-MD5); Thu, 22 Apr 2010 23:17:19 -0700 (PDT) From: Scott Lovenberg To: linux-cifs-client@lists.samba.org Date: Fri, 23 Apr 2010 02:17:13 -0400 Message-Id: <1272003433-9426-2-git-send-email-scott.lovenberg@gmail.com> X-Mailer: git-send-email 1.6.2.5 In-Reply-To: <1272003433-9426-1-git-send-email-scott.lovenberg@gmail.com> References: <1272003433-9426-1-git-send-email-scott.lovenberg@gmail.com> MIME-Version: 1.0 Cc: jlayton@samba.org Subject: [linux-cifs-client] [PATCH 2/2] Continued cleanup of open_cred_file and fixed a potential security risk. X-BeenThere: linux-cifs-client@lists.samba.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: The Linux CIFS VFS client List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-cifs-client-bounces@lists.samba.org Errors-To: linux-cifs-client-bounces@lists.samba.org The parsing for values has been moved to its own function and is a bit cleaner, IMHO. With the parsing de-coupled from the open_cred_file, this should work on a broader scope (e.g. - Jeff's multimount?). Temporary buffers are zeroed out before being freed to ensure passwords/credentials aren't left in released memory. Signed-off-by: Scott Lovenberg --- mount.cifs.c | 141 ++++++++++++++++++++++++++++++++++++---------------------- 1 files changed, 87 insertions(+), 54 deletions(-) --------------1.6.2.5-- diff --git a/mount.cifs.c b/mount.cifs.c index 97dae82..cf79bce 100644 --- a/mount.cifs.c +++ b/mount.cifs.c @@ -121,6 +121,14 @@ */ #define CIFS_SETUID_FLAGS (MS_NOSUID|MS_NODEV) +/* + * Values for parsing a credentials file. + */ +#define CRED_UNPARSABLE 0 +#define CRED_USER 1 +#define CRED_PASS 2 +#define CRED_DOM 4 + /* struct for holding parsed mount info for use by privleged process */ struct parsed_mount_info { unsigned long flags; @@ -521,44 +529,64 @@ static void null_terminate_endl(char* source) *newline = '\0'; } - +/* + * Parse a line from the credentials file. Points 'target' to the first character + * after '=' on 'line' and returns the value type (CRED_) of the line. + * Returns CRED_UNPARSABLE on failure or if line is NULL. + */ +static int parse_cred_line(char* line, char* target) +{ + if (line == NULL) + goto parsing_err; + + /* position target at first char of value */ + target = strchr(line, '='); + if (!target) + goto parsing_err; + target++; + + if (strncasecmp("user", line, 4) == 0) + return CRED_USER; + if (strncasecmp("pass", line, 4) == 0) + return CRED_PASS; + if (strncasecmp("dom", line, 3) == 0) + return CRED_DOM; + + /* if we're here the line, or target, wasn't sane */ + parsing_err: + return CRED_UNPARSABLE; +} static int open_cred_file(char *file_name, struct parsed_mount_info *parsed_info) { char *line_buf; - char *temp_val; + char *temp_val = NULL; FILE *fs = NULL; int i; const int line_buf_size = 4096; const int min_non_white = 10; - i = toggle_dac_capability(0, 1); - if (i) - return i; - i = access(file_name, R_OK); - if (i) { + if ((i = toggle_dac_capability(0, 1))) + goto return_i; + if ((i = access(file_name, R_OK))) { toggle_dac_capability(0, 0); - return i; + goto return_i; } - fs = fopen(file_name, "r"); - if (fs == NULL) { + if ((fs = fopen(file_name, "r")) == NULL) { toggle_dac_capability(0, 0); - return errno; + i = errno; + goto return_i; } - i = toggle_dac_capability(0, 0); - if (i) { - fclose(fs); - return i; - } + if((i = toggle_dac_capability(0, 0))) + goto return_i; - line_buf = (char *)malloc(line_buf_size); - if (line_buf == NULL) { - fclose(fs); - return EX_SYSERR; + if ((line_buf = (char*)malloc(line_buf_size)) == NULL) { + i = EX_SYSERR; + goto return_i; } /* parse line from credentials file */ @@ -570,47 +598,52 @@ static int open_cred_file(char *file_name, } null_terminate_endl(line_buf); - /* parse user */ - if (strncasecmp("user", line_buf + i, 4) == 0) { - temp_val = strchr(line_buf + i, '='); - if (temp_val) { - /* go past equals sign */ - temp_val++; - parsed_info->got_user = 1; - strlcpy(parsed_info->username, temp_val, - sizeof(parsed_info->username)); - } - } - - /* parse password */ - else if (strncasecmp("pass", line_buf + i, 4) == 0) { - temp_val = strchr(line_buf + i, '='); - if (!temp_val) - continue; - ++temp_val; - i = set_password(parsed_info, temp_val); - if (i) - return i; - } - - /* parse domain */ - else if (strncasecmp("dom", line_buf + i, 3) == 0) { - temp_val = strchr(line_buf + i, '='); - if (temp_val) { - /* go past equals sign */ - temp_val++; + /* parse next token */ + switch(parse_cred_line(line_buf + i, temp_val)) { + /* user */ + case CRED_USER: + parse_username(temp_val, parsed_info); + break; + + /* password */ + case CRED_PASS: + if ((i = set_password(parsed_info, temp_val))) + goto return_i; + break; + + /* domain */ + case CRED_DOM: if (parsed_info->verboseflag) fprintf(stderr, "\nDomain %s\n", temp_val); strlcpy(parsed_info->domain, temp_val, sizeof(parsed_info->domain)); - } - } + break; + /* error */ + case CRED_UNPARSABLE: + if (parsed_info->verboseflag) + fprintf(stderr, + "Bad line in credentials file, ignoring"); + break; + + } } - fclose(fs); - SAFE_FREE(line_buf); - return 0; + /* if we've gotten to this line there are no error */ + i = 0; + + /* jump point for errors */ + return_i: + if (fs != NULL) + fclose(fs); + /* make sure passwords are scrubbed from memory */ + if (line_buf != NULL) + memset(line_buf, 0, line_buf_size); + if (temp_val != NULL) + memset(temp_val, 0, strlen(temp_val)); + SAFE_FREE(line_buf); + SAFE_FREE(temp_val); + return i; } static int