From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS4713 221.184.0.0/13 X-Spam-Status: No, score=-1.1 required=3.0 tests=BAYES_00,BIGNUM_EMAILS, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=no autolearn_force=no version=3.4.2 Received: from neon.ruby-lang.org (neon.ruby-lang.org [221.186.184.75]) by dcvr.yhbt.net (Postfix) with ESMTP id D2BEE1F461 for ; Sun, 8 Sep 2019 03:36:12 +0000 (UTC) Received: from neon.ruby-lang.org (localhost [IPv6:::1]) by neon.ruby-lang.org (Postfix) with ESMTP id DA3D0120939; Sun, 8 Sep 2019 12:36:03 +0900 (JST) Received: from xtrwkhkc.outbound-mail.sendgrid.net (xtrwkhkc.outbound-mail.sendgrid.net [167.89.16.28]) by neon.ruby-lang.org (Postfix) with ESMTPS id A47BF12091A for ; Sun, 8 Sep 2019 12:36:00 +0900 (JST) Received: by filter0082p3las1.sendgrid.net with SMTP id filter0082p3las1-3529-5D747721-14 2019-09-08 03:36:01.64690679 +0000 UTC m=+385706.076029255 Received: from herokuapp.com (unknown [54.146.238.68]) by ismtpd0010p1iad1.sendgrid.net (SG) with ESMTP id S3bI1WRJTvaCSVI4oEfMew for ; Sun, 08 Sep 2019 03:36:01.548 +0000 (UTC) Date: Sun, 08 Sep 2019 03:36:01 +0000 (UTC) From: XrXr@users.noreply.github.com Message-ID: References: Mime-Version: 1.0 X-Redmine-MailingListIntegration-Message-Ids: 70392 X-Redmine-Project: ruby-trunk X-Redmine-Issue-Id: 16151 X-Redmine-Issue-Author: alanwu X-Redmine-Sender: alanwu X-Mailer: Redmine X-Redmine-Host: bugs.ruby-lang.org X-Redmine-Site: Ruby Issue Tracking System X-Auto-Response-Suppress: All Auto-Submitted: auto-generated X-SG-EID: =?us-ascii?Q?PWg67P6owy8ojUUZg1G=2FQM4Z0jTQ2XLCqLM8Y2L8tUvRFuUPM6+aKANjRFY5CJ?= =?us-ascii?Q?rIOLvFElk1tV4+eC1eNonGNlIBBX77Zn9RVAMVg?= =?us-ascii?Q?SIOmaWpGFyKObxsRVODMGs5ked3Quc5kGflwReP?= =?us-ascii?Q?QpzfEgL3dENmDFOmmZlbslZoZZK2n1LnxtmF2Zw?= =?us-ascii?Q?5mmZKSe9woNvIF5p=2F0WpTp3c3XsD17yKnyA=3D=3D?= To: ruby-core@ruby-lang.org X-ML-Name: ruby-core X-Mail-Count: 94837 Subject: [ruby-core:94837] [Ruby master Bug#16151] [PATCH] Fix a class of fstring related use-after-free X-BeenThere: ruby-core@ruby-lang.org X-Mailman-Version: 2.1.15 Precedence: list Reply-To: Ruby developers List-Id: Ruby developers List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ruby-core-bounces@ruby-lang.org Sender: "ruby-core" Issue #16151 has been updated by alanwu (Alan Wu). My reading is that `STR_NOFREE` exists solely for strings that point to C string literals. It exists to prevent `free("literal")` which is UB. There are checks for `STR_NOFREE` everywhere buffers are freed in case a static string shows up. For heap allocated buffers, someone will eventually have to free them, so setting `STR_NOFREE` on strings with heap buffers surely leads to a leak. The only way to find out whether a string is a shared root is by traversing the entire heap. This is problematic for the call to `str_replace_shared_without_enc` in `rb_fstring` as it doesn't have enough information to decide whether to free a string or not. Not freeing anything, like we did before #15916, leads to leaks. Freeing with limited information leads to use-after-free situations. The semantics for `STR_IS_SHARED_M` is a bit weird to me too. I think it's a mechanism for detecting whether it's safe to free a shared root. When `rb_str_tmp_frozen_acquire` returns a shared root, the root has `STR_IS_SHARED_M` set in all situations where it's unsafe for `rb_str_tmp_frozen_release` to recycle the root. It's not quite "a root that has multiple dependants" since you could have a root that only has one dependant, but has `STR_IS_SHARED_M` set anyways. This could happen through this code path: https://github.com/ruby/ruby/blob/e9bc8b35c6c860e227627e3b0aab86b4f51c59af/string.c#L1291 When `rb_str_tmp_frozen_release` detects that it's safe to recycle a shared root, it undos the sharing. This is done by basically doing the reverse of https://github.com/ruby/ruby/blob/e9bc8b35c6c860e227627e3b0aab86b4f51c59af/string.c#L1303-L1312. I think this is why `STR_NOFREE` is involved there. My hunch is that there are ways to simplify away `STR_IS_SHARED_M` without compromising on performance. It feels unnecessarily complicated to me. Luckily `STR_IS_SHARED_M` doesn't come in to play for Strings one could easily create in Ruby land... I agree that there should be more documentation around the String's internal states and invariants. It would also be great if we could also rename some struct members and variables. For example, `STR_SHARED`'s name is particularly unintuitive to me. A shared root is "shared", but it should not have `STR_SHARED` set. ---------------------------------------- Bug #16151: [PATCH] Fix a class of fstring related use-after-free https://bugs.ruby-lang.org/issues/16151#change-81457 * Author: alanwu (Alan Wu) * Status: Open * Priority: Normal * Assignee: * Target version: * ruby -v: ruby 2.7.0dev (2019-09-07T18:26:35Z master e9bc8b35c6) [x86_64-linux] * Backport: 2.5: UNKNOWN, 2.6: UNKNOWN ---------------------------------------- Pull request: https://github.com/ruby/ruby/pull/2435 ## The bug Run the following against master(e9bc8b3) to observe use-after-free: ```ruby -('a' * 30).force_encoding(Encoding::ASCII) a = ('a' * 30).force_encoding(Encoding::ASCII).taint t = Thread.new{} t.name = a eval('', binding, t.name) p a ``` ```ruby -('a' * 30).force_encoding(Encoding::ASCII) a = ('a' * 30).force_encoding(Encoding::ASCII).taint require 'ripper' ripper = Ripper.new("", a) eval('', binding, ripper.filename) p a ``` There may be other cases in the standard library or in the wild. ## Background When a string has both `STR_NOEMBED` and `STR_SHARED` set, it relies on a different string for its buffer. I will refer to strings that are depended upon as "shared roots". Shared roots are frozen and have the `STR_SHARED` unset. This is a bit unintuitive to me. A name for `STR_SHARED` that makes more sense to me would be `STR_BUFFER_ELSEWHERE`. ## What went wrong It is not safe to free the buffer of a shared root while it has dependants. The root and its dependants use the same buffer. As such, it is only safe to free the shared buffer when all users are unreachable on the heap. ## The Fix `rb_fstring` has a code path that frees and replaces the buffer of its input. Using this code path on the shared root of dependant strings sets up use-after-free. This patch removes the problematic code path as no tests require said buffer replacement functionality. Additionally, there has been three other issues that steam from this particular code path. See #15926, #15916 and #16136 --- I used @mame's commit in #16136 as the starting point for this investigation. Thank you! -- https://bugs.ruby-lang.org/