[Mono-dev] patch for StrongName to fix minor security bug

[Sebastien Pouliot]

Hello Andrew,

On Sat, 2005-11-12 at 09:57 -0500, Andrew Yates wrote:
> Hi,
> It is my understanding that the key generated by "sn -k foo.snk" is a
> private key used to sign assemblies, and should not be shared with
> anyone other than the key's owner.

It depends on why you are signing your assembly (e.g. the Mono runtime
doesn't verify the signature so the public key is used to produce a
"better", i.e. stronger, name for the assembly). 

But you're right that sn, or any other tool producing private keys (like
makecert), cannot predict the user's usage of the generated keys.

> If this is the case, there is a minor security bug in sn: when it
> creates the key, it creates it according to the user's umask. The
> umask for most distributions is 0022 by default, so this would create
> the file with rw-r-r (644) permissions, allowing anyone with access to
> that computer to read the key, assuming the key's parent directories
> have rwx-rx-rx (755) permissions.

Good point. I don't see any good reason to allow more than the user to
read the file.

Microsoft implementation of SN doesn't do this (e.g. creating a .snk in
c:\temp will make the key available to all Users to be read/execute) -
but it's not a behaviour I care to follow ;-).

> The key should be created with rw - - permissions (600). Below is a
> patch which chmods the key to rw - - immediately after creating it, in
> order to prevent unauthorized users from taking it and compromising
> security.

See comments in the source code

> Andrew
> 
> 
> --- sn.cs.old   2005-11-10 08:31:54.000000000 -0500
> +++ sn.cs       2005-11-12 09:32:45.000000000 -0500
> @@ -352,7 +352,14 @@
>                                                 }
>                                         }
>                                         sn = new StrongName (size);
> -                                       WriteToFile (args[i],
> CryptoConvert.ToCapiKeyBlob (sn.RSA, true));
> +                                               try {
> +                                                       WriteToFile
> (args[i], CryptoConvert.ToCapiKeyBlob (sn.RSA, true));
> +                                                      
> Mono.Unix.Native.Syscall.chmod (args[i],
> (Mono.Unix.Native.FilePermissions)0x180);

I doubt this would work, as is, under Windows. But we could add a
platform check.

It also introduce a new dependency for 'sn' which is required very early
in the bootstrap process. This also affects packaging the core, minimal,
mono. Not really a big deal either as we could include the p/invoke
declaration inside sn itself.

A simpler solution would be to change the sn (and makecert) script(s) to
execute "umask 077" before calling "mono sn ...". But I'll check if this
could have some undesirable side effects before committing this change
(at least it doesn't have any effect for re-signing an assembly file as
it already exists and keep it's permissions intact).

> +                                               }
> +                                               catch {
> +                                                      
> Console.WriteLine("An error occurred while creating the file '{0}'.",
> args[i]);
> +                                                       break;
> +                                               }
>                                         if (!quiet)
>                                                 Console.WriteLine ("A
> new {0} bits strong name keypair has been generated in file '{1}'.",
> size, args [i]);
>                                         break;

Thanks for spotting (and reporting) this.
-- 
Sebastien Pouliot
email: sebastien at ximian.com
blog: http://pages.infinit.net/ctech/