LWN: Comments on "Coverity catches X Window Security Hole"

They scanned all of X and only found one roothole?

bronson — Sun, 07 May 2006 18:19:14 +0000

"This is the axe my great grandfather used! Only replaced the head three times and the handle four."

With the modularization of the X tree it beomces easier to rip out, rewrite, and throw away individual pieces. I expect great things will come from x.org in the next few years.

(the quote might be from a Joel on Software article, memory is hazy)

Not just recycled, but deceptive

daniels — Thu, 04 May 2006 11:32:42 +0000

That's just a phenomenally bad accident of timing. I sent off some wording changes to Coverity a bit over a week ago, but I'd misconfigured my mail server, so all my mail was a week late. By the time I got the mail out, it was the day before we unembargoed the Render fix, so the press release went out that day.

It's in no way Coverity's fault (obviously, they didn't know about this vulnerability until everyone else did).

Coverity catches X Window Security Hole

dlang — Wed, 03 May 2006 22:45:49 +0000

x.org wasn't their customer true, however they were being paied by their customer (department of homeland security) specificly to test the X codebase. so their notification about the bugs was part of what they were paied to do

Coverity catches X Window Security Hole

TwoTimeGrime — Wed, 03 May 2006 16:37:39 +0000

> ... and, as usual, didn't bother to actually tell anyone else.

They might not have known that it could be a security vulnerability. They could have been fixing a typo in the code. As another poster pointed out, X.Org fixed it about two months ago as well. Coverity found it again independently but also recognized the significance of the problem.

Coverity catches X Window Security Hole

cortana — Wed, 03 May 2006 16:05:47 +0000

That's a separate issue. ;)

Coverity catches X Window Security Hole

kleptog — Wed, 03 May 2006 14:10:29 +0000

Coverity is a nice tool, it indeed picks up all sorts of logic errors. Unfortunatly, it's not the brightest. Nearly two-thirds of the "issues" it found in PostgreSQL are false positives. But it's this low-hanging stuff thats nice to have an automated tool to check for because no human will see it.

The other bonus is that if your code is confusing enough to confuse coverity, it probably needs a rewrite anyway :)

They scanned all of X and only found one roothole?

Wol — Wed, 03 May 2006 12:14:11 +0000

Oh well ... at least the maintainer has now changed ...

Don't forget, the XFree guys were almost "not maintaining" X in the apparent hope it would die. XOrg are now actively maintaining and improving it, so I suspect a lot of your "not so good" code will soon disappear as a result of renewed code churn.

I certainly get the impression that the XFree tree is full of cruft, and the reason for the fork is that the guy running the fork was being actively blocked from cleaning things up.

Cheers,
Wol

They scanned all of X and only found one roothole?

arjan — Wed, 03 May 2006 09:14:51 +0000

No offence to the X guys, but there is a lot of really not-so-good code in there.. if the coverity guys only found one roothole.. that's surprisingly low ;)

Coverity catches X Window Security Hole

trey — Wed, 03 May 2006 07:39:57 +0000

Actually, OpenBSD announced the fix today:

http://marc.theaimsgroup.com/?l=openbsd-security-announce...

Coverity catches X Window Security Hole

trey — Wed, 03 May 2006 07:37:52 +0000

"The flaw in question is CVE-2006-0745."

I don't think so. IMHO CVE-2006-1526.

Coverity catches X Window Security Hole

grouch — Wed, 03 May 2006 07:32:16 +0000

So you're admitting that you're unable to differentiate between a function pointer and a function call?

So you're not going to answer the question? It's simple, really. You asserted:

You don't really need Coverity to find bugs like this.
[...]
Simple enough for human beings to detect and fix.

Your assertion contradicts facts. The X.Org release manager quoted in the article seems to appreciate Coverity's report. In fact, he is quoted as saying, "Coverity exposed vulnerabilities in our code that likely wouldn't have been spotted with human eyes."

The bug was spotted by Coverity. X.Org was not their customer, yet they did the right thing and reported it to X.Org. An X.Org representative acknowledges the bug existed, is of a type that they "find once every three to six years", and that it was unlikely to be spotted by human eyes poring over the code. Unless you can show where you or anyone else reported it before Coverity, I think I will take the X.Org representative's words at face value over your assertion.

