Errata

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.

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.

in this code the aim is to mover data from src to dst securely
However once data is copied into dst it is still in src array.
We should have the following code instead
if (csrc > cdst && csrc < cdst + len)
for (i = 0; i < len; i++) {cdst[i] = csrc[i]; csrc[i] = c; // overwrite with c
else
while (len--) {cdst[len] = csrc[len]; csrc[len] = c; //overwrite with c

actually, the code printed in the book (at least, as viewed on google books) is correct. what's far worse is that the downloadable tarball ( spc-1.1.tar.gz) contains a typo.

in the tarball, the crucial call to setreuid() is, instead, a call to setregid(). happily, this causes an abort() later on when the uid hasn't actually been successfully changed. but there's a very real risk of a user assuming the check is wrong, and deleting it, rather than tracking down the issue. (it took me about an hour to notice the typo.)

In the spc_fork() function, the function spc_sanitize_files() shall not be called from this function. Indeed spc_sanitize_files() closes all file descriptors (fd) but when spc_fork() is used by the function spc_popen (given in page 32) the fd of the two pipes (stdin_pipe and stdout_pipe) are also closed which is not good because they are used by the child and parent process.
As proove: halt the process before and after spc_sanitize_files() in Linux see /proc/1234/fd where 1234 = getpid(). You'll see 4 pipes fd before and no more fd after.

This function for calling ls command is ok (working for me) but for calling an external software to interact with it (read its output and send to it data on its stdin) is not working.

This can be fixed quite easily :
1/ pass pipes fd to the spc_sanitize_files(int stdin_pipe[2], int stdout_pipe[2])
2/ Instead of for (fd = 3; fd < fds; fd++) close(fd);
Do not close fd for pipes: for (fd = 3; fd < fds; fd++) { if (fd != stdin_pipe[0] && fd != stdin_pipe[1] && fd != stdout_pipe[0] && fd != stdout_pipe[1]) { close(fd); }}
3/ do not call spc_fork() inside spc_popen() but call spc_sanitize_files(stdin_pipe, stdout_pipe), spc_drop_privileges() and reseed() before calling execve().

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;

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.

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.

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.

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;
}

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;
}

In page 117, where it says...

/* When statically allocated */
unsigned char *key[KEYLEN_BYTES];

...why do you need to create a pointer here? This makes sense if we are creating a list of KEYLEN_BYTES length strings, but that's not the case. Thus, this should be changed to...

/* When statically allocated */
unsigned char key[KEYLEN_BYTES];

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.

This was reported earlier by Anonymous but I wanted to expand on it. The downloaded code sample looks to be an earlier version of the code than what is printed in the book. The problem in the downloaded code is physical line# 14: toalloc = (len / 3) * 4 + (3 - mod) % 3 + 1;
It can be fixed as: toalloc = (len / 3) * 4 + (mod? 5: 1);

The version in the book tried to fix it on line 21: p = output = (unsigned char *)malloc(((len / 3) + (mod ? 1 : 0)) * 4 + 1);

That's essentially the same fix as above, but if you use that then the "wrap" code doesn't work. So neither example is correct.

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."

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
", spc_group_ismember("system", "dwmalone"));
printf("wheel %d
", spc_group_ismember("wheel", "dwmalone"));
return 0;
}
% ./mem
system 0
wheel 1

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

"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.