master process handling patch

[Bron Gondwana]

On Thu, Jul 22, 2010 at 05:06:22PM -0500, Patrick Goetz wrote:
> On 07/21/2010 05:12 PM, Bron Gondwana wrote:
> >
> >We decided to pull map_stupidshared.  Are you on the cyrus-devel
> >mailing list?
> >
> 
> Yeah, but I've only been on it for a couple of months -- maybe this
> was discussed previously.

Hmm... I thought it was more recent.  Anyway - it's gone!
 
> Trying to evaluate C code that consists of lots of little functions
> with no documentation is like playing nethack (you just entered a
> maze of twisty little tunnels....).

Well, yeah.  I'm getting quite good at it by now!  I'm trying to
document the reasoning behind the _why_ of stuff I'm doing, but I'm
not going to document all the "what".  If you're going to patch C,
you need to be able to read C!
 
> For example, one of the debian package maintainers introduced this
> patch to ~/master/master.c:
> 
> ----------------------------------------------------
> @ -195,13 +195,17 @@
>      free(a);
>  }
> 
> -void get_prog(char *path, unsigned size, char *const *cmd)
> +void get_prog(char *path, unsigned int size, char *const *cmd)
>  {
>      if (cmd[0][0] == '/') {
>    /* master lacks strlcpy, due to no libcyrus */
>    snprintf(path, size, "%s", cmd[0]);
> +  path[size-1] = '\0';
> +    }
> +    else {
> +  snprintf(path, size, "%s/%s", SERVICE_PATH, cmd[0]);
> +  path[size-1] = '\0';
>      }
> -    else snprintf(path, size, "%s/%s", SERVICE_PATH, cmd[0]);
>  }
> ----------------------------------------------------
>
> The parameter type correction at the top fixes a bug, but what the
> code null terminating path?  If path is used as a string then this
> is OK, but otherwise it could be overwriting a necessary character.
> To properly check if this is an OK patch to submit to the bugzilla,
> I have to track down every use of the get_prog function.

You haven't seen this pattern before?  It's just protecting against
an overrun if cmd[0] is longer than 'size'.  That patch looks fine
to use anywhere - it can't break correct input, and it stops a
buffer overrun on incorrect input.  If the formatted data is longer
than size then there's no "correct" answer anyway.

Now you could agrue that get_prog should return an int, and an error
code if the string can't be made to fit in the buffer - but always
NULLing the last byte just in case is prudent regardless.

Now... if it was me, I would have put the: 

path[size-1] = '\0';

After the if/else rather than copying it inside both cases, but that's
just bikeshedding.  It's a perfectly cromulent patch.
 
> >They're both being removed in Cyrus 2.4.  GUID is now compulsary,
> >so sha1s will be calculated on append.
> >
> 
> Not sure what "GUID is now compulsory" means, but it turns out I was
> wrong and that sha1 has also been compromised
> http://www.schneier.com/blog/archives/2005/02/sha1_broken.html
> 
> so hopefully this is just being used to generate checksums and not
> for actually security....

Security is a myth.  There's just levels of confidence and acceptable
risk.  "collisions in the the full SHA-1 in 2**69 hash operations,
much less than the brute-force attack of 2**80 operations based on
the hash length." - excuse me while I remain underwhelmed.

It's being used to uniquely identify messages for replication purposes.
And for data integrity checking (e.g. checksums).  It's theoretically
possible to switch it out for another algorithm, but there's nothing
else that's significantly better being pushed.

Yes - an attacker who already knew the sha1 of one of your emails could
theoretically upload another email with the same sha1 to their own
folder on the same server, somehow convince it to replicate AT THE SAME
TIME as your email was replicated for the first time (or copied to another
folder) and make your email be replaced by their email.  I don't think
it's a plausible attack scenario.

Bron.