From patchwork Fri Jun 17 20:19:50 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Eric Dumazet X-Patchwork-Id: 100857 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 05406B6F72 for ; Sat, 18 Jun 2011 06:20:19 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932538Ab1FQUT6 (ORCPT ); Fri, 17 Jun 2011 16:19:58 -0400 Received: from mail-wy0-f174.google.com ([74.125.82.174]:40816 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756796Ab1FQUT4 (ORCPT ); Fri, 17 Jun 2011 16:19:56 -0400 Received: by wyb38 with SMTP id 38so555865wyb.19 for ; Fri, 17 Jun 2011 13:19:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:subject:from:to:cc:in-reply-to:references :content-type:date:message-id:mime-version:x-mailer :content-transfer-encoding; bh=ZQdYUJGQ8FAswjEYxUKT1on/3gAWZbKNjM0DeW3Ak/Y=; b=b2MdoZ8Exr97S4j9/4oshsK2ntTVToq4sv6m5emXWRU0GfwMvhK/UKkMdWTjApE3Zy kYSwWdmyGqugOs95XaKCx7vYOEEjm0T2+i8FQPenPq5ImP67MVifbe5Tj/GcL9dgWmym mToMbrUH8ssKiEgNZB47CcDX+ywPOFpqSlddo= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=subject:from:to:cc:in-reply-to:references:content-type:date :message-id:mime-version:x-mailer:content-transfer-encoding; b=c1aKSSiF8ALV/nAyEfwViQH0agb0muR4zUyhBDHEq9gPWs1EP/asj9ZQz8AZLluIja z2fkaRI3mSY0ln3j80CRpNUDJMJHlcVgTDvN9oviPmUiX6edEtq4dv3ReFfW7HDqeAzR JoKN4wxcSYX/k7o/ywTtNYGNUZfIpX4A6VJ1A= Received: by 10.227.43.9 with SMTP id u9mr2492953wbe.74.1308341995116; Fri, 17 Jun 2011 13:19:55 -0700 (PDT) Received: from [10.150.51.218] (gw0.net.jmsp.net [212.23.165.14]) by mx.google.com with ESMTPS id fl19sm1217437wbb.32.2011.06.17.13.19.52 (version=SSLv3 cipher=OTHER); Fri, 17 Jun 2011 13:19:53 -0700 (PDT) Subject: [PATCH] inet_diag: fix inet_diag_bc_audit() From: Eric Dumazet To: Eugene Teo Cc: Dan Rosenberg , davem@davemloft.net, kuznet@ms2.inr.ac.ru, netdev@vger.kernel.org, security@kernel.org, Arnaldo Carvalho de Melo , Stephen Hemminger In-Reply-To: References: <1306942849.3150.10.camel@dan> Date: Fri, 17 Jun 2011 22:19:50 +0200 Message-ID: <1308341990.3539.27.camel@edumazet-laptop> Mime-Version: 1.0 X-Mailer: Evolution 2.32.2 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Le vendredi 03 juin 2011 à 14:55 +0800, Eugene Teo a écrit : > Cc'ed acme. > > On Wed, Jun 1, 2011 at 11:40 PM, Dan Rosenberg wrote: > > It seems to me that the auditing performed by inet_diag_bc_audit() is > > insufficient to prevent pathological INET_DIAG bytecode from doing bad > > things. > > > > Firstly, it's possible to cause an infinite loop in inet_diag_bc_audit() > > with a INET_DIAG_BC_JMP opcode with a "yes" value of 0. The valid_cc() > > function, also called from here, seems suspicious as well. > > > > Once the bytecode is actually run in inet_diag_bc_run(), it looks like > > more infinite loops are possible, if appropriate "yes" or "no" values > > are set to zero and weren't validated by the audit. > > > > Finally, I can't seem to find any validation that the reported length of > > the netlink message header doesn't exceed the skb length, as checked in > > some other netlink receive functions, which could result in reading > > beyond the bounds of the socket data. I could just be missing something > > here though. > > > > Regards, > > Dan > > Thanks guys, here is the patch I cooked to address this problem, sorry for the long delay again. [PATCH] inet_diag: fix inet_diag_bc_audit() A malicious user or buggy application can inject code and trigger an infinite loop in inet_diag_bc_audit() Also make sure each instruction is aligned on 4 bytes boundary, to avoid unaligned accesses. Reported-by: Dan Rosenberg Signed-off-by: Eric Dumazet CC: Stephen Hemminger CC: Arnaldo Carvalho de Melo CC: Eugene Teo CC: Alexey Kuznetsov --- net/ipv4/inet_diag.c | 14 ++++++-------- 1 files changed, 6 insertions(+), 8 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c index 6ffe94c..3267d38 100644 --- a/net/ipv4/inet_diag.c +++ b/net/ipv4/inet_diag.c @@ -437,7 +437,7 @@ static int valid_cc(const void *bc, int len, int cc) return 0; if (cc == len) return 1; - if (op->yes < 4) + if (op->yes < 4 || op->yes & 3) return 0; len -= op->yes; bc += op->yes; @@ -447,11 +447,11 @@ static int valid_cc(const void *bc, int len, int cc) static int inet_diag_bc_audit(const void *bytecode, int bytecode_len) { - const unsigned char *bc = bytecode; + const void *bc = bytecode; int len = bytecode_len; while (len > 0) { - struct inet_diag_bc_op *op = (struct inet_diag_bc_op *)bc; + const struct inet_diag_bc_op *op = bc; //printk("BC: %d %d %d {%d} / %d\n", op->code, op->yes, op->no, op[1].no, len); switch (op->code) { @@ -462,22 +462,20 @@ static int inet_diag_bc_audit(const void *bytecode, int bytecode_len) case INET_DIAG_BC_S_LE: case INET_DIAG_BC_D_GE: case INET_DIAG_BC_D_LE: - if (op->yes < 4 || op->yes > len + 4) - return -EINVAL; case INET_DIAG_BC_JMP: - if (op->no < 4 || op->no > len + 4) + if (op->no < 4 || op->no > len + 4 || op->no & 3) return -EINVAL; if (op->no < len && !valid_cc(bytecode, bytecode_len, len - op->no)) return -EINVAL; break; case INET_DIAG_BC_NOP: - if (op->yes < 4 || op->yes > len + 4) - return -EINVAL; break; default: return -EINVAL; } + if (op->yes < 4 || op->yes > len + 4 || op->yes & 3) + return -EINVAL; bc += op->yes; len -= op->yes; }