From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-6.0 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD shortcircuit=no autolearn=ham autolearn_force=no version=3.4.0 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id C2FFE20441 for ; Mon, 16 Jan 2017 20:33:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750981AbdAPUdM (ORCPT ); Mon, 16 Jan 2017 15:33:12 -0500 Received: from bsmtp3.bon.at ([213.33.87.17]:49589 "EHLO bsmtp3.bon.at" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750863AbdAPUdM (ORCPT ); Mon, 16 Jan 2017 15:33:12 -0500 Received: from dx.site (unknown [93.83.142.38]) by bsmtp3.bon.at (Postfix) with ESMTPSA id 3v2Q0j39mpz5tlF; Mon, 16 Jan 2017 21:33:08 +0100 (CET) Received: from [IPv6:::1] (localhost [IPv6:::1]) by dx.site (Postfix) with ESMTP id 1B911FA1; Mon, 16 Jan 2017 21:33:08 +0100 (CET) Subject: Re: What's cooking in git.git (Jan 2017, #02; Sun, 15) To: Johannes Schindelin , Jeff King References: <257b4175-9879-7814-5d8d-02050792574d@kdbg.org> <20170116160456.ltbb7ofe47xos7xo@sigill.intra.peff.net> Cc: Junio C Hamano , git@vger.kernel.org From: Johannes Sixt Message-ID: <677a335f-889c-2924-b7bd-93c2b6663175@kdbg.org> Date: Mon, 16 Jan 2017 21:33:07 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Am 16.01.2017 um 18:06 schrieb Johannes Schindelin: > On Mon, 16 Jan 2017, Jeff King wrote: >> Hmm. I am not sure to what degree CRLFs are actually a problem here. >> Keep in mind these are error messages generated via error(), and so not >> processing arbitrary data. I can imagine that CRs might come from: > > Please note the regression test I added. It uses rev-parse's --abbrev-ref > option which quotes the argument when erroring out. This argument then > gets munged. > > So error() (or in this case, die()) *very much* processes arbitrary data. > > I *know* that rev-parse --abbrev-ref is an artificial example, it is > highly unlikely that anybody will use > > git rev-parse --abbrev-ref "$( generates CR/LF line endings>)" > > However, there are plenty other cases in regular Git usage where arguments > are generated by external programs to which we have no business dictating a > specific line ending style. However, Jeff's patch is intended to catch exactly these cases (not for the cases where this happens accidentally, but when they happen with malicious intent). We are talking about user-provided data that is reproduced by die() or error(). I daresay that we do not have a single case where it is intended that this data is intentionally multi-lined, like a commit message. It can only be an accident or malicious when it spans across lines. I know we allow CR and LF in file names, but in all cases where such a name appears in an error message, it is *not important* that the data is reproduced exactly. On the contrary, it is usually more helpful to know that something strange is going on. The question marks are a strong indication to the user for this. > If you absolutely insist, I will spend time to find a plausible example > and use that in the regression test. I don't want to see you on an endeavor with dubious results. I'd prefer to wait until the first case of "incorrectly munged data" is reported because, as I said, I have a gut feeling that there is none. >> I am certainly open to loosening the sanitizing for CR to make things >> work seamlessly on Windows. But I am having trouble imagining a case >> that is actually negatively impacted. I came to the same conclusion. I regret having sent out a warning message in, well, such a haste(*), without thinking the case through first. IMHO, Jeff's patch should be fine as is. (*) literally; I had to catch a train. -- Hannes