32-bit to 64-bit Conversion Woes with strtoul()

Proper error checking when using the strtol() family of functions is notoriously difficult.  See this stackoverflow thread explaining it.

Focusing only on strtoul() here, the correct error checking in most cases is:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17

unsigned long mystrtoul(char *str, bool *success) { unsigned long val = 0; char *eptr = NULL; errno = 0; *success = true; val = strtoul(str, &eptr, 0); if (eptr == str || *eptr != '\0' || (val == ULONG_MAX && errno == ERANGE) || (val == 0 && errno == EINVAL)) *success = false; return val; }

That is:

• IF your end pointer points to the beginning of your string, you've got a problem
• IF your end pointer doesn't point to the end of your string, you've got a problem
• IF your output is at the max end of the range (a potentially valid value) AND you get errno, you've got a problem
• IF your output is 0 (a potentially valid value) AND you get errno, you've got a problem
• ELSE congratulations, you've converted a string to an integer

Understandably, a lot of people get this error checking wrong when using this function. I've reviewed source code I work on and found mistakes.  But even if you got all of this right, you may still have gotten it wrong.

If you originally wrote this code for a 32-bit system, with the intention of converting and storing a 32-bit number, then what you wrote is correct.  If you then recompile this correct code on a 64-bit system, it becomes incorrect.

That's right with no modifications (and no compiler warnings), your carefully reviewed integer conversion function goes from right to (dangerously) wrong.

For example give as input "4294967296" and assign to an unsigned int:

1 2 3 4 5 6 7 8 9 10 11

int main(void) { bool success = false; unsigned int val = 0; val = mystrtoul("4294967296", &success); printf("%s %u\n", success?"success":"failure", val); return 0; }

Result:

• On 32-bit Linux:  failure 4294967296 (ERANGE)
• On 64-bit Linux:  success 0

The 32-bit compilation catches the integer overflow and errors out.  The 64-bit version does not and rolls back around to 0.  So your output is wrong and the usual possible errors and security holes with integer overflows apply:

• malloc(4294967296) allocate 0 bytes and a buffer overflow is created
• uid_t 4294967296 become 0 and a user is root

And strings passed in to strtoul() are very often user controlled input.  That's why you need to convert them from an on-the-wire, CLI, or on-disk string format to an in-memory integer.

This isn't strictly a strtoul() problem.  This is a LP64 data model problem.  Any assumption about the 32-bit size of a long needs to be checked when converting from 32-bit to 64-bit runtime.  This is just a particularly tricky example of the trade offs made by LP64.

Windows Update Error Code 0x80073712

I have several Windows 7 VMs which I use for various dev testing.  For the past 9 months all of them having been failing during Windows Update.  Most updates apply, but Service Pack 1 consistently errors during installation with the unknown error code 0x80073712.

Microsoft has a well written KB explaining steps you can take when this issue occurs: 

http://support.microsoft.com/kb/957310

I went through each step, running the checksur.exe and sfc.exe tools and downloading the stand alone Service Pack 1 installer.  All failed.  Next, I tried step 4 attempting to upgrade with the original Windows 7 CD.  Note, that the CD image must be the exact same used to install the original OS.  Same release flavor, Enterprise, in my case, and same SP level.  I thought I'd want to upgrade using the ISO image with SP1 built in, but no that refuses to even attempt an upgrade.

Finally, with the correct ISO image I got the clue that no previous error message had given:

• There is not enough free space on partition (C:). A total of 16372 megabytes (MB) of free disk space is required. Try Disk Cleanup, or moving files to an external location such as a CD, DVD, or external hard drive, and then restart the installation.

I didn't have enough free space on the drive to install the Service Pack.  At first I was surprised because I had 10GB free, but in fact Win7 SP1 requires about 16GB to install.  Next, I was annoyed that this simple error wasn't raised by Windows Update in the first place.

I keep my drive space pretty sparse on my test VMs because they don't have many applications or data stored on them.  But a 40GB primary drive size is not enough for Windows 7.  I expanded it to 80GB using VMWare Workstation 8's built-in "Expand Disk" tools then used sysresccd and gparted to expand the NTFS partition across the whole disk.

Success!  Windows Update installs Service Pack 1 with no problems.  I hope this helps you or someone else avoid a half-day of IT induced profanity.

Using shquote() to Combat Command Injection

I've been incredibly impressed with FreeBSD's shquote(3) function as a singular way to prevent command injection in a server process.  I'm currently working on a RESTful API (aren't we all), backed in part by a server daemon written in C.  The largest threat to any networked API is user input.  A server developer must assume that all input is malicious until proven valid.  There are many types of injection attacks against web services; I've been focused on Unix command injection today.

