From owner-freebsd-security@FreeBSD.ORG Wed Apr 23 19:14:46 2014 Return-Path: Delivered-To: freebsd-security@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id A4A31925 for ; Wed, 23 Apr 2014 19:14:46 +0000 (UTC) Received: from mail-out.apple.com (mail-out.apple.com [17.151.62.50]) (using TLSv1 with cipher DES-CBC3-SHA (168/168 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 846311B93 for ; Wed, 23 Apr 2014 19:14:46 +0000 (UTC) MIME-version: 1.0 Content-transfer-encoding: 7BIT Content-type: text/plain; CHARSET=US-ASCII Received: from mail-out.apple.com by local.mail-out.apple.com (Oracle Communications Messaging Server 7.0.5.30.0 64bit (built Oct 22 2013)) id <0N4H00200ZYQZE00@local.mail-out.apple.com> for freebsd-security@freebsd.org; Wed, 23 Apr 2014 12:14:40 -0700 (PDT) Received: from relay6.apple.com ([17.128.113.90]) by local.mail-out.apple.com (Oracle Communications Messaging Server 7.0.5.30.0 64bit (built Oct 22 2013)) with ESMTP id <0N4I009U303KACD0@local.mail-out.apple.com>; Wed, 23 Apr 2014 12:14:40 -0700 (PDT) X-AuditID: 1180715a-f79cb6d00000168c-32-5358112004e6 Received: from [17.149.224.243] (Unknown_Domain [17.149.224.243]) (using TLS with cipher AES128-SHA (128/128 bits)) (Client did not present a certificate) by relay6.apple.com (Apple SCV relay) with SMTP id BD.A8.05772.02118535; Wed, 23 Apr 2014 12:14:40 -0700 (PDT) Subject: Re: OpenSSL static analysis, was: De Raadt + FBSD + OpenSSH + hole? From: Charles Swiger In-reply-to: <50CA7E78-BB5E-4872-A272-B7374627EC12@cederstrand.dk> Date: Wed, 23 Apr 2014 12:14:40 -0700 Message-id: References: <10999.1398215531@server1.tristatelogic.com> <50CA7E78-BB5E-4872-A272-B7374627EC12@cederstrand.dk> To: Erik Cederstrand X-Mailer: Apple Mail (2.1510) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprLLMWRmVeSWpSXmKPExsUiOPXBZ10FwYhgg61TRCyevrW36Nn0hM2B yaN5+WJ2jxmf5rMEMEVx2aSk5mSWpRbp2yVwZRw+85ul4JVExYdJl1kaGLuEuxg5OSQETCSO 3JrDAmGLSVy4t54NxBYS6GeS2HBFAMRmFtCSuPHvJVMXIwcHr4CexPZfciCmsICPxIMjxiAm m4CaxISJPCDFnAJOEv1LfoENYRFQldi9eg8TxBAviUePJzJC2NoSyxa+ZgaxeQWsJJYd7GSH WJonsf/TEbAaEQEDiRMf3zNDHCYrcfrcc5YJjPyzkNwzC+GeWUimLmBkXsUoUJSak1hpppdY UJCTqpecn7uJERRoDYVROxgbllsdYhTgYFTi4ZW4HB4sxJpYVlyZe4hRgoNZSYTXliEiWIg3 JbGyKrUoP76oNCe1+BCjNAeLkjgvAxdQtUB6YklqdmpqQWoRTJaJg1OqgbH8qOnZ7f/W/S0/ YX2N8d+0W5dq29l3OQWf7ktvVJT8FHBzn1dzDkOojMcltivHjmw+YhwrKbUwVOLzQjMb7hVS YmXsBucObn+28YZK38LN/oceMISd9y5blftwrerPsI0OC+o041a23Eg7c6HqYT+jcbP05du8 uZx86ze1WNf8q9DaMbeqsVeJpTgj0VCLuag4EQBdlg2OMAIAAA== Cc: "freebsd-security@freebsd.org security" X-BeenThere: freebsd-security@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: "Security issues \[members-only posting\]" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Apr 2014 19:14:46 -0000 Hi-- On Apr 23, 2014, at 3:06 AM, Erik Cederstrand wrote: > Den 23/04/2014 kl. 03.12 skrev Ronald F. Guilmette : [ ... ] >> I do imagine that the truth or falsehood of your assertion may depend >> quite substantally on what one does or does not consider a "false >> positive" in this context. > > Have a look at the ~10.000 reports at http://scan.freebsd.your.org/freebsd-head/ (unavailable ATM). Silly things are reported like missing return at the end of main() or not free()ing memory two lines before program exit. There are nonsensical reports because the analyzer doesn't detect exit() in a usage() function because usage() is defined in a separate compilation unit, or this: Sure, static analysis isn't perfect and runs into false positives, some of which are truly harmless and some of which actually do indicate an area where refactoring the code in light of the warning would be an improvement. It's worth noting that even if you believe that (e.g.) the clang static analyzer isn't properly doing liveness analysis and misjudging whether there's a dead assignment (writing to a variable which is never read), the clang compiler will be using the same analysis when doing dead-code elimination and common-subexpression elimination and such while optimizing. > int foo(int y, int z) { > int x; > if (y == z) { > x = 0; > } else { > if (y != z) { > x = 1; > } > } > return x; > } > > warning that x may be uninitialized. Fixing these require considerable effort e.g. improving IPA and adding alpha-remaning support to the analyzer's constraint manager, or would result in unnecessary code churn in FreeBSD just to work around the reports. Ah, that's a classic example. If you declared y and z as const, then I'd agree that the compiler should be free to make assumptions that one of the two if statements must be true. On the other hand, if you assume that the arguments are volatile and that maybe another thread might update y or z on the stack between the time when the first if test is evaluated and the second if, one realizes that the static analyzer might actually have a point. (Or you're on an SMP system and don't get sequential consistency / total-store ordering without memory barriers....) Sure, your code might never intentionally try to mess with the stack, but there's a long history of bugs like typing ~1030 characters at a password prompt and blowing past a char passwd[1024] buffer that someone assumed would be more than enough. The most straightforward changes to this snippet would be either: int foo(int y, int z) { int x; if (y == z) { x = 0; } else { x = 1; } return x; } ...or: int foo(int y, int z) { int x = 0; if (y != z) { x = 1; } return x; } Not only are both of these shorter and they pass clang's static analyzer without a warning, I'd argue that the second version is noticeably cleaner. Regards, -- -Chuck