Not just recycled, but deceptive

dberkholz — Wed, 03 May 2006 06:55:04 +0000

Because an entirely separate vulnerability was announced this morning.

http://lists.freedesktop.org/archives/xorg/2006-May/01513...

Coverity catches X Window Security Hole

airlied — Wed, 03 May 2006 06:45:00 +0000

X fixed it months ago as well, OpenBSD only found about it from X.org...

Coverity catches X Window Security Hole

nix — Wed, 03 May 2006 06:38:11 +0000

... and, as usual, didn't bother to actually tell anyone else.

Recycled news... and recycled discussion.

eru — Wed, 03 May 2006 06:27:55 +0000

This seems to be the same bug that was discussed over a month ago in this LWN article: http://lwn.net/Articles/176233/. It sparked a long discussion on static checking and the C language. Pointless to repeat it here, read the old comments...

Deja Vu

Ross — Wed, 03 May 2006 06:24:34 +0000

Didn't we already have this discussion? This is the same hole that was announced 3-4 weeks back. My opinion is still that GCC should be warning about this usage in -Wall (really anywhere a function is being compared with any constant, unless it's weak and the constant is NULL), and that the X sources should be cleaned up enough that the warning wouldn't be lost in the noise.

Coverity catches X Window Security Hole

job — Wed, 03 May 2006 06:17:05 +0000

I believe the point of static checkers such as Coverity's tool is to catch "low-hanging fruit", i.e. obvious mistakes buried in code and not the really obscure ones.

Coverity catches X Window Security Hole

TwoTimeGrime — Wed, 03 May 2006 05:59:32 +0000

> They found it, nobody else did.

Actually, the OpenBSD people found and fixed it almost two months ago.

See http://www.openbsd.org/cgi-bin/cvsweb/XF4/xc/programs/Xse...

Rapid development -> new bugs all the time

JoeBuck — Wed, 03 May 2006 04:57:14 +0000

Linux (the kernel) is undergoing rapid change, and has large numbers of developers. This is a recipe for introducing new bugs at a rapid rate, both because you can expect a certain number of bugs to be introduced proportional to the number of new lines of code, and because it's a large, complex piece of software with many constraints that developers must keep in mind.

To get code near-bug-free, you have to stop changing anything about it other than the bugs, and even then you'll introduce some fraction of new bugs per bug fix.

Coverity catches X Window Security Hole

dang — Wed, 03 May 2006 04:18:58 +0000

The game isn't "find the most obscure bugs" but is rather "find the bugs." If you've got a problem with Coverity, then sift through all of the fixes in linux kernel that come out of Viro's bird tree and see how many of the bugs caught through sparse annotations are obvious to the practiced eye. ( Hint: Viro's comments may help you detect these. ) And think about it. Linux code has more active eyes on it than many open source projects and sparse finds stuff. Is it luck? Would people find this stuff eventually? For some of these, empirically the answer seems to be "no" given how long a few of the bugs have been in the code. And even if someone were to eventually find everything, do you really want to wait? Or do you really think that the tool isn't worthwhile for finding things a hell of a lot sooner? Or spin it this way: Why did Linus invest his time in rolling sparse in the first place? Why does Viro spend time on sparse annotations? Why does mingo invest time writing automated lock checking code? The answer is that for any complex code base, you really need automated code checkers and good regression test suites if you want to find and fix bugs ( and it doesn't matter if the bugs are simple or mind-bendingly complex).

Anyone who brings a tool to the table that can improve bug detection rates has a tool worth praise. And if they bother to run it against tons of open source code and feed back to the developers, then they deserve praise for that as well.

Or put it this way: Coverity just provided X users a very valuable service free of charge. Whether they then market the effort has no bearing on the correct response from X users, which should be "thank you."

Coverity catches X Window Security Hole

kirkengaard — Wed, 03 May 2006 03:57:03 +0000

Hooah. Alan Cox says Open Source software is always late -- here we have a need, and here we have a description for the tool to fix the problem, and so now somebody will hopefully be moved to scratch that particular itch.