Let's say you're building a service which takes a directory path as input and creates this directory path on the server's file system.  In C we can just call:

 void mkdir_recursive(char* path) {
      char* cmd;
      asprintf(&cmd, "mkdir -p %s", path);
      system(cmd);
 }

Now wait!  Using system() is bad!  It's prone to command injection attacks and there's usually always a programmable API to do the same thing in a safer, cleaner way.

I agree, but I am interested in exploring command injection and mkdir -p is so convenient for recursive directory creation.  There's no easy libc alternative and writing it myself would take 10x as much code.  Yes, I'm complaining about 3 lines vs 30, but Larry Wall says laziness is one of a programmers greatest virtues.

So normally to protect this simple code from command injection we'd validate the input for shell meta-characters.  We must check for ; & ( ) ` | > < $.  These could allow an attacker to append an arbitrary command onto our simple mkdir, read or write files, and access environment variables.  For instance we could receive a path which would make the above command look like this:

mkdir -p a/new/dir ; cat /etc/passwd > /usr/local/apache/htdocs/passwd

Which unfiltered would cause the server's local user list to now be published in a publicly accessible directory.  To further cause issues, in my particular use case, all of the above characters are valid for Unix directory paths.  So simply removing them from the user's input would be incorrect behavior.

In comes shquote().  It protects against all of these command injections and allows these characters to be used in path names, because it puts everything in strong quotes.  With a refactor of our above code snippet:

 void mkdir_recursive(char* path) {
      char *cmd;
      int len = shquote(path, NULL, 0) + 1;
      char *safe_path = malloc(len);
      shquote(path, &safe_path, len);
      asprintf(&cmd, "mkdir -p %s", path);
      system(cmd);
 }

The execute command now looks like this:

mkdir -p 'a/new/dir ; cat /etc/passwd > /usr/local/apache/htdocs/passwd'

Which will safely create 10 new directories, instead of calling 2 commands.

# while [ "`ls`" ]; do ls -F && cd "`ls`"; done
a/
new/
dir ; cat /
etc/
passwd > /
usr/
local/
apache/
htdocs/
passwd/

Surely we can still trick shquote() with embedded \ or ' characters right?  Let's try adding intermediate quotes to break up the two commands:

a/new/dir' ; 'cat /etc/passwd > /usr/local/apache/htdocs/passwd

becomes

mkdir -p 'a/new/dir'\'' ; '\''cat /etc/passwd > /usr/local/apache/htdocs/passwd'

# while [ "`ls`" ]; do ls -F && cd "`ls`"; done
a/
new/
dir' ; 'cat /
etc/
...

That didn't give us an exploit.  So let's try escaping those intermediate quotes:

a/new/dir\' ; \'cat /etc/passwd > /usr/local/apache/htdocs/passwd

becomes

mkdir -p 'a/new/dir\'\'' ; \'\''cat /etc/passwd > /usr/local/apache/htdocs/passwd'

# while [ "`ls`" ]; do ls -F && cd "`ls`"; done
a/
new/
dir\' ; \'cat /
etc/
...

My simple function handles them both correctly with successive strong quote wrappings.  It still isn't protected from other attacks like path traversal trickery using embedded .. but shquote() goes a long way in santizing input.  I couldn't think of a way to break it.  Please tell me if you can.

64-bit Stack Padding

I tracked down an interesting bug recently worth noting due to the lessons I learned about 64-bit computing.  The bug was in some unit test code, essentially testing different types of a custom object. Here's a simplified example of the bug in a standalone program:

 #include <stdio.h>
 #include <stdlib.h>
 
 enum type {
         T0 = 0,
         T1,
         T2,
         T3,
         T4,
         T5,
         TMAX = T5
 };
 
 struct typed_item {
         enum type t;
         void *data;
 };
 
 void test1(void)
 {
         struct typed_item *list[TMAX];
         char *success_string = "Success";
         int i = 0;
 
         for (i = T0; i <= TMAX; i++) {
                 list[i] = malloc(sizeof(struct typed_item));
                 list[i]->t = i;
         } 
          
         /* do tests on list */
          
         printf("%s\n", success_string);
 
         /* cleanup */
 }
 
 int main(void) 
 {   
         test1();
 }

Here's the console output:

sdann32# gcc stack.c && ./a.out

sdann32#

I expected "Success" but instead get a non-printable character and a newline.  Can you spot the bug?

