From patchwork Sun Aug 3 06:22:54 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Shirish Pargaonkar X-Patchwork-Id: 4665591 Return-Path: X-Original-To: patchwork-cifs-client@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id F2C809F38D for ; Sun, 3 Aug 2014 06:23:18 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 09C3A2018A for ; Sun, 3 Aug 2014 06:23:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D6DB620155 for ; Sun, 3 Aug 2014 06:23:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751330AbaHCGW5 (ORCPT ); Sun, 3 Aug 2014 02:22:57 -0400 Received: from mail-lb0-f171.google.com ([209.85.217.171]:37656 "EHLO mail-lb0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750860AbaHCGW5 (ORCPT ); Sun, 3 Aug 2014 02:22:57 -0400 Received: by mail-lb0-f171.google.com with SMTP id l4so4332168lbv.2 for ; Sat, 02 Aug 2014 23:22:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=i00zyzg5o8MYNIQF3ETZl2dTdMLNNsLvoBInBj9WPPw=; b=W994xE30nHEHuMTdkJbumeyghGIZMhZKYYRICx61qZvPA7aHT7EtQwurDsSpJi19wJ +sFO/gZlcNe36tEuPU2ZNLPxdtz3l4FIQ8l4QuuMy+BL74L+4hvWqd0M+mpehbnH+OBV oXVrCQAEDsPkCxKjo2eq3glUqpTggNQ9QmKYtz8e36087MsDKgEq4yEfsRXsNkhPnt1s 72SbA8A4CafvqZerPw832dqjuUgXS3plBLVGCOukMOj2KJVxarOZomSVk41VDCXuLiR0 eiqQD8BhJNzOCPV68uz+YiiTZNf+g8U3bxtNI2AlPkVN3qTX/czDv6FbWDJQxz+fX0IM +jGw== MIME-Version: 1.0 X-Received: by 10.152.184.163 with SMTP id ev3mr79775lac.13.1407046974836; Sat, 02 Aug 2014 23:22:54 -0700 (PDT) Received: by 10.114.160.100 with HTTP; Sat, 2 Aug 2014 23:22:54 -0700 (PDT) In-Reply-To: <1407025666.1835.16.camel@joe-AO725> References: <1406413711-7798-1-git-send-email-rickard_strandqvist@spectrumdigital.se> <1407000807.1835.14.camel@joe-AO725> <1407025666.1835.16.camel@joe-AO725> Date: Sun, 3 Aug 2014 01:22:54 -0500 Message-ID: Subject: Re: [PATCH] fs: cifs: cifsencrypt.c: Cleaning up missing null-terminate in conjunction with strncpy From: Shirish Pargaonkar To: Joe Perches Cc: Rickard Strandqvist , Steve French , linux-cifs , samba-technical , LKML Sender: linux-cifs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-cifs@vger.kernel.org X-Spam-Status: No, score=-7.5 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Joe, Thanks for watching out. I think this patch is not correct and think also the current LM authentication code is broken if the password length exceeds 14 characters. Reading from Chris Hertel's book (Chapter 15, 15.3.3 Creating the LM Hash), it says: The LM Hash is a sixteen byte string, created as follows: 1. The password, as entered by the user, is either padded with nuls (0x00) or trimmed to fourteen (14) bytes. * Note that the 14-byte password string is not handled as null-terminated string. If the user enters 14 or more bytes, the last byte in the modified string will not be nul. * Also note the password is given in the 8-bit OEM character set (extended ASCII), not Unicode. 2. The 14-byte password string is converted to all uppercase. and so on.... So I thought a (quick) patch like this should be a fix instead but it does not work against a Samba server if the strlen(password) is more than 14. So there is work to be done in lanman auth code but this patch should not be included. So taking back the ack. On Sat, Aug 2, 2014 at 7:27 PM, Joe Perches wrote: > On Sun, 2014-08-03 at 01:13 +0200, Rickard Strandqvist wrote: >> 2014-08-02 19:33 GMT+02:00 Joe Perches : >> > On Sat, 2014-08-02 at 11:55 -0500, Shirish Pargaonkar wrote: >> >> Acked-by: Shirish Pargaonkar >> > [] >> >> > diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c >> > [] >> >> > @@ -307,7 +307,7 @@ int calc_lanman_hash(const char *password, const char *cryptkey, bool encrypt, >> >> > >> >> > memset(password_with_pad, 0, CIFS_ENCPWD_SIZE); >> >> > if (password) >> >> > - strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE); >> >> > + strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE - 1); >> > >> > >> > Is this always correct? > [] >> Because password_with_pad gets set to all zeros above, the character, >> I do not guarantee a copy terminating null. >> Unless it is so that you do not want any terminating null. > > That's the question for Steve and cifs people. > --- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c index 4934347..aecb4a0 100644 --- a/fs/cifs/cifsencrypt.c +++ b/fs/cifs/cifsencrypt.c @@ -307,7 +307,8 @@ int calc_lanman_hash(const char *password, const char *crypt memset(password_with_pad, 0, CIFS_ENCPWD_SIZE); if (password) - strncpy(password_with_pad, password, CIFS_ENCPWD_SIZE); + strncpy(password_with_pad, password, + min_t(unsigned int, strlen(password), 14)); if (!encrypt && global_secflags & CIFSSEC_MAY_PLNTXT) { memcpy(lnm_session_key, password_with_pad,