Thursday, January 29, 2009

A single line assignment filled with epic fail

Today I've been working with a piece of third-party software. I was having big problems getting it working and decided to delve into its source (which was included).

I'm not going to name the application (and I've obscured the details below), but the bug is so epic that it manages to combine on a single line four separate types of bugs: a design bug, two implementation bugs, a security hole and an out of date comment.

The program needs to copy a file into a directory. In the process of doing so it sets write permission on the directory into which the file will be copied. The first bug is the design: the program shouldn't be updating permissions on a directory without the user having requested the change. If the directory isn't writable an exception should be generated and an appropriate error displayed.

This particular JavaScript program avoided all that by just setting write permission. It did it like this:

dir.permissions = 666; // Make the file writable


dir
is an object that points to the directory to be written to and the permissions attribute allows direct 'chmod' style updating of UNIX-style permissions. Setting the permissions to octal 666 sets read/write for the owner of the directory, the group the owner belongs to and everybody else. It also removes execute permission. In the UNIX world execute permission on a directory means that you can CD into it; with it removed the directory cannot be accessed (i.e. you can't get a list of the files in the directory). That's bug number two.

The third bug is the security hole: by setting read and write for owner, group and everyone absolutely anyone can read and write to the directory that the user was trying to copy a file into. Imagine doing that on your My Documents or similar.

The fourth bug is that the implementer has accidentally used decimal 666 and not octal 666 (which should have been 0666) which means that they are doing the equivalent of octal 1232 which gives the owner of the directory just write permission, the group write and execute and everyone has permission to write. So the permissions are totally wrong.

The extra 1 at the beginning sets the sticky bit whose effect will be platform dependent.

And, finally, bug number five: the comment is incorrect. The permission change is happening on the directory and not the file. Worse, it's the permissions on the file that the coder was originally trying to change by introducing this one line change.

Labels:

If you enjoyed this blog post, you might enjoy my travel book for people interested in science and technology: The Geek Atlas. Signed copies of The Geek Atlas are available.

15 Comments:

Blogger JMS said...

I argue the comment is correct. Comments should state the codes goal.

7:32 PM  
Blogger Jessica S. said...

I like the bugs that cause easily avoidable security problems. That way I can give coders a hard time for not knowing what they're doing, rather than the other way around...

Normally I'm at the receiving end of an ignorant programmer, being a user of software and all.

9:13 AM  
Blogger Pádraig Brady said...

A devil of a line that.

2:25 PM  
Blogger SaltwaterC said...

Actually there is one bug: not using the octal notation.

The first "bug" is just a design choice. It's arguable if a situation where the error is recoverable makes sense to ask the user for permission. Sometimes it makes sense, sometimes it doesn't. It really depends on the application itself. One line of code is too little to tell.

The second "bug" is bogus. Not being able to list the directory contents is not the same as not being able to read / write files which was the actual intention. If it happens to know the file path, this is a non-issue. Really. Unless you want to find the path by walking the directory which is a totally different beast.

The third "bug" is contextual. Given certain applications, it may not be a bug at all. Even then, the defaults are 0755 and 0644 for new directories / files. This doesn't mean that somebody else may read the files of your home directory, unless your /home/foo is not set properly in the first place. If the dir happens to be inside another dir with 0700 permissions, then the hierarchy would deny the group and others to actually reach that level.

The fifth "bug", come on, since when the comments are "bugs"? Have you taken some time to actually read the definition of a software bug before posting this article? And the intention was actually to write something into that dir, namely files. It isn't that bad, actually.

1:46 PM  
OpenID id said...

@SaltwaterC: So, are you claiming John is unable to distinguish the context in the actual application?

By the way, you're wrong about bug 2. Testing:

$ mkdir john
$ echo test > john/file
$ cat john/file
test
$ chmod -x john
$ cat john/file
cat: john/file: Permission denied

In case you're wondering if cat lists the directory, let's see with strace: after some initialization calls, we get:

open("john/file", O_RDONLY|O_LARGEFILE) = -1 EACCES (Permission denied)

So no, even if you know the path, you can't read the file.

3:04 PM  
Blogger Marco Lopes said...

SaltwaterC wins the trophy for trolling!!!

3:17 PM  
Blogger Marco Lopes said...

SaltwaterC wins the trophy for trolling.
Here's why:

Bug 1-The first "bug" is just a design choice.

No it's not. Unless you're hacking a script to help you with some quick task, changing permissions on an arbitrary folder is wrong. The writer of the post already established we're talking about an application, so bug nr. 1.

Bug 2-Bug number 2 is a bug. An hardcoded permission that will prevent users from entering the folder or listing it's content is wrong. Either the developer didn't knew much about permissions (which seems to be the case given all the failures on that line of code) or, again, made a poor design choice.

Bug 3 - Actually the one you end up calling a bug, is not a bug per se, is just a security issue. It's still bad, and ends up being bad in a different way because of bug 4 causing the permissions to be set to something different that what first seem to be the case.

Bug 5 - In fact it isn't a bug, but it's quite bad, having comments describing a behaviour that is not what the code does, is misleading, it's noise and shouldn't be there.

3:27 PM  
Blogger Pete Hawkins said...

the main bug here is using JS server-side...

6:30 PM  
Blogger RBerenguel said...

Amusing little line. Also a few amusing comments in the post... Congrats John ;) I hope you are not using this piece of software any more, beware of the lions waiting far below...

Ruben

6:56 PM  
Blogger Jordan said...

I agree with everything except the comment being labeled as a bug. While the comment did not reflect what was actually happening (ie changing permissions in directory), it did reflect what he wanted to accomplish, thus informing anyone reading it of his intended goals. It could have been more detailed but it describes what he was trying to do at least.

9:06 PM  
Blogger KK said...

What about umask? Does that not take care of bugs 1 and 2?

10:14 PM  
Blogger Chani said...

@SaltwaterC: please, tell me what programs you've contributed to, so I can NEVER USE THEM.

10:40 PM  
Blogger korina said...

In Unix, directories ARE files. His comment is technically accurate.

8:24 AM  
Blogger SaltwaterC said...

@id: yes, that was a confusion from my end. If the directory has the executable bit, then one can access / write the file by knowing the path, but listing is forbidden.

@Marco Lopez:

1. by your logic, every PO out there should remove the possibility to change permissions since that is a job for "scripts" and / or UNIX standard tools.

2. yes, it was a legit "bug".

3. the "security issues" are legit bugs. The worst kind. And even then, the hierarchy may avoid it.

5. wasn't the whole purpose of the comment for being ignored by the compiler / interpreter / etc? Since it can't affect the execution flow, it doesn't actually fit the description.

@Chani: please don't use them. In fact, nobody gives a shit. I may start to contribute to some that you use, just to piss you off.

2:50 PM  
Blogger SaltwaterC said...

@id: yes, that was a confusion from my end. If the directory has the executable bit, then one can access / write the file by knowing the path, but listing is forbidden.

@Marco Lopez:

1. by your logic, every PO out there should remove the possibility to change permissions since that is a job for "scripts" and / or UNIX standard tools.

2. yes, it was a legit "bug".

3. the "security issues" are legit bugs. The worst kind. And even then, the hierarchy may avoid it.

5. wasn't the whole purpose of the comment for being ignored by the compiler / interpreter / etc? Since it can't affect the execution flow, it doesn't actually fit the description.

@Chani: please don't use them. In fact, nobody gives a shit. I may start to contribute to some that you use, just to piss you off.

2:50 PM  

Post a Comment

Links to this post:

Create a Link

<< Home