Coverity catches X Window Security Hole

jreiser — Wed, 03 May 2006 03:46:55 +0000

geteuid != 0

gcc is completely silent on the matter.

Then that's an `"opportunity for enhancement" in the compiler :-) As long gcc knows that geteuid is not a weak external [such as if there is an "unprotected" call geteuid() anywhere else in the same compilation unit], then there should be a warning that the comparison is between constants and is always true [the address of a non-weak defined subroutine is never 0.]

Coverity catches X Window Security Hole

drag — Wed, 03 May 2006 03:27:55 +0000

They found it, nobody else did. They deserve credit for it.

It doesn't matter that it's easily detected by humans when no human has or is going to look closely enough to spot something like that. If they didn't detect it then it would still be sitting there.

This is the whole point behind automated testing.

Remember, get enough eyes and then any code is transparent.. even if some of these eyes are computer applications.

Here is what you can do (I'd try but I don't know enough about this sort of stuff):
Write a program that goes thru the code and looks for this sort of error. A script or perl or whatever.. It just goes through the code looking for errors caused by missing parathesis of this nature.

If it has happenned once I bet there is a 90% chance that this error has occured in other parts of X.org's code base. Also it probably exists in other applications.

Coverity catches X Window Security Hole

bk — Wed, 03 May 2006 02:33:24 +0000

I don't necessarily disagree. My point is more that while I'm sure the Coverity tool is useful for finding lots of very difficult and obscure bugs, this doesn't look like one of them. It's as much due to luck that Coverity happened to find this before some human developer as opposed to the inherent superiority of the proprietary Coverity scanner.

In that respect the press release by Coverity is a little bit misleading.

Coverity catches X Window Security Hole

elanthis — Wed, 03 May 2006 02:05:25 +0000

He's admitting that among 100,000s lines of code, the human eye is likely to just flat out not notice that. If you are looking at the line of code in isolation, the error is easy to spot. Surrounded by many other lines of code when you're not specifically looking for that error, it's something you just are not likely to notice. It's entirely possible to stare directly at the line of code and your subconscious "fills in the blanks" and hides the error from you. That's just how human brains work.

Coverity catches X Window Security Hole

bk — Wed, 03 May 2006 01:33:09 +0000

So you're admitting that you're unable to differentiate between a function pointer and a function call?

Coverity catches X Window Security Hole

davej — Wed, 03 May 2006 01:18:30 +0000

neither -Wall or -Wextra will catch this bug.

The only thing I've found that prints any warning at all, is splint, which prints the following...

test.c:5:23: Operands of != have incompatible types ([function (void) returns
                __uid_t], int): geteuid != 0
  Types are incompatible. (Use -type to inhibit warning)

gcc is completely silent on the matter.

Coverity catches X Window Security Hole

grouch — Wed, 03 May 2006 00:54:37 +0000

Soooo, how come Coverity pointed it out instead of you?

Coverity catches X Window Security Hole

bk — Wed, 03 May 2006 00:48:37 +0000

You don't really need Coverity to find bugs like this. It's not like there was a missing parentheses in some horrible deeply nested expression, here was the problem:

This:

if (getuid() == 0 || geteuid != 0)

Should have been this:

if (getuid() == 0 || geteuid() != 0)

Simple enough for human beings to detect and fix.

Coverity catches X Window Security Hole

cortana — Tue, 02 May 2006 22:36:54 +0000

The flaw in question is CVE-2006-0745. This is good lesson as to why you should always work with the -Wall and -Werror compiler options; I guess Xorg generates so many warnings that the additional 'comparison between pointer and int' went un-noticed. :)

Coverity catches X Window Security Hole

jwb — Tue, 02 May 2006 22:18:14 +0000

I love how this story highlights the great benefit of free software. If you or your company are considering using free software in a critical role, you can hire Coverity to analyze the code before you use it. You would not be able to do so with proprietary software unless you were large enough and wealthy enough to get the vendor to disclose the source code to you.

Coverity catches X Window Security Hole

gorpon — Tue, 02 May 2006 21:56:01 +0000

The missing close parenthesis that launched 50,000 emails.