... An example of flawed code

Do you recall our small exercise in the last issue of the Bulletin?

 

We were wondering how well written the following code was:

 

 

1 /* Safely Exec program: drop privileges to user uid and group
2 * gid, and use chroot to restrict file system access to jail
3 * directory. Also, don’t allow program to run as a
4 * privileged user or group */
5 void ExecUid(int uid, int gid, char *jailDir, char *prog, char *const
argv[])
6 {
7 if (uid == 0 || gid == 0) {
8 FailExit(“ExecUid: root uid or gid not allowed”);
9 }
10
11 chroot(jailDir); /* restrict access to this dir */
12
13 setuid(uid); /* drop privs */
14 setgid(gid);
15
16 fprintf(LOGFILE, “Execvp of %s as uid=%d gid=%d\n”, prog, uid, gid);
17 fflush(LOGFILE);
18
19 execvp(prog, argv);
20}
(Courtesy of Barton Miller, University of Wisconsin, Madison, US)

Indeed, it was not so well written since it contained at least 13 flaws:

1. Line 1: Incomplete specification: Does it run *arbitrary* commands or just a few selected ones? Who checks for errors? The function or the caller? Does it run on *arbitrary* chroot jails? What about thread-safety? Is this expected to run in a multithreaded environment?

2. Line 5: Depending on the platform, there may be integer-related issues.

3. Line 5: No sanitization of "jailDir". For example "/" will do nothing.

4. Line 11: No check for errors on "chroot". chroot("lkjhkjlhkljh") or chroot(NULL) would bypass the jail.

5. Line 11: Missing "chdir(jailDir)" before the chroot, or chroot("/") after it.

6. Line 11: No checks for errors.

7. Lines 13/14: setuid & setgid run in the wrong order.

8. Lines 13/14: No checks for errors, so the attacker may choose some random number for uid and gid and run the program as root.

9.  Line 16: Is LOGFILE actually open? This may crash the program, or may make it exploitable.

10. Line 19: No sanitization of prog, it may cause NULL pointer dereferences, crashes, etc. and make the code exploitable.

11. Line 19: No environment sanitization.

12. Line 19: No error handling: if execvp() returns it means there is some error to be handled. The specification is weak in this case.

13. If the program runs in a multithreaded environment, sanitization will have to make private copies of jailDir, prog and argv[] and perform the checks on them.


The winners of the three marvellous books on software security are Bertrand Lefort (BE/OP), Paolo Torelli (extern) and Remi Mommsen (CMS). Congratulations!!!

You think this is not easy? True --- and this is the advantage for any attacker. He just has to find a few flaws to exploit that code and take over the corresponding server. Thus, please check basic rules for good programming as well as essential books on proper software development in the section for software developers on our security web page. Also, you can easily test your software yourself. Thoroughly check the warnings from your compiler and run one of our suggested static code analysers. In addition, HR technical training provides excellent courses on secure programming in Java, C++, Python, Perl and web languages. The next one-day, hands-on courses are on securing  PHP, Java and Web applications (September 27th, 28th, and 29th, respectively) as well as for secure programming in Python (October 28th). There are still places available!!!

Finally, do not hesitate to contact Computer.Security@cern.ch if your prefer an external review of your software!

Of course, if you have questions, suggestions or comments, please contact Computer.Security@cern.ch or visit us at http://cern.ch/security.

by Computer Security Team