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 five 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 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's been initialized to encapsulate 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.

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:

3 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  

Post a Comment

Links to this post:

Create a Link

<< Home