Secure Programming Cookbook for C and C++ 
by John Viega, Matt Messier

The unconfirmed error reports are from readers.  They have not yet been 
approved or disproved by the author or editor and represent solely the
opinion of the reader.

Here's a key to the markup:
        [page-number]: serious technical mistake
        {page-number}: minor technical mistake
        <page-number>: important language/formatting problem
        (page-number): language change or minor formatting problem
        ?page-number?: reader question or request for clarification

This page was updated July 24, 2008.


UNCONFIRMED  errors and comments from readers: 


[4]  5th paragraph;
Page 4 appears to be an enumeration of user environmental variables.
Two consecutive paragraphs on this page start with "Finally,".
In the second instance, the name of the variable appears to be missing.


{39}  Last paragraph, first two lines.;
The first two lines of this paragraph say "..., a user may delete or rename any file
in a directory that the user owns, regardless of whether the user owns the file."
Ownership has no impact on who can delete/rename files, it is determined by the
user's ability to write to the directory. The paragraph should probably say "..., a
user may delete or rename any file in a directory that the user can write to, ...".
The example below demonstrates this on a Linux machine.

10:02:turing 10% ls -ld dir1 dir2
drwxrwxrwx    2 root     root         4096 Feb 22 10:01 dir1
dr-xr-xr-x    2 dwmalone system       4096 Feb 22 10:01 dir2
10:02:turing 11% ls -ld dir1/file dir2/file
-rw-r--r--    1 dwmalone system          0 Feb 22 10:01 dir1/file
-rw-r--r--    1 dwmalone system          0 Feb 22 10:01 dir2/file
10:02:turing 12% \rm -f dir1/file dir2/file
rm: cannot remove `dir2/file': Permission denied
10:03:turing 13% ls -ld dir1/file dir2/file
ls: dir1/file: No such file or directory
-rw-r--r--    1 dwmalone system          0 Feb 22 10:01 dir2/file


{40}  Paragraph 7;
The second paragraph of "The setgid bit" paragraph explains the standard sgid
directory behaviour on SysV derived systems. It doesn't explain that BSD derived
systems act as if the sgid bit is always set


{44}  (pseudo-)Code example at the bottom of the page;
The pseudo code calls open(), then spc_restore_privileges(), and then does a perror
expecting errono to be unchanged from when open is called. However, the setgroups
call in spc_restore_privileges() may clobber errno if it fails. (If any of the other
calls
fail, then the spc_restore_privileges() calls abort(), so that shouldn't be  a
problem.)



{90}  Bad code example in "Unsigned-to-signed coercion" paragraph;
This code is also bad (ISO) C for a resaon not mentioned in the text. C only allows
you to add a number to a pointer, if the resulting pointer will be between the start
and (one past) the end of the chunk of memory that was allocated to produce that
object. See section 6.5.6 of the ISO C99 standard for details.


[102]  spc_email_isvalid function;
If an address without a domain (e.g. "alice") is passed to the function then the
domain validation will go past the end of the string.

The first for loop can terminate when *c is NUL (the end of the address is reached).
When "if (!*(domain = ++c)) return 0;" is executed, domain and c will point to the
byte after the NUL that terminates the address.

One way to fix it is to change
   if (c == address || *(c - 1) == '.') return 0;
to
   if (!*c || c == address || *(c - 1) == '.') return 0;



[111]  The for loop in the code given for spc_utf8_isvalid;
This code doesn't seem to work. Note the use of the variable nb: nb+1 is added to c
at the end of each iteration of the for loop. However, because of the while() which
is the last statement of the for loop, nb always has value -1 when the loop ends,
thus c is never advanced to the next utf-8 character.


[113]bottom Code block;

1) Implementation of spc_fd_set(...) is incorrect. You forgot you need to calculate bit sizes, not bytes.

Should be:

#define BITS 8

void spc_fd_set(int fd, SPC_FD_SET *fdset) {
long *tmp_bits;
size_t new_size;

if (fd < 0) return;
if (fd >= fdset->fds_size * BITS) {
new_size = sizeof(long)*((fd+(sizeof(long)*BITS))/(sizeof(long)*BITS));
if (!(tmp_bits = (long *)realloc(fdset->fds_bits, new_size))) return;
fdset->fds_bits = tmp_bits;
fdset->fds_size = new_size;
}
fdset->fds_bits[fd / (sizeof(long)*BITS)] |= (1 << (fd % (sizeof(long)*BITS)));
}


2) Implementation of spc_fd_clr(...) is identical to spc_fd_set(...).
(Copy/Paste issue)

Probably want something like this:

void spc_fd_clr(int fd, SPC_FD_SET *fdset) {

if (fd < 0) return;
if (fd >= fdset->fds_size*BITS) return;

fdset->fds_bits[fd / (sizeof(long)*BITS)] &= ~(1 << (fd % (sizeof(long)*BITS)));
}

3) Should also be made clear this recipe will not work in Microsoft Windows.


[114]Top;
Continuing previous submission:

1) spc_fd_isset(...) is incorrect due to lack of bit size calculation.

Should be:

int spc_fd_isset(int fd, SPC_FD_SET *fdset) {
if (fd < 0 || fd >= fdset->fds_size*BITS) return 0;
return (fdset->fds_bits[fd / (sizeof(long)*BITS)] & (1 << (fd % (sizeof(long)*BITS))));
}


2) and spc_fd_setsize(...) should probably return number of bits, not bytes.

int spc_fd_setsize(SPC_FD_SET *fdset) {
return fdset->fds_size*BITS;
}


{124} 2nd statement in source code;
The char array b64table is declared to hold 64 characters but it is assigned 65 characters 
(including the terminating "null"). In fact, modern C/C++ compilers will fail to compile 
the code.

Therefore, the second statement in the code should be

static char b64table[65] = "ABCDEF ... 012356789+/";



[124] Near line 20 in example code;
In recipe 4.5, the statement that allocates the buffer to pointers 'p' and 'output' 
(ie. p = output = (unsigned char *)malloc(....) ) does not allocated a big enough buffer in most cases.

It is worth noting that the code in the book and the downloadable code are different when it comes to 
allocating the buffer but both codes did not allocated a big enough buffer.

This bug is insidious because the compiler will not catch the error but it will cause a runtime error. 
You will probably get a runtime error if you try to free the resource returned by spc_base64_encode() 
because in most cases the pointer 'p' has advanced pass the end of the buffer that was allocated. Thus 
when you call free() to free the resource, you are trying to free a part of the memory that does not 
belong to the program. Even if the program does not crash when you free the resource returned by 
spc_base64_encode(), you still have a memory leak problem.

I compiled the code with VC++ 2005 and run it and it caused runtime error every time I try to free the 
resource returned by spc_base64_encode(). I suspect on other platforms this bug may cause erratic and 
hard to debug runtime error.



(136)  Section 4.10.3, in the function "static void pkcs5_F( ... )".;

static void pkcs5_F(unsigned char *p, size_t plen, unsigned char *salt,
                     size_t saltlen, size_t ic, size_t bix, unsigned char *out) {
  size_t        i = 1, j, outlen;
  unsigned char ulast[PRF_OUT_LEN];

  memset(out,0,  PRF_OUT_LEN);
  pkcs5_initial_prf(p, plen, salt, saltlen, bix, ulast, &outlen);
  while (i++ <= ic) {
    for (j = 0;  j < PRF_OUT_LEN;  j++) out[j] ^= ulast[j];
    pkcs5_subsequent_prf(p, plen, ulast, PRF_OUT_LEN, ulast, &outlen);
  }
  for (j = 0;  j < PRF_OUT_LEN;  j++) out[j] ^= ulast[j];
}

pkcs5_initial is called once, and pkcs_subsequent is called ic times, which is a
total of ic + 1 iterations of the PRF applied to the password.  The while loop should
evaluate to:
    while(i++ < ic) {
    ...
    }



{233}  2nd Code block (code for openssl decryption);
In the discussion for using OpenSSL low-level encryption and decryption function, it
first states that the encryption and decryption routines are completely symmetric.
It then goes on to explain the EVP_EncryptFinal_ex() function, and use it in the code
block (bottom of page 232, top of page 233).  However, in the example code for
decryption on page 233, it does not use EVP_DecryptFinal_ex(), so the code examples
are not completely symmetric.


[5.25.3 - p240]  SpcEncrypt code example;
The data length and buffer length arguments to the first call to CryptEncrypt are
swapped around. The buffer length is getting passed in as the data length and the
call always fails.

  if (!CryptEncrypt(hKey, 0, bFinal, 0, pbResult, &dwDataLen, *cbData)) {
      LocalFree(pbResult);
      return 0;
    }
  *cbData = dwDataLen;
  return pbResult;

should just be:

  if (!CryptEncrypt(hKey, 0, bFinal, 0, pbResult, cbData, dwDataLen)) {
      LocalFree(pbResult);
      return 0;
    }
  return pbResult;

The second call to CryptEncrypt in this function is correct.


(322)  Several entries in table 7-2.;
Between the entry for BN_nnmod and BN_mod_exp the descriptions say things like:

r = |a mod b|

Consider when a = -1 and b = 3, then we would consider a mod b to be either -1
or 2, and so the |a mod b| will be 1 or 2. What BN_nnmod actually returns is:

r = a mod b,  0 <= r < b

unless |a mod b| is a notation for non-negative remainder that I've never seen
before.

Also, no Description is given for BN_mod_inverse and the comment for BN_div_word (on
page 323) is actually supposed to be the comment for BN_mod_word.



[324]  1st paragraph;
This paragraph says "no even numbers are primes" - what about 2? The fact that two is
prime has lots of important implications for cryptographs, it is just that 2 isn't a
very good choice as a large prime number!

Maybe the paragraph should say "the only even prime number is 2, and it isn't big
enough to be useful as a large prime."


(330)  code example in Discussion part of 7.7;
The line after the code example says "Be sure to deallocate the BIGNUM objects if
you're erasing the last references to them". This should probably refer to BN_free()
and BN_clear_free() in Recipe 7.4, as the code doesn't make it clear that it is
NULLing out a pointer rather than zeroing out a number.

Note that the example code in Recipe 7.5 and 7.6 seems to leak memory for BIGNUMs
and Contexts, though I haven't tested to be certain.


(374)  second code example, definition of spc_group_getname;
Function calls libc function "getgruid(gid)", this should be "getgrgid(gid)" as the
former function does not exist.


[375]  definition of spc_group_ismember function;
The spc_group_ismember function only checks a user's secondary groups and
does not check a user's primaty group. For example:

% groups
system vet wheel www floppy src lj4 unsup images submit
% tail -5 mem.c
int main(int argc, char **argv) {
        printf("system %d\n", spc_group_ismember("system", "dwmalone"));
        printf("wheel %d\n", spc_group_ismember("wheel", "dwmalone"));
        return 0;
}
% ./mem
system 0
wheel 1


(382)  Table 8-1, third entry;
The table says that "10/24" means that "any address starting with 10 will be
matched", I'm fairly sure this should say "any address starting 10.0.0. will be
matched".


[383-384]  Definition of spc_host_init function;
The bufsz variable can be used before it is initialised.

% gcc -W -Wall -pedantic -ansi -O2 -c 4-init.c
4-init.c: In function `spc_host_init':
4-init.c:79: warning: `bufsz' might be used uninitialized in this function


{389}  definition load_wordlist function;
The variable offsets uses a size_t to store offsets returned by ftell - it should use
longs, or even use off_t and ftello().


{467}  5th line from top;
The line-

bResult = HttpSendRequest(hRequest,
lpszHeaders,dwHeadersLenght,lpOptional,dwOptionalLenght);

should read:

bResult = HttpSendRequest(hRequest,
lpszHeaders,dwHeadersLenght,lpszOptional,dwOptionalLenght);


{480}  Definition of spc_socket_accept;
The check of the return value of accept looks wrong:

if (!(new_sock->sd = accept(..))
  goto error_exit;

Accept usually returns -1 on failure and >=0 on success,
so this code does the wrong thing if accept returns 0 or
-1.


{483}  second paragraph;
The text says that FreeBSD does not return the process ID of the sending process,
however it seems it does:

struct cmsgcred {
        pid_t   cmcred_pid;             /* PID of sending process */
        uid_t   cmcred_uid;             /* real UID of sending process */
        uid_t   cmcred_euid;            /* effective UID of sending process */
        gid_t   cmcred_gid;             /* real GID of sending process */
        short   cmcred_ngroups;         /* number or groups */
        gid_t   cmcred_groups[CMGROUP_MAX];     /* groups */
};

I haven't checked NetBSD.


(518)  3rd paragraph;
"You will be prompted for a password when running the first command" should say "...
second command".


(535)  3rd paragraph;
The second sentence says (roughly) "SSL without cert verification is no better than
exchanging keys in the clear" and the third sentence says that "SSL without cert
verification  gives security against passive eavesdroppers". However, exchanging keys
in the clear is vunerable to passive eavesdroppers. The second sentence should
probably be deleted, as it is the incorrect one I think.


[540]  2nd paragraph, last sentence;
"If you do not, any site could present you with Microsoft's certificate, claiming it
as their own, and it would successfully verify."

This could only happen if the site had stolen the private key matching the public key
embedded in the certificate.  The whole point of certificates is to securely bind an
identity to a public key matching a private key verifiably held by the owner of the
identity.  This sentence is simply wrong.


(571)  second paragraph;
This paragraph says that a (0.25,0.75) is 50% easier to guess than a fully entropic
source (0.5,0.5), however neither the Shannon Entropy or the avergae number of
guesses of these quantities are related by a difference of 50%:

> (0.5*ln(0.5) + 0.5*ln(0.5))/ln(0.5)
        1
> (0.25*ln(0.25) + 0.75*ln(0.75))/ln(0.5)
        ~.81127812445913286390
> 1*0.5 + 2*0.5
        1.5
> 1*0.75 + 2*0.25
        1.25

It looks like you should just drop the "50%" and the last sentence in the paragraph.


(572)  First poing in "tips on collecting entropy";
The first point seems to say that postprocessing your data can help ensure that your
data has enough bits of entropy, however no postprocessing can increase the entropy
associated with some data. It can increase the density of entropy, by squashing the
entropy into a shorter string, as pointed out in the text, but the wording of the
point is slightly misleading.


(585,587,589)  Start of code examples;
C++ style comments are started with "/*" rather than "//", seems to be fixed in
downloadable code examples.


 3rd paragraph (not counting aside);
This paragraph says that on systems with a /proc, most of the interesting OS info is
in /proc. This isn't really true, except on Linux. On most OSes /proc only has
running information processes but not on things like interrupts, network stack stats,
...


{714}  Code example at end of page;
The variables set by the signal handlers should have type "volatile sig_atomic_t",
not type "int", see C99 standard section 7.14.1.