-        struct typed_item *list[TMAX];
+        struct typed_item *list[TMAX+1];

This is a classic off-by-one error causing stack corruption.  The for loop writes one entry past the end of the array and modifies the next variable on the stack.  This was straightforward to track down in gdb.  What was tricky, is that I couldn't repro such a simple bug consistently.  When I ran this test on a different machine, everything worked fine.

My company is in the process of converting from a 32-bit userspace, to a 64-bit userspace.  Because of the way our feature branches are managed in a source tree, often I'm switching between development on a 32-bit build and a 64-bit build.  This bug only reproed on a 32-bit platform as seen above.  The 64-bit system succeeded:

sdann64# gcc stack.c && ./a.out
Success
sdann64#

Since my initial belief was that this issue was caused by stack corruption, I dropped into gdb on the 64-bit machine to examine the test1() stack variables.  To see changes in the stack variables easier, I've initialized the array with a known value of 2.

sdann64# gcc stack.c -g3 -O0
sdann64# gdb ./a.out       
GNU gdb 6.1.1 [FreeBSD]
Copyright 2004 Free Software Foundation, Inc.
This GDB was configured as "amd64-marcel-freebsd"...
(gdb) b test1
Breakpoint 1 at 0x4005d9: file stack.c, line 21.
(gdb) run
Breakpoint 1, test1 () at stack.c:21
21        struct typed_item *list[TMAX] = { (void*)2, (void*)2, (void*)2, (void*)2, (void*)2 };
(gdb) n
22        char *success_string = "Success";
(gdb) n
23        int i = 0;
(gdb) n
25        for (i = T0; i <= TMAX; i++) {
(gdb) p $rbp
$1 = (void *) 0x7fffffffe870
(gdb) p $rsp
$2 = (void *) 0x7fffffffe820
(gdb) x /10xg $rsp
0x7fffffffe820:    0x0000000800902040    0x0000000000000002
0x7fffffffe830:    0x0000000000000002    0x0000000000000002
0x7fffffffe840:    0x0000000000000002    0x0000000000000000
0x7fffffffe850:    0x000000000040065d    0x00000000006fb08c
0x7fffffffe860:    0x0000000000000001    0x0000000000000001

Once past the local variable initialization instructions I check the frame base pointer ($rbp) and the frame stack pointer ($rsp) and see that there's a 0x50 difference between them, or 80 bytes.  Listing this portion of the stack gives the local variables for the function.

(gdb) x /10xg $rsp
0x7fffffffe820:    0x0000000000000002    0x0000000000000002
0x7fffffffe830:    0x0000000000000002    0x0000000000000002
0x7fffffffe840:    0x0000000000000002    0x0000000000000000
0x7fffffffe850:    0x000000000040065d    0x00000000006fb08c
0x7fffffffe860:    0x0000000000000001    0x0000000000000001

Annotated with color above, the red section are saved registers and temporary variables.  The three local variables are highlighted as such:

struct typed_item *list[TMAX];
char *success_string = "Success";
int i = 0;

Just as I suspected, when asking the compiler for 80 bytes of stack storage space, the allocation is padded to 96 bytes.  It is this extra 16 bytes which allows my accidental off-by-one overwrite to have no functional affect on the program.  I've never run into this issue on 32-bit platforms, so I'm now mindful that 64-bit stack padding can mask bugs that appear on other architectures.

(All testing done with Intel x64 processor, FreeBSD 7.3, and gcc 4.2.)

SDC 2011 Presentations Online

The SNIA Storage Developer Conference is held every September in sunny Santa Clara, CA and is attended by almost every file protocol developer in the world.  All the geeks that do my job (SMB file server development) at all the major OS manufacturers (Microsoft, Apple, NetApp) get together to talk about odd bugs, watch presentations, and actually test interoperability in SMB lab.

The benefit of this event is 3-fold:

1. Networking with other professionals
2. Testing client/server interaction with the developers who wrote the stack, and can fix the bugs immediately.
3. Thoughtful, informative, presentations of past and future technology.

While you have to attend to get the first two, access to the presentations has just been made available online by SNIA.  In addition to the slide decks, several key presentations were recorded as well.  All PDF and streaming videos are available at:

http://www.snia.org/events/storage-developer2011/2011presentations

On the SMB front, I personally recommend the talk SMB 2.2: Bigger. Faster. Scalier. by David Kruse and Matthew George of Microsoft.  Going into the details is a whole separate post, but I will say Microsoft has decided to be engineers again and implement some truly innovative features in their file protocol.