Re: [PATCH 0/8] Implement Curve448 ECDH and Ed448
Hello Niels, Daiki Ueno writes: > ni...@lysator.liu.se (Niels Möller) writes: > >> I think there are three main pieces left to integrate. >> >> 1. Curve operations to support Curve448 (i.e., diffie-hellman >>operations). I have made some progress, on my curve448 branch, >> >> 2. SHAKE 128/256. I think I had some question on the interface design. >> >> 3. EdDSA 448. >> >> Optimization of the mod p arithmetic isn't that important yet, Is there anything I can do to get this merged in upstream? Now that OpenSSL supports the curve and EdDSA, it would be interesting to add it in GnuTLS. I tried to integrate it into GnuTLS bundling the current code, and it can interoperate with OpenSSL: https://gitlab.com/gnutls/gnutls/merge_requests/984 For convenience I am attaching the remaining patches for nettle. Regards, -- Daiki Ueno ___ nettle-bugs mailing list nettle-bugs@lists.lysator.liu.se http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs
Re: Intel CET protection
Simo Sorce writes: > I understand this is not a high bar, and I will fold the segment note > in if you think that is what we should do, but I wanted to make you > aware of why I did not do the same as what we do with the stack note. I think we should aim to make all files "cet-compliant" if we do it att all. After all, the common case is to have a libnettle.so, and then any single object file missing the annotation will make the linker disable the feature, if I've understood it correctly. I agree it should be disabled by default until we're more confident in test coverage. BTW, do you know how that works with late loading using dlopen? One could have a process running in CET-mode, which dynamically loads an so-file with code lacking endbr instructions and corresponding annotation. If we think about it as an arch-specific thing, which I guess we should, then maybe the m4 divert should be in x86_64/machine.m4 and x86/machine.m4, not asm.m4. > That is funny, I actually used CET_ENDBR to make it easier to find for > others grepping as binutils also uses a _CET_ENDBR macro, it sounded > more consistent I agree your name is better for readability, even if less amusing. >> I'd like to understand what's missing. Maybe we can fix it more >> explicitly? > > I do not think we can easily fix it manually, it is mostly other > section notes that the gcc compiler adds when it fortifies C code. But > those notes do not really apply to handcrafted assembly. [...] > So this flag > is basically just saying to the compiler that it should add whatever is > appropriate (which may change depending on compiler flags) because our > code is good as is. Since the command line flag is passed with -Wa, it tells the *assembler* to add notes. Which ones? Is it based only on the command line, say, $(CFLAGS) contains -fharden-foo makes the assembler produces a "foo" note. Or is it based on what's actually in the assembly input file? Is there a risk that it automatically generates a note promising something about the assembly code which we don't actually fulfill? Is there any documentation? I see that it is mentioned in the binutils-2.31 release announcement, but I've not found it mentioned in the Gas manual. > https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf > (I do no have a better link) Looks like reasonable doc. (Closest on wikpedia seems to be https://en.wikipedia.org/wiki/Control-flow_integrity). > See above explanation and let me know if that changes your opinion, > otherwise I will do this. > >> > +ALIGN(8) >> > +.long 1f - 0f >> > +.long 4f - 1f >> > +.long 5 >> > +0: >> > +.string "GNU" >> > +1: >> > +ALIGN(8) >> > +.long 0xc002 >> > +.long 3f - 2f >> > +2: >> > +.long 0x03 >> > +3: >> > +ALIGN(8) >> > +4: >> >> Are there no symbolic names for this note? Since we require assembler to >> suport endbr instructions, can we require that it know about these notes >> as well? What does it look like in gcc output? > > There are symbolic names to compose the 0x03 value and for the > 0xc002 values, the rest are just label arithmetic. > > I will change in next submission. I see, I was hoping for something more similar to .section .note.GNU-stack,"",TYPE_PROGBITS I'm still curious as to what it looks like in gcc output. Regards, /Niels -- Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677. Internet email is subject to wholesale government surveillance. ___ nettle-bugs mailing list nettle-bugs@lists.lysator.liu.se http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs
Re: Intel CET protection
Ok attached find new patches, they address all concerns except for adding the CET_SECTION macro automagically to all asm files. I also added a patch to deal with the missing epilogues. Simo. -- Simo Sorce Sr. Principal Software Engineer Red Hat, Inc From 8c41a89ed3ef913bc8a12f8e6d21edf3627007ee Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Tue, 23 Apr 2019 18:03:35 -0400 Subject: [PATCH 1/3] Add Intel CET protection support In upcoming processors Intel will make available Control-Flow Enforcement Technology, which is comprised of two hardware countermeasures against Return-Oriented Programming attacks. The first is called Shadow Stack and checks that return from function calls are not tampered with by keeping a shadow stack that cannot be modified by applications. This measure requires no code changes (except for code that intentionally modifies the return pointer on the stack). The second is called Indirect Branch Tracking and is used to insure only targets of indirect jumps are actually jumped to. This requires modification of code to insert a special instruction that identifies a valid indirect jump target. When enforcement is turned on, if an indirect jump does not end on this special instruction the cpu raises an exception. These instructions are noops on older CPU models so it is safe to use them in all x86(_64) code. To enable these protections GCC also introduces a new GNU property note section that marks a piece of code as CET ready. If the note is in place the dynamic linker will be able to confirm that all loaded libraries support CET and will turn on CET protection for the binary. The changes here consist mostly in adding the GNU property note section to all x86(_64) assembly files and the proper ENDBRANCH instruction for the function entrypoints which is where other code calls into via indirect call. Signed-off-by: Simo Sorce --- asm.m4| 36 ++- config.m4.in | 2 ++ configure.ac | 17 + x86/aes-decrypt-internal.asm | 1 + x86/aes-encrypt-internal.asm | 1 + x86/arcfour-crypt.asm | 1 + x86/camellia-crypt-internal.asm | 1 + x86/md5-compress.asm | 1 + x86/sha1-compress.asm | 1 + x86_64/aes-decrypt-internal.asm | 1 + x86_64/aes-encrypt-internal.asm | 1 + x86_64/aesni/aes-decrypt-internal.asm | 1 + x86_64/aesni/aes-encrypt-internal.asm | 1 + x86_64/camellia-crypt-internal.asm| 1 + x86_64/chacha-core-internal.asm | 1 + x86_64/fat/cpuid.asm | 2 +- x86_64/gcm-hash8.asm | 1 + x86_64/md5-compress.asm | 1 + x86_64/memxor.asm | 1 + x86_64/memxor3.asm| 1 + x86_64/poly1305-internal.asm | 1 + x86_64/salsa20-core-internal.asm | 1 + x86_64/salsa20-crypt.asm | 1 + x86_64/serpent-decrypt.asm| 2 ++ x86_64/serpent-encrypt.asm| 2 ++ x86_64/sha1-compress.asm | 1 + x86_64/sha256-compress.asm| 1 + x86_64/sha3-permute.asm | 1 + x86_64/sha512-compress.asm| 1 + x86_64/sha_ni/sha1-compress.asm | 1 + x86_64/sha_ni/sha256-compress.asm | 1 + x86_64/umac-nh-n.asm | 1 + x86_64/umac-nh.asm| 1 + 33 files changed, 86 insertions(+), 2 deletions(-) diff --git a/asm.m4 b/asm.m4 index 8da47201..9622906e 100644 --- a/asm.m4 +++ b/asm.m4 @@ -32,7 +32,8 @@ define(,<>)dnl define(, <.globl C_NAME($1) DECLARE_FUNC(C_NAME($1)) -C_NAME($1):>) +C_NAME($1): +CET_ENDBR>) define(, , m4exit(1)>)>) define(, , <$1>)>) +dnl GNU properties section to enable CET protections macros + +dnl GNU Poperty type +define(, <5>) +dnl GNU Program Property Type range +define(GNU_PROPERTY_X86_UINT32_AND_LO, <0xc002>) +dnl Indirect Branch Tracking +define(, <0x01>) +dnl Shadow Stack +define(, <0x02>) + +dnl NOTE: GNU Property sections MUST have alignment of 8 +define(, +,<>)>) + dnl Struct defining macros dnl STRUCTURE(prefix) diff --git a/config.m4.in b/config.m4.in index 11f90a40..c3ebad60 100644 --- a/config.m4.in +++ b/config.m4.in @@ -8,6 +8,8 @@ define(, <@ASM_ALIGN_LOG@>)dnl define(, <@W64_ABI@>)dnl define(, <@ASM_RODATA@>)dnl define(, <@ASM_WORDS_BIGENDIAN@>)dnl +define(, <@ASM_CET_PROTECTION@>)dnl +define(, <@ASM_CET_ENDBR@>)dnl divert(1) @ASM_MARK_NOEXEC_STACK@ divert diff --git a/configure.ac b/configure.ac index 00d2bf5d..6e12bb1f 100644 --- a/configure.ac +++ b/configure.ac @@ -93,6 +93,10 @@ AC_ARG_ENABLE(mini-gmp, AC_HELP_STRING([--enable-mini-gmp], [Enable mini-gmp, used instead of libgmp.]),, [enable_mini_gmp=no]) +AC_ARG_ENABLE(cet-protection, + AC_HELP_STRING([--enable-cet-protection], [Enable intel CET protection instructions. (default=no)]),, + [enable_cet_protection=no]) + if test
Re: Intel CET protection
On Fri, 2019-04-26 at 10:45 +0200, Niels Möller wrote: > Simo Sorce writes: > > > the attached patches have been used to successfully enable and test > > Intel CET support in an Intel emulator on SDV hardware. > > Thanks. > > > GCC already has all the needed support to create CET hardened code, > > however the hand-coded assembly needs to be changed to conform. > > Without these changes all the binaries that load nettle will otherwise > > have CET disabled, as it is an all-or-nothing at the binary level and > > missing ENDBRANCH instruction cause the program to terminate on > > indirect jump/call instructions. > > For the .note.gnu.property thing (which is per object file, right?), I > think it's better to do it in the same place as ASM_MARK_NOEXEC_STACK. > That's substituted in config.m4.in, and added at the end of each asm file > using m4 divert. So the reason I did not do that an instead explicitly added a statement in the individual .asm files is that you cannot just add the note on any random file. The note states that the file uses CET instructions for all indirect jump targets. So, it just so happens now that no indirect jumps (except for the target of the initial call into the functions) are used in our handcrafted assembly, but that is not a given in the future. So want I mean to say is that I took an explicit macro to mean the file was actually analyzed and made CET compliant. If we embed the macro magically instead this "inspection" part will be missing and we may end up marking assembly files that do nbot in fact comply with CET and will make the program segfault (that's how the exception is surfaced) when run. I understand this is not a high bar, and I will fold the segment note in if you think that is what we should do, but I wanted to make you aware of why I did not do the same as what we do with the stack note. > The endbr instructions in the prologue are in the right place, as far as > I can tell. And I'd be very tempted to rename the macro CET_ENDBR to > COME_FROM, see https://en.wikipedia.org/wiki/COMEFROM ;-) That is funny, I actually used CET_ENDBR to make it easier to find for others grepping as binutils also uses a _CET_ENDBR macro, it sounded more consistent to use something with both "CET" and ENDBR in it, easier to find for someone looking and figuring out hwat it is about. > > The second patch is used to make the system happy when hardening flags > > are enabled in gcc, as it generates the appropriate section information > > that tells the linker all is good. > > I'd like to understand what's missing. Maybe we can fix it more > explicitly? I do not think we can easily fix it manually, it is mostly other section notes that the gcc compiler adds when it fortifies C code. But those notes do not really apply to handcrafted assembly. So this flag is basically just saying to the compiler that it should add whatever is appropriate (which may change depending on compiler flags) because our code is good as is. It silence some checking tools mostly, so I do not think it is worth wasting too much time trying to figure out what notes should be added, the compiler is happy to do it for us. > > Finally while looking at the assembly I noticed that some functions > > have a PROLOGUE() defined but not an EPILOGUE() macro defined in their > > .asm files. It is unclear to me if this is an error or intentional so > > didn't touch those, it doesn't affect functionality for this patch > > anyway. > > I think it's an error. Except in the files in arm/fat with lines like > > dnl PROLOGUE(_nettle_chacha_core) picked up by configure > > I find three offending files, using > > $ diff <(git grep -c '^ *EPILOGUE') <(git grep -c '^ *PROLOGUE') > 49c49 > < x86_64/poly1305-internal.asm:2 > --- > > x86_64/poly1305-internal.asm:3 > 51a52,53 > > x86_64/serpent-decrypt.asm:1 > > x86_64/serpent-encrypt.asm:1 > > have you seen any others? No, those are pretty much the places where I noticed it. Would you want an additional patch that adds those EPILOGUES ? > Some minor comments below. > > > From de1b9bfeb4f8ad9a6bf8608c4b8c727dba315982 Mon Sep 17 00:00:00 2001 > > From: Simo Sorce > > Date: Tue, 23 Apr 2019 18:03:35 -0400 > > Subject: [PATCH 1/2] Add Intel CET protection support > > > > In upcoming processors Intel will make available Control-Flow > > Enforcement Technology, which is comprised of two hardware > > countermeasures against ROP based attacks. > > Please spell out ROP. It would be good with a link to further > information in some reasonable place in the code. Should I add a link in asm.m4 to this ? https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf (I do no have a better link) > > The first is called Shadow Stack and checks that return from function > > calls are not tampered with by keeping a shadow stack that cannot be > > modified by aplications. This measure requires no code changes (except > >
Re: Intel CET protection
Simo Sorce writes: > the attached patches have been used to successfully enable and test > Intel CET support in an Intel emulator on SDV hardware. Thanks. > GCC already has all the needed support to create CET hardened code, > however the hand-coded assembly needs to be changed to conform. > Without these changes all the binaries that load nettle will otherwise > have CET disabled, as it is an all-or-nothing at the binary level and > missing ENDBRANCH instruction cause the program to terminate on > indirect jump/call instructions. For the .note.gnu.property thing (which is per object file, right?), I think it's better to do it in the same place as ASM_MARK_NOEXEC_STACK. That's substituted in config.m4.in, and added at the end of each asm file using m4 divert. The endbr instructions in the prologue are in the right place, as far as I can tell. And I'd be very tempted to rename the macro CET_ENDBR to COME_FROM, see https://en.wikipedia.org/wiki/COMEFROM ;-) > The second patch is used to make the system happy when hardening flags > are enabled in gcc, as it generates the appropriate section information > that tells the linker all is good. I'd like to understand what's missing. Maybe we can fix it more explicitly? > Finally while looking at the assembly I noticed that some functions > have a PROLOGUE() defined but not an EPILOGUE() macro defined in their > .asm files. It is unclear to me if this is an error or intentional so > didn't touch those, it doesn't affect functionality for this patch > anyway. I think it's an error. Except in the files in arm/fat with lines like dnl PROLOGUE(_nettle_chacha_core) picked up by configure I find three offending files, using $ diff <(git grep -c '^ *EPILOGUE') <(git grep -c '^ *PROLOGUE') 49c49 < x86_64/poly1305-internal.asm:2 --- > x86_64/poly1305-internal.asm:3 51a52,53 > x86_64/serpent-decrypt.asm:1 > x86_64/serpent-encrypt.asm:1 have you seen any others? Some minor comments below. > From de1b9bfeb4f8ad9a6bf8608c4b8c727dba315982 Mon Sep 17 00:00:00 2001 > From: Simo Sorce > Date: Tue, 23 Apr 2019 18:03:35 -0400 > Subject: [PATCH 1/2] Add Intel CET protection support > > In upcoming processors Intel will make available Control-Flow > Enforcement Technology, which is comprised of two hardware > countermeasures against ROP based attacks. Please spell out ROP. It would be good with a link to further information in some reasonable place in the code. > The first is called Shadow Stack and checks that return from function > calls are not tampered with by keeping a shadow stack that cannot be > modified by aplications. This measure requires no code changes (except > for code that intentionally modifes the return pointer on the stack). Fix spelling of "aplication", "modifes" > The second is called Indirect Branch Tracking and is used to insure only > targets of indirect jumps are actually jumped to. This requires > modification of code to insert a special instruction that identifies a > valid indirect jump target. When enforcement is turned on, if an indirect > jump does not end on this special instruction the cpu raises an exception. > These instructions are noops on older CPU models so it is safe to use > them in all x86(_64) code. > > To enable these protections gcc also inroduces a new GNU property note and "inroduces" > +dnl GNU properties section to enable CET protections > +define(, > + +<.pushsection .note.gnu.property,"a" As I said, prefer to dot his globally with an m4 divert in config.m4.in. > +ALIGN(8) > +.long 1f - 0f > +.long 4f - 1f > +.long 5 > +0: > +.string "GNU" > +1: > +ALIGN(8) > +.long 0xc002 > +.long 3f - 2f > +2: > +.long 0x03 > +3: > +ALIGN(8) > +4: Are there no symbolic names for this note? Since we require assembler to suport endbr instructions, can we require that it know about these notes as well? What does it look like in gcc output? > diff --git a/x86_64/aes-decrypt-internal.asm b/x86_64/aes-decrypt-internal.asm > index 43f2f394..5db39868 100644 > --- a/x86_64/aes-decrypt-internal.asm > +++ b/x86_64/aes-decrypt-internal.asm > @@ -62,7 +62,7 @@ C work. > define(,<%rbp>) > > .file "aes-decrypt-internal.asm" > - > + > C _aes_decrypt(unsigned rounds, const uint32_t *keys, > C const struct aes_table *T, > C size_t length, uint8_t *dst, > @@ -70,6 +70,7 @@ define(,<%rbp>) > .text > ALIGN(16) > PROLOGUE(_nettle_aes_decrypt) > + Please trim unrelated whitespace changes from the patch (I known it's not 100% consistent, but if we ever want to do something like M-x whitespace-cleanup on all files, that should be in a separate commit. On new changes, I usually run git diff --check to catch odd whitespace use). Regards, /Niels -- Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677. Internet email is subject to wholesale government surveillance. ___ nettle-bugs mailing list
