From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25319 invoked by alias); 8 Jun 2018 02:08:19 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 25298 invoked by uid 89); 8 Jun 2018 02:08:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=regard, conscious, Correct, pay X-HELO: mail-qk0-f195.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=UAGKQPNpqQYA3hRbs58c+PcJNOGZU3zICVIkgr6C5ec=; b=BQZuSgG8jKNyexLV3SRcFpcQJU8udLVjCqsxV1/SYMx7+QtTIeUmGLrRw+UvMI7+y/ oDDVCFLh30oaBMvpZyyJzCQrW4vWDQzwVbOkTRSqQAN4c7zAeJoCaNe7THw9oE/uu7Nv UB3OS4TfposbjWfeeDAWg3sY5GcxeIA7NaNkUFQe2xb6GMc+hF6hznkB+DVzeAiyQwLj B321AUGW4CctEgmC+FPY3DWqwFxjZV2seTrRYqiX/bPxBp8wmFwGwRPVtWaUa25CrrM5 VK1WhLStSLmh+3gfA71JiZGRd/6+jc2syJ31TAZFyXbSkJQMbb7to3W5lP0kiZicc3sT 0JgA== X-Gm-Message-State: APt69E10HMrEKzR4NP3E45pvH7PT7YNaNhTah5nAS8gBzoEC/oQOMsoW 7nsf5qCMfSj2xVY/9343uCEcSg== X-Google-Smtp-Source: ADUXVKKSSmB4xI4osQ5UDGTQ9carT/DQo2U4hXwG6tE1eSg3qfPOPu/CZdLdntmBHAYjgNCFjmVeaw== X-Received: by 2002:a37:4d90:: with SMTP id a138-v6mr3729101qkb.139.1528423695995; Thu, 07 Jun 2018 19:08:15 -0700 (PDT) Subject: Re: [PATCH] Improve DST handling (Bug 23102, Bug 21942, Bug 18018, Bug, 23259, CVE-2011-0536 ). To: Andreas Schwab Cc: GNU C Library , "Dmitry V. Levin" , Florian Weimer References: <9cf43cb6-511c-ec6c-9a87-e89a467238d9@redhat.com> <7f17e96d-383d-ead5-deea-1b951513caba@redhat.com> From: Carlos O'Donell Message-ID: <577ead85-e6d9-bf21-d568-edabf3670cea@redhat.com> Date: Fri, 08 Jun 2018 02:08:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-06/txt/msg00150.txt.bz2 On 06/07/2018 02:48 AM, Andreas Schwab wrote: > On Jun 06 2018, Carlos O'Donell wrote: > >> On 06/06/2018 12:30 PM, Andreas Schwab wrote: >>> On Jun 06 2018, Carlos O'Donell wrote: >>> >>>> + /* Find longest valid input sequence. */ >>>> + ilen = 0; >>>> + while ((input[ilen] >= 'A' && input[ilen] <= 'Z') >>>> + || (input[ilen] >= 'a' && input[ilen] <= 'z') >>>> + || (input[ilen] >= '0' && input[ilen] <= '9') >>>> + || (input[ilen] == '_')) >>>> + ++ilen; >>>> + >>>> + rlen = strlen (ref); >>>> + >>>> + /* Can't be the DST we are looking for. */ >>>> + if (rlen != ilen) >>>> + return 0; >>> >>> Why do you need that? Just compare, then check the next character. >> >> Are you suggesting that: >> ~~~ >> rlen = strlen (ref); >> >> /* Can't be the DST we are looking for. */ >> if (rlen != ilen) >> return 0; >> ~~~ >> Can be dropped because we are going to compare the strings anyway? > > Drop the whole part. When you have compared the string you know that it > is valid so far, so what's the value of validating it twice? Careful, is_dst () takes as input the start of a DST sequence, but that sequence is not validated yet. We must compare the longest legal sequence with that of the DST comparison. This is different from comparing *both* sequences. The old glibc code was wrong in this regard. In the old code we had: len = 0; while (name[len] == str[len] && name[len] != '\0') ++len; What if name was '$ORIGIN-' but str was '$ORIGIN'? With the above code we reject '$ORIGIN-' != '$ORIGIN', but this is not true. I have added a test for this in my test case: With system glibc: Test 1e: Test that RPATH of $ORIGIN- loads from /root- as expected. FAIL: Wrong primary library. With patched glibc: Test 1e: Test that RPATH of $ORIGIN- loads from /root- as expected. 24538: expand_dynamic_string_token input=$ORIGIN-, start=$ORIGIN- 24538: _dl_dst_count $ORIGIN- (7f7934f38d20) 24538: is_dst input=ORIGIN- (begin) 24538: is_dst ilen=6 rlen=6 24538: is_dst ilen=6 input=ORIGIN- (7f7934f38d21) (ref=ORIGIN) 24538: _dl_dst_count next - 24538: _dl_dst_count cnt 1 24538: DL_DST_REQUIRED l_origin 7f7934f38d40 24538: _dl_dst_substitute input=$ORIGIN- start=$ORIGIN- 24538: is_dst input=ORIGIN- (begin) 24538: is_dst ilen=6 rlen=6 24538: is_dst ilen=6 input=ORIGIN- (7f7934f38d21) (ref=ORIGIN) 24538: _dl_dst_substitute 7f7934f38d40 24538: security = 0 24538: security = 0 24538: security = 0 24538: security = 0 PASS: Correct primary library. The above code also makes is_dst () robust against mistakes if the caller violates the ELF gABI specification by trying to use a DST which doesn't conform to the [A-Za-z0-9_] requirement. To do that the code does: * Follow the ELF gABI rules to get the longest name. * Compare the longest name to the DST we are looking to find. If we didn't do the "longest name" computation we might allow both glibc to use invalid DST names, and applications to get away with using them without error and have them work. With my patch the developer would have to knowingly adjust the allowed longest name sequence, and that's a conscious change which is good to avoid mistakes. There is a bit of a performance cost to pay here, but I've not seen any argument that this code is highly performance sensitive. If it is really performance sensitive then we can talk about many other ways to make it faster. Again, this is a change from previous behaviour, but brings us more sane parsing of the DSTs IMO. Therefore I don't think this code has to change. Cheers, Carlos.