NetBSD Problem Report #58468

From www@netbsd.org  Fri Jul 26 14:06:39 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 707C31A9239
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 26 Jul 2024 14:06:39 +0000 (UTC)
Message-Id: <20240726140638.86D621A923B@mollari.NetBSD.org>
Date: Fri, 26 Jul 2024 14:06:38 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: kernel libsodium import needs review and crypto self-tests
X-Send-Pr-Version: www-1.0

>Number:         58468
>Category:       kern
>Synopsis:       kernel libsodium import needs review and crypto self-tests
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    riastradh
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Jul 26 14:10:00 +0000 2024
>Closed-Date:    
>Last-Modified:  Wed Oct 09 12:45:02 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current, 10
>Organization:
The NaBSD Foundation
>Environment:
>Description:
The import of libsodium into the kernel, under sys/crypto/sodium and sys/external/isc/libsodium, has some glue code that needs review, and as a crypto component the functionality needs self-tests.

Fortunately nothing uses it besides the still-experimental-and-not-production-ready wg(4).
>How-To-Repeat:
code inspection
>Fix:
Yes, please!

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: kern-bug-people->riastradh
Responsible-Changed-By: riastradh@NetBSD.org
Responsible-Changed-When: Fri, 26 Jul 2024 16:53:21 +0000
Responsible-Changed-Why:
my import, my responsibility


