Hi Georg,
On 02/03/15 11:53, Georg Koppen wrote:
> Hi,
>
> I am putting a patch inline (not sure if attachments are allowed on this
> mailing list) that does not make it possible for a local process anymore
> to replace canaries which in turn might disable SSP. Comments and/or
> review are much appreciated.
>
> The problem it is trying to solve is outlined in
> https://trac.torproject.org/13169#comment:4:
>
> (Quoting a cypherpunk)
>
> "In Windows you can to create any directories for any disks(C:, D:, ..
> Z:), only system directories (Windows directory, Program files, etc) are
> protected. Any process with privileges of any standard user can to
> create C:\dev\urandom file and to fill it by any stuff."
>
> And now the patch (arguably the subject can be a bit more elaborate):
Thanks for the patch, but it belongs to GCC and as such gcc-patches ML
is the right place to send it.
I have nothing to do with libssp nor have much experience with GCC
patches, but I have some suggestions:
> From 83e3ea38a5720df52bd5f78dcd3c7b7b842ddc3b Mon Sep 17 00:00:00 2001
> From: Erinn Clark <er...@torproject.org>
> Date: Wed, 12 Mar 2014 16:09:10 +0100
> Subject: [PATCH] skruffy patch
>
> ---
> libssp/ssp.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/libssp/ssp.c b/libssp/ssp.c
> index aaa5a32..37f4e27 100644
> --- a/libssp/ssp.c
> +++ b/libssp/ssp.c
> @@ -55,6 +55,7 @@ see the files COPYING3 and COPYING.RUNTIME
> respectively. If not, see
> /* Native win32 apps don't know about /dev/tty but can print directly
> to the console using "CONOUT$" */
> #if defined (_WIN32) && !defined (__CYGWIN__)
> +#include <windows.h>
> # define _PATH_TTY "CONOUT$"
> #else
> # define _PATH_TTY "/dev/tty"
> @@ -75,6 +76,20 @@ __guard_setup (void)
> if (__stack_chk_guard != 0)
> return;
>
> +#if defined (_WIN32) && !defined (__CYGWIN__)
> + HCRYPTPROV hprovider = 0;
Looking at the rest of the file, it seems that it maintains C89
compatibility or at least coding style uses declarations in the
beginning of the block.
> + if (CryptAcquireContext(&hprovider, NULL, NULL, PROV_RSA_FULL,
> + CRYPT_VERIFYCONTEXT | CRYPT_SILENT))
> + {
> + if (CryptGenRandom(hprovider, sizeof (__stack_chk_guard),
> + (BYTE *)&__stack_chk_guard) && __stack_chk_guard != 0)
> + {
> + CryptReleaseContext(hprovider, 0);
> + return;
> + }
> + CryptReleaseContext(hprovider, 0);
> + }
Did you consider using RtlGenRandom instead? This would simplify the code.
Another alternative would be using rand_s, which is a variant of rand
that doesn't require srand (see Wine implementation for an example [1]).
Its problem is, however, that it returns int, which is inconvenient for
64-bit arch in this case.
> +#else
> fd = open ("/dev/urandom", O_RDONLY);
> if (fd != -1)
> {
> @@ -85,6 +100,7 @@ __guard_setup (void)
> return;
> }
>
> +#endif
> /* If a random generator can't be used, the protector switches the guard
> to the "terminator canary". */
> p = (unsigned char *) &__stack_chk_guard;
> --
> 1.7.10.4
Thanks,
Jacek
[1] http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/msvcrt/misc.c#l68
------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