From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58468 CVS commit: src/sys
Date: Fri, 26 Jul 2024 18:25:03 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Jul 26 18:25:03 UTC 2024

 Modified Files:
 	src/sys/external/isc/libsodium/conf: files.libsodium
 	src/sys/external/isc/libsodium/src: sodium_module.c
 	src/sys/modules/sodium: Makefile.sodmod
 	src/sys/rump/kern/lib/libcrypto: Makefile
 Added Files:
 	src/sys/crypto/sodium: sodium_selftest.h
 	src/sys/external/isc/libsodium/src: sodium_selftest.c

 Log Message:
 sys/crypto/sodium: Add a self-test for IETF ChaCha20/Poly1305 AEAD.

 PR kern/58468


 To generate a diff of this commit:
 cvs rdiff -u -r0 -r1.1 src/sys/crypto/sodium/sodium_selftest.h
 cvs rdiff -u -r1.6 -r1.7 src/sys/external/isc/libsodium/conf/files.libsodium
 cvs rdiff -u -r1.1 -r1.2 src/sys/external/isc/libsodium/src/sodium_module.c
 cvs rdiff -u -r0 -r1.1 src/sys/external/isc/libsodium/src/sodium_selftest.c
 cvs rdiff -u -r1.3 -r1.4 src/sys/modules/sodium/Makefile.sodmod
 cvs rdiff -u -r1.23 -r1.24 src/sys/rump/kern/lib/libcrypto/Makefile

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58468 CVS commit: src/sys/external/isc/libsodium/include
Date: Fri, 26 Jul 2024 18:27:48 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Jul 26 18:27:48 UTC 2024

 Modified Files:
 	src/sys/external/isc/libsodium/include: crypto_verify_16.h

 Log Message:
 sys/crypto/sodium: Fill out crypto_verify_16 stub.

 Without this change, libsodium silently accepts forgeries.

 This one's a doozy, and it's a sobering reminder that:

 (a) wg(4) is still experimental (only user of libsodium in kernel;
     both are available only through default-off optional modules).

 (b) Known-answer test vectors are critical, including negative tests
     (test that forgeries are rejected), and must be mandatory for all
     new crypto code -- and should be added to old crypto code too.

 (c) Crypto code must also have self-tests that run in the same
     environment, not just the same code in a different build or test
     environment -- the libsodium code itself is fine, but we built it
     differently and need to exercise it differently from upstream's
     automatic tests.

 It's my fault for not catching this earlier.  What happened is:

 1. ozaki-r@ adapted libsodium to build in the kernel with various
    glue to build code meant for standard userland C, like errno.h and
    string.h.

 2. Since libsodium's crypto_verify_16.c uses various SIMD intrinsics
    on various architectures, it couldn't be used directly in the
    kernel build, because -- at the time -- we hadn't wired up any
    header files for SIMD intrinsics or any runtime support for saving
    and restoring SIMD state appropriately in the kernel.

 3. ozaki-r@ put a similar glue header file crypto_verify_16.h to
    override libsodium's, with a stub to be implemented later, and
    presumably forgot to remind me about it.

 4. I missed the stub in crypto_verify_16.h when reviewing the
    libsodium import and wg(4) code because it was in the same
    directory as various other simple glue code that I deemed
    low-risk.

    (I did make one change to that glue code, to replace cprng_fast by
    cprng_strong, but I suspect I found that by searching for
    cprng_fast users rather than by reviewing this code.)

 5. I broke my own rule about always having known-answer test vectors
    for crypto code because I figured libsodium was well-enough
    exercised that we could skimp on it for now, and my focus was more
    on the state machine and synchronization logic than on the crypto.

 6. I had not yet written known-answer test vectors for the
    higher-level wg(4) protocol messages.

 Before we can remove the `experimental' tag from wg(4) we will need
 to (among other things):

   i. Write self-tests for the rest of (what we use from) libsodium.
  ii. Write extensive known-answer test vectors for all the wg(4)
      protocol messages (and ideally state machine transitions).
 iii. Write self-tests for a reasonable subset of the wg(4) KATs.
  iv. Review all of the libsodium glue code I neglected to review.

 PR kern/58468


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r1.2 \
     src/sys/external/isc/libsodium/include/crypto_verify_16.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58468 CVS commit: src/sys/external/isc/libsodium/include
Date: Fri, 26 Jul 2024 18:28:27 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Jul 26 18:28:27 UTC 2024

 Modified Files:
 	src/sys/external/isc/libsodium/include: string.h

 Log Message:
 sys/crypto/sodium: Simplify string.h stub.

 Not sure of any particular problem with the previous stub, but let's
 make sure to use the same prototypes for memset/memcpy/memmove as
 everything else in the kernel.

 PR kern/58468


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r1.2 src/sys/external/isc/libsodium/include/string.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58468 CVS commit: src/sys/external/isc/libsodium/include
Date: Fri, 26 Jul 2024 18:29:03 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Jul 26 18:29:03 UTC 2024

 Removed Files:
 	src/sys/external/isc/libsodium/include: assert.h

 Log Message:
 sys/crypto/sodium: Nix unused assert.h stub.

 Maybe this was a vestige of an earlier draft of the libsodium import,
 but it doesn't appear to be needed now by any libsodium files we use.

 PR kern/58468


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r0 src/sys/external/isc/libsodium/include/assert.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58468 CVS commit: src/sys/external/isc/libsodium/include
Date: Fri, 26 Jul 2024 18:29:50 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Jul 26 18:29:50 UTC 2024

 Modified Files:
 	src/sys/external/isc/libsodium/include: core.h

 Log Message:
 sys/crypto/sodium: Nix risky defines from core.h stub.

 These are risky not because they might cause crypto flaws, but
 because they might cause usage of the SIMD unit in the kernel along
 paths where we haven't made it safe.

 That said -- no change to the amd64 module .o and .kmod files, so
 this doesn't currently make a difference; it's just risky to have
 around in case we later include other parts of libsodium that it does
 affect, like the Salsa20 code.

 PR kern/58468


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r1.2 src/sys/external/isc/libsodium/include/core.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58468 CVS commit: src/sys/external/isc/libsodium/include
Date: Fri, 26 Jul 2024 18:30:28 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Jul 26 18:30:27 UTC 2024

 Removed Files:
 	src/sys/external/isc/libsodium/include: errno.h

 Log Message:
 sys/crypto/sodium: Nix unused errno.h.

 Maybe this was a vestige of an earlier draft of the libsodium import,
 but it doesn't appear to be needed now by any libsodium files we use.

 PR kern/58468


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r0 src/sys/external/isc/libsodium/include/errno.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58468 CVS commit: src/sys/external/isc/libsodium/include
Date: Fri, 26 Jul 2024 18:30:44 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Jul 26 18:30:43 UTC 2024

 Modified Files:
 	src/sys/external/isc/libsodium/include: stdint.h

 Log Message:
 sys/crypto/sodium: Simplify stdint.h stub.

 No change to the .o or .kmod files; just the .d make dependency files
 change.

 PR kern/58468


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r1.2 src/sys/external/isc/libsodium/include/stdint.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58468 CVS commit: src/sys/external/isc/libsodium/include
Date: Fri, 26 Jul 2024 18:31:45 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Jul 26 18:31:45 UTC 2024

 Modified Files:
 	src/sys/external/isc/libsodium/include: stdlib.h

 Log Message:
 sys/crypto/sodium: Tighten stdlib.h glue.

 1. Make sure nothing uses malloc and free.  All of the routines we
    need should work in fixed-size, caller-allocated buffers and
    reasonable stack space.

 2. Make panic message for abort() stub clearer.  There are calls to
    it, but they imply internal errors inside libsodium which should
    not happen unless there is an unrecoverable software bug in
    libsodium.

 PR kern/58468


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r1.2 src/sys/external/isc/libsodium/include/stdlib.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58468 CVS commit: src/sys
Date: Fri, 26 Jul 2024 18:32:15 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Jul 26 18:32:15 UTC 2024

 Modified Files:
 	src/sys/crypto/sodium: sodium_selftest.h
 	src/sys/external/isc/libsodium/src: sodium_selftest.c

 Log Message:
 sys/crypto/sodium: Add self-test for XChaCha20/Poly1305 AEAD.

 PR kern/58468


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r1.2 src/sys/crypto/sodium/sodium_selftest.h
 cvs rdiff -u -r1.1 -r1.2 src/sys/external/isc/libsodium/src/sodium_selftest.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->pending-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Tue, 08 Oct 2024 18:31:56 +0000
State-Changed-Why:
pullup-10 #933: https://releng.netbsd.org/cgi-bin/req-10.cgi?show=933

Still needs review once this is pulled up, so return to state open.
(Need to check whether we need more self-tests.)


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58468 CVS commit: [netbsd-10] src/sys
Date: Wed, 9 Oct 2024 10:49:04 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Wed Oct  9 10:49:04 UTC 2024

 Modified Files:
 	src/sys/external/isc/libsodium/conf [netbsd-10]: files.libsodium
 	src/sys/external/isc/libsodium/include [netbsd-10]: core.h
 	    crypto_verify_16.h stdint.h stdlib.h string.h
 	src/sys/external/isc/libsodium/src [netbsd-10]: sodium_module.c
 	src/sys/modules/sodium [netbsd-10]: Makefile.sodmod
 	src/sys/rump/kern/lib/libcrypto [netbsd-10]: Makefile
 Added Files:
 	src/sys/crypto/sodium [netbsd-10]: sodium_selftest.h
 	src/sys/external/isc/libsodium/src [netbsd-10]: sodium_selftest.c
 Removed Files:
 	src/sys/external/isc/libsodium/include [netbsd-10]: assert.h errno.h

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #933):

 	sys/external/isc/libsodium/src/sodium_module.c: revision 1.2
 	sys/external/isc/libsodium/include/core.h: revision 1.2
 	sys/external/isc/libsodium/include/stdlib.h: revision 1.2
 	sys/modules/sodium/Makefile.sodmod: revision 1.4
 	sys/external/isc/libsodium/include/crypto_verify_16.h: revision 1.2
 	sys/external/isc/libsodium/include/errno.h: file removal
 	sys/crypto/sodium/sodium_selftest.h: revision 1.1
 	sys/external/isc/libsodium/include/stdint.h: revision 1.2
 	sys/crypto/sodium/sodium_selftest.h: revision 1.2
 	sys/external/isc/libsodium/include/assert.h: file removal
 	sys/external/isc/libsodium/conf/files.libsodium: revision 1.7
 	sys/rump/kern/lib/libcrypto/Makefile: revision 1.24
 	sys/external/isc/libsodium/src/sodium_selftest.c: revision 1.1
 	sys/external/isc/libsodium/src/sodium_selftest.c: revision 1.2
 	sys/external/isc/libsodium/include/string.h: revision 1.2

 sys/crypto/sodium: Add a self-test for IETF ChaCha20/Poly1305 AEAD.
 PR kern/58468

 sys/crypto/sodium: Fill out crypto_verify_16 stub.

 Without this change, libsodium silently accepts forgeries.

 This one's a doozy, and it's a sobering reminder that:
 (a) wg(4) is still experimental (only user of libsodium in kernel;
     both are available only through default-off optional modules).
 (b) Known-answer test vectors are critical, including negative tests
     (test that forgeries are rejected), and must be mandatory for all
     new crypto code -- and should be added to old crypto code too.
 (c) Crypto code must also have self-tests that run in the same
     environment, not just the same code in a different build or test
     environment -- the libsodium code itself is fine, but we built it
     differently and need to exercise it differently from upstream's
     automatic tests.

 It's my fault for not catching this earlier.  What happened is:
 1. ozaki-r@ adapted libsodium to build in the kernel with various
    glue to build code meant for standard userland C, like errno.h and
    string.h.
 2. Since libsodium's crypto_verify_16.c uses various SIMD intrinsics
    on various architectures, it couldn't be used directly in the
    kernel build, because -- at the time -- we hadn't wired up any
    header files for SIMD intrinsics or any runtime support for saving
    and restoring SIMD state appropriately in the kernel.
 3. ozaki-r@ put a similar glue header file crypto_verify_16.h to
    override libsodium's, with a stub to be implemented later, and
    presumably forgot to remind me about it.
 4. I missed the stub in crypto_verify_16.h when reviewing the
    libsodium import and wg(4) code because it was in the same
    directory as various other simple glue code that I deemed
    low-risk.
    (I did make one change to that glue code, to replace cprng_fast by
    cprng_strong, but I suspect I found that by searching for
    cprng_fast users rather than by reviewing this code.)
 5. I broke my own rule about always having known-answer test vectors
    for crypto code because I figured libsodium was well-enough
    exercised that we could skimp on it for now, and my focus was more
    on the state machine and synchronization logic than on the crypto.
 6. I had not yet written known-answer test vectors for the
    higher-level wg(4) protocol messages.

 Before we can remove the `experimental' tag from wg(4) we will need
 to (among other things):
   i. Write self-tests for the rest of (what we use from) libsodium.
  ii. Write extensive known-answer test vectors for all the wg(4)
      protocol messages (and ideally state machine transitions).
 iii. Write self-tests for a reasonable subset of the wg(4) KATs.
  iv. Review all of the libsodium glue code I neglected to review.
 PR kern/58468

 sys/crypto/sodium: Simplify string.h stub.

 Not sure of any particular problem with the previous stub, but let's
 make sure to use the same prototypes for memset/memcpy/memmove as
 everything else in the kernel.
 PR kern/58468

 sys/crypto/sodium: Nix unused assert.h stub.

 Maybe this was a vestige of an earlier draft of the libsodium import,
 but it doesn't appear to be needed now by any libsodium files we use.
 PR kern/58468

 sys/crypto/sodium: Nix risky defines from core.h stub.

 These are risky not because they might cause crypto flaws, but
 because they might cause usage of the SIMD unit in the kernel along
 paths where we haven't made it safe.

 That said -- no change to the amd64 module .o and .kmod files, so
 this doesn't currently make a difference; it's just risky to have
 around in case we later include other parts of libsodium that it does
 affect, like the Salsa20 code.
 PR kern/58468

 sys/crypto/sodium: Nix unused errno.h.

 Maybe this was a vestige of an earlier draft of the libsodium import,
 but it doesn't appear to be needed now by any libsodium files we use.
 PR kern/58468

 sys/crypto/sodium: Simplify stdint.h stub.
 No change to the .o or .kmod files; just the .d make dependency files
 change.
 PR kern/58468

 sys/crypto/sodium: Tighten stdlib.h glue.
 1. Make sure nothing uses malloc and free.  All of the routines we
    need should work in fixed-size, caller-allocated buffers and
    reasonable stack space.
 2. Make panic message for abort() stub clearer.  There are calls to
    it, but they imply internal errors inside libsodium which should
    not happen unless there is an unrecoverable software bug in
    libsodium.
 PR kern/58468

 sys/crypto/sodium: Add self-test for XChaCha20/Poly1305 AEAD.
 PR kern/58468


 To generate a diff of this commit:
 cvs rdiff -u -r0 -r1.2.2.2 src/sys/crypto/sodium/sodium_selftest.h
 cvs rdiff -u -r1.6 -r1.6.4.1 \
     src/sys/external/isc/libsodium/conf/files.libsodium
 cvs rdiff -u -r1.1 -r0 src/sys/external/isc/libsodium/include/assert.h \
     src/sys/external/isc/libsodium/include/errno.h
 cvs rdiff -u -r1.1 -r1.1.20.1 src/sys/external/isc/libsodium/include/core.h \
     src/sys/external/isc/libsodium/include/crypto_verify_16.h \
     src/sys/external/isc/libsodium/include/stdint.h \
     src/sys/external/isc/libsodium/include/stdlib.h \
     src/sys/external/isc/libsodium/include/string.h
 cvs rdiff -u -r1.1 -r1.1.4.1 \
     src/sys/external/isc/libsodium/src/sodium_module.c
 cvs rdiff -u -r0 -r1.2.2.2 \
     src/sys/external/isc/libsodium/src/sodium_selftest.c
 cvs rdiff -u -r1.3 -r1.3.4.1 src/sys/modules/sodium/Makefile.sodmod
 cvs rdiff -u -r1.23 -r1.23.4.1 src/sys/rump/kern/lib/libcrypto/Makefile

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->open
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Wed, 09 Oct 2024 12:45:02 +0000
State-Changed-Why:
Changes so far are pulled up to 10, still need more self-tests.


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.47 2022/09/11 19:34:41 kim Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2024